Bug 184353 – Use the system's link color when system appearance is desired for a WebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Timothy Hatcher]

The link color does not match the system, when the system appearance is desired. We should be using [NSColor linkColor].

<rdar://problem/9420053>

[Timothy Hatcher]

Created attachment 337338 [details]
Patch

[Timothy Hatcher]

Created attachment 337346 [details]
Patch

[EWS Watchlist]

Comment on attachment 337346 [details]
Patch

Attachment 337346 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7223433

New failing tests:
css3/selectors3/xml/css3-modsel-61.xml
tables/mozilla/bugs/bug46623-1.html
tables/mozilla/bugs/bug8950.html
css3/selectors3/html/css3-modsel-17.html
fast/selectors/017.html
fast/dom/34176.html
fast/selectors/061.html
css3/selectors3/html/css3-modsel-61.html
css3/selectors3/xhtml/css3-modsel-61.xml
css3/selectors3/xhtml/css3-modsel-17.xml
fast/css/style-tag-display-none.html
tables/mozilla/bugs/bug51140.html
css3/selectors3/xml/css3-modsel-17.xml

[EWS Watchlist]

Created attachment 337347 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Timothy Hatcher]

Created attachment 337380 [details]
Patch

[Megan Gardner]

Comment on attachment 337380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337380&action=review

r=me, with my comments, except that I can't actually review, so you'll need someone else to get a real review from.

> Source/WebCore/css/StyleResolver.cpp:1841
> +            options |= StyleColor::Options::UseSystemAppearance;

The above three lines are in many places. Could you consolidate it into a constructor for StyleColor::Options?

> Source/WebCore/rendering/RenderThemeMac.mm:447
> +            // FIXME: Use a semantic system color for this, instead of systemPurpleColor.

File a radar for this FIXME, also is there a reason you're using systemPurpleColor rather than the one that was in Document?

> Source/WebCore/rendering/RenderThemeMac.mm:463
> +                return useSystemAppearance ? @selector(linkColor) : nullptr;

Why don't we always want to use the link color from NSColor? It seems like we would, for consistency.

> Source/WebCore/rendering/RenderThemeMac.mm:466
> +                // FIXME: Use a semantic system color for this, instead of systemRedColor.

Radar for this FIXME. Also, not sure if we want to do something different for if we are following system appearance or not.

[Wenson Hsieh]

Comment on attachment 337380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337380&action=review

> Source/WebCore/ChangeLog:4
> +

Nit - extra newline here.

> Source/WebCore/ChangeLog:8
> +        Have Document consult RenderTheme via StyleColor for the various link colors.

Nit - the description should go after the Reviewed by line.

> Source/WebCore/PAL/ChangeLog:3
> +        Use the system's link color when system appearance is desired for a WebView.

Could we write an API test for this? I imagine we could do something like:

1. Swizzle out +[NSColor linkColor]
2. Load an HTML string with a link
3. Evaluate JavaScript to query the computed style of the link

There are helpers to make (2) and (3) more convenient if you create a TestWKWebView, and (1) can be accomplished via ClassMethodSwizzler (also see TestWebKitAPI).

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:2609
> +		9418278B1D8B244000492764 /* StyleColor.h in Headers */ = {isa = PBXBuildFile; fileRef = 941827891D8B242200492764 /* StyleColor.h */; settings = {ATTRIBUTES = (Private, ); }; };

I think I might've missed something here, but is there a reason StyleColor.h needs to become private? I don't see StyleColor.h being included anywhere outside of WebCore.

[Timothy Hatcher]

(In reply to Megan Gardner from comment #6)
> Comment on attachment 337380 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337380&action=review
> 
> r=me, with my comments, except that I can't actually review, so you'll need
> someone else to get a real review from.
> 
> > Source/WebCore/css/StyleResolver.cpp:1841
> > +            options |= StyleColor::Options::UseSystemAppearance;
> 
> The above three lines are in many places. Could you consolidate it into a
> constructor for StyleColor::Options?

I agree. I'll see if I can clean this up.

> > Source/WebCore/rendering/RenderThemeMac.mm:447
> > +            // FIXME: Use a semantic system color for this, instead of systemPurpleColor.
> 
> File a radar for this FIXME, also is there a reason you're using
> systemPurpleColor rather than the one that was in Document?

HI said we should use systemPurpleColor. I'll add the Radar for this fixme.

> > Source/WebCore/rendering/RenderThemeMac.mm:463
> > +                return useSystemAppearance ? @selector(linkColor) : nullptr;
> 
> Why don't we always want to use the link color from NSColor? It seems like
> we would, for consistency.

The main reason is that the colors in Document are from the HTML spec, so it is consistent cross-browser to keep the old ones for web content.

> > Source/WebCore/rendering/RenderThemeMac.mm:466
> > +                // FIXME: Use a semantic system color for this, instead of systemRedColor.
> 
> Radar for this FIXME. Also, not sure if we want to do something different
> for if we are following system appearance or not.

HI said we should use systemRedColor, but we will likely do something else for active. I'll add the Radar for this fixme.

[Timothy Hatcher]

(In reply to Wenson Hsieh from comment #7)
> Comment on attachment 337380 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337380&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +
> 
> Nit - extra newline here.
> 
> > Source/WebCore/ChangeLog:8
> > +        Have Document consult RenderTheme via StyleColor for the various link colors.
> 
> Nit - the description should go after the Reviewed by line.
> 
> > Source/WebCore/PAL/ChangeLog:3
> > +        Use the system's link color when system appearance is desired for a WebView.
> 
> Could we write an API test for this? I imagine we could do something like:
> 
> 1. Swizzle out +[NSColor linkColor]
> 2. Load an HTML string with a link
> 3. Evaluate JavaScript to query the computed style of the link
> 
> There are helpers to make (2) and (3) more convenient if you create a
> TestWKWebView, and (1) can be accomplished via ClassMethodSwizzler (also see
> TestWebKitAPI).

Okay. I'll give that a try.

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:2609
> > +		9418278B1D8B244000492764 /* StyleColor.h in Headers */ = {isa = PBXBuildFile; fileRef = 941827891D8B242200492764 /* StyleColor.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> I think I might've missed something here, but is there a reason StyleColor.h
> needs to become private? I don't see StyleColor.h being included anywhere
> outside of WebCore.

StyleColor.h isn't, but RenderTheme.h is, which now includes StyleColor.h. I hit WebKit build errors without this.

[Timothy Hatcher]

Comment on attachment 337380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337380&action=review

>>> Source/WebCore/ChangeLog:4
>>> +
>> 
>> Nit - extra newline here.
> 
> Okay. I'll give that a try.

Not sure why this comment went here.

>> Source/WebCore/PAL/ChangeLog:3
>> +        Use the system's link color when system appearance is desired for a WebView.
> 
> Could we write an API test for this? I imagine we could do something like:
> 
> 1. Swizzle out +[NSColor linkColor]
> 2. Load an HTML string with a link
> 3. Evaluate JavaScript to query the computed style of the link
> 
> There are helpers to make (2) and (3) more convenient if you create a TestWKWebView, and (1) can be accomplished via ClassMethodSwizzler (also see TestWebKitAPI).

Okay. I'll give that a try.

[Timothy Hatcher]

Created attachment 337417 [details]
Patch

[Timothy Hatcher]

(In reply to Wenson Hsieh from comment #7)
> Could we write an API test for this? I imagine we could do something like:
> 
> 1. Swizzle out +[NSColor linkColor]
> 2. Load an HTML string with a link
> 3. Evaluate JavaScript to query the computed style of the link
> 
> There are helpers to make (2) and (3) more convenient if you create a
> TestWKWebView, and (1) can be accomplished via ClassMethodSwizzler (also see
> TestWebKitAPI).

I wrote an API test that checks link color. I couldn't use ClassMethodSwizzler, since that needs to swizzle in the web process, not the UI process. I did test with system appearance on and off. I also couldn't test visited links, since we hide visited colors from JavaScript.

[Timothy Hatcher]

Created attachment 337418 [details]
Patch

[Wenson Hsieh]

Comment on attachment 337418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337418&action=review

> Source/WebCore/ChangeLog:4
> +

Nit - extra new line in these ChangeLog entries.

[WebKit Commit Bot]

Comment on attachment 337418 [details]
Patch

Clearing flags on attachment: 337418

Committed r230368: <https://trac.webkit.org/changeset/230368>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.