| Summary: | [macOS] [WK2] Support changing foreground colors via color panel | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> |
| Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | bdakin, commit-queue, ews-watchlist, mitz, rniwa, thorton, tsavell, webkit-bug-importer, wenson_hsieh |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Wenson Hsieh
2018-09-06 16:29:14 PDT
Created attachment 349376 [details]
Patch
Comment on attachment 349376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349376&action=review > Tools/TestWebKitAPI/Tests/mac/FontManagerTests.mm:276 > + checkFontColorAtStartAndEndWithInputEvents("rgb(255, 0, 0)"); Let's also add an assertion to make sure we're inserting a font element instead of a span in this case. > Tools/TestWebKitAPI/Tests/mac/FontManagerTests.mm:287 > + checkFontColorAtStartAndEndWithInputEvents("rgb(204, 179, 51)"); Ditto. Comment on attachment 349376 [details] Patch Attachment 349376 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9169149 New failing tests: editing/style/inline-style-container.html Created attachment 349379 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 349376 [details] Patch Attachment 349376 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9169220 New failing tests: editing/style/inline-style-container.html Created attachment 349380 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349376 [details] Patch Attachment 349376 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9169404 New failing tests: editing/style/inline-style-container.html Created attachment 349381 [details]
Archive of layout-test-results from ews202 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 349376 [details] Patch Attachment 349376 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9169327 New failing tests: editing/style/inline-style-container.html Created attachment 349382 [details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349376 [details] Patch Attachment 349376 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9169365 New failing tests: editing/style/inline-style-container.html Created attachment 349384 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 349376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349376&action=review >> Tools/TestWebKitAPI/Tests/mac/FontManagerTests.mm:276 >> + checkFontColorAtStartAndEndWithInputEvents("rgb(255, 0, 0)"); > > Let's also add an assertion to make sure we're inserting a font element instead of a span in this case. Sounds good! (In reply to Build Bot from comment #13) > Created attachment 349384 [details] > Archive of layout-test-results from ews121 for ios-simulator-wk2 > > The attached test failures were seen while running run-webkit-tests on the > ios-sim-ews. > Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4 The test expectation here doesn't seem right to me: > -PASS foreColor(rgba(0, 50, 100, 0.4)) on all of "<font><u style="color:red;">hello</u></font>" yields "<font color="rgba(0, 50, 100, 0.4)"><u>hello</u></font>" > +FAIL foreColor(rgba(0, 50, 100, 0.4)) on all of "<font><u style="color:red;">hello</u></font>" yields "<font><u style="color: rgba(0, 50, 100, 0.4);">hello</u></font>", expected "<font color="rgba(0, 50, 100, 0.4)"><u>hello</u></font>" This test expects <font color="rgba(0, 50, 100, 0.4)"><u>hello</u></font> after changing the foreground color to a color with alpha, but this just results in a bogus text color, since the font element does not support RGBA colors (I'm seeing a fully opaque dark green in Safari 12 when visiting data:text/html,<font%20color="rgba(0,%2050,%20100,%200.4)"><u>hello</u></font>). After this patch, we get <font><u style="color: rgba(0, 50, 100, 0.4);">hello</u></font>, which results in light blue, transparent text (as one would probably expect). I think this can be safely rebaselined. (In reply to Wenson Hsieh from comment #15) > (In reply to Build Bot from comment #13) > > Created attachment 349384 [details] > > Archive of layout-test-results from ews121 for ios-simulator-wk2 > > > > The attached test failures were seen while running run-webkit-tests on the > > ios-sim-ews. > > Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4 > > The test expectation here doesn't seem right to me: > > > -PASS foreColor(rgba(0, 50, 100, 0.4)) on all of "<font><u style="color:red;">hello</u></font>" yields "<font color="rgba(0, 50, 100, 0.4)"><u>hello</u></font>" > > +FAIL foreColor(rgba(0, 50, 100, 0.4)) on all of "<font><u style="color:red;">hello</u></font>" yields "<font><u style="color: rgba(0, 50, 100, 0.4);">hello</u></font>", expected "<font color="rgba(0, 50, 100, 0.4)"><u>hello</u></font>" > > This test expects <font color="rgba(0, 50, 100, 0.4)"><u>hello</u></font> > after changing the foreground color to a color with alpha, but this just > results in a bogus text color, since the font element does not support RGBA > colors (I'm seeing a fully opaque dark green in Safari 12 when visiting > data:text/html,<font%20color="rgba(0,%2050,%20100,%200.4)"><u>hello</u></ > font>). After this patch, we get <font><u style="color: rgba(0, 50, 100, > 0.4);">hello</u></font>, which results in light blue, transparent text (as > one would probably expect). > > I think this can be safely rebaselined. Actually, instead of just rebaselining the test, I think I'll change this failing test case to just apply "rgb(0, 50, 100)" without alpha, and then add "rgba(0, 50, 100, 0.4)" as an additional test case. Created attachment 349397 [details]
Patch for EWS
Created attachment 349401 [details]
Another patch for EWS
> Actually, instead of just rebaselining the test, I think I'll change this
> failing test case to just apply "rgb(0, 50, 100)" without alpha, and then
> add "rgba(0, 50, 100, 0.4)" as an additional test case.
On second thought, maybe it's not a great idea to use inline styles to represent RGBA font colors, when "StyleWithCSS" is set to false...
Another approach is to just drop the alpha component when setting the color attribute of the font element, since we can't represent transparent colors using only attributes on the font element (rather than CSS). Then, for our API test, we can exercise the case where "StyleWithCSS" is true.
I posted a separate patch that implements this approach.
Chatted with Ryosuke about this, and agree that fulfilling user expectation is more important than respecting StyleWithCSS being false, when setting a font color with alpha ≠ 1. We'll go with the original approach. Committed r235914: <https://trac.webkit.org/changeset/235914> Looks like the new API, FontManagerTests.ChangeFontColorWithColorPanel, test is failing on Sierra WK1 from: https://trac.webkit.org/changeset/235914 Log: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/12719/steps/run-api-tests/logs/stdio Run Failure: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/12719 (In reply to Truitt Savell from comment #21) > Looks like the new API, FontManagerTests.ChangeFontColorWithColorPanel, test > is failing on Sierra WK1 from: https://trac.webkit.org/changeset/235914 > > > Log: > https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/ > builds/12719/steps/run-api-tests/logs/stdio > > Run Failure: > https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/ > builds/12719 Thanks — I'll take a look shortly. Reopening to attach new patch. Created attachment 349492 [details]
Patch
Comment on attachment 349492 [details] Patch Clearing flags on attachment: 349492 Committed r235931: <https://trac.webkit.org/changeset/235931> All reviewed patches have been landed. Closing bug. |