Bug 189382 – [macOS] [WK2] Support changing foreground colors via color panel

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

Currently, attempting to choose a font color using the color picker (accessible through NSFontPanel) does nothing in WKWebView. However, this works in legacy WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/44227311>

[Wenson Hsieh]

Created attachment 349376 [details]
Patch

[Ryosuke Niwa]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

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!

[Wenson Hsieh]

(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.

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Created attachment 349397 [details]
Patch for EWS

[Wenson Hsieh]

Created attachment 349401 [details]
Another patch for EWS

[Wenson Hsieh]

> 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.

[Wenson Hsieh]

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>

[Truitt Savell]

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

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 349492 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349492 [details]
Patch

Clearing flags on attachment: 349492

Committed r235931: <https://trac.webkit.org/changeset/235931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.