Bug 183418 – Allow WebViews to disable system appearance

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Allow WebViews to disable system appearance

[Megan Gardner]

Created attachment 335229 [details]
Patch

[Megan Gardner]

<rdar://problem/36975642>

[Tim Horton]

Comment on attachment 335229 [details]
Patch

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

> Source/WebCore/css/StyleColor.cpp:40
> +Color StyleColor::colorFromKeyword(CSSValueID keyword, bool useSystemAppearance)

Not necessary to do in this patch but it would probably be better to make this an enum class

> Source/WebCore/css/StyleResolver.cpp:1825
> +        return RenderTheme::focusRingColor(document().page()->useSystemAppearance());

Can page be null?

> Source/WebCore/css/StyleResolver.cpp:1832
> +        return StyleColor::colorFromKeyword(identifier, document().page()->useSystemAppearance());

Can page be null?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:87
> +    context->drawFocusRing(path, 1, 1, RenderTheme::focusRingColor(element.document().page()->useSystemAppearance()));

Can Page be null?

> Source/WebCore/platform/mac/ThemeMac.mm:76
> +    WebCore::LocalDefaultSystemAppearance localAppearence(useSystemAppearance);

This local is spelled wrong. There might be others, maybe grep for "appearence"

> Source/WebCore/rendering/RenderTheme.h:153
> +    virtual Color platformFocusRingColor(bool useSystemAppearance) const { UNUSED_PARAM(useSystemAppearance); return Color(0, 0, 0); }

You can just leave the name off the arg instead, then you don't have to UNUSED_PARAM

[Tim Horton]

Comment on attachment 335229 [details]
Patch

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

>> Source/WebCore/css/StyleColor.cpp:40
>> +Color StyleColor::colorFromKeyword(CSSValueID keyword, bool useSystemAppearance)
> 
> Not necessary to do in this patch but it would probably be better to make this an enum class

Something like:

enum class UseSystemAppearance { Yes, No }

and then you'd use it like UseSystemAppearance::Yes.

[Megan Gardner]

Created attachment 335244 [details]
Patch

[EWS Watchlist]

Comment on attachment 335244 [details]
Patch

Attachment 335244 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6847672

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335246 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335244 [details]
Patch

Attachment 335244 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6847809

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335249 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335244 [details]
Patch

Attachment 335244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6847894

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335250 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Megan Gardner]

Created attachment 335251 [details]
Patch

[EWS Watchlist]

Comment on attachment 335251 [details]
Patch

Attachment 335251 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6848895

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335256 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335251 [details]
Patch

Attachment 335251 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6848887

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335257 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335251 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

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

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

[Tim Horton]

Comment on attachment 335251 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasStyle.cpp:-74
> -    return parseColor(colorString);

It’s pretty clear from the crashes that canvas can be null.

> Source/WebCore/html/canvas/CanvasStyle.cpp:74
> +    CSSParserContext context = CSSParserContext(canvas->document());

Not sure it’s a good idea to make a CSSParserContext here that’s missing most of its context -- we might enable features that we don’t want to, etc., because we don’t create it correctly. Right? Or is this all we need to fill one in?

[Megan Gardner]

Created attachment 335263 [details]
Patch

[EWS Watchlist]

Comment on attachment 335263 [details]
Patch

Attachment 335263 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6850889

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335265 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335263 [details]
Patch

Attachment 335263 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6851132

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335269 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335263 [details]
Patch

Attachment 335263 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6851095

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335270 [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

[Megan Gardner]

Created attachment 335311 [details]
Patch

[Megan Gardner]

Created attachment 335315 [details]
Patch

[Megan Gardner]

Created attachment 335317 [details]
Patch

[Megan Gardner]

Created attachment 335322 [details]
Patch

[Megan Gardner]

Created attachment 335325 [details]
Patch

[Megan Gardner]

Created attachment 335331 [details]
Patch

[Megan Gardner]

Created attachment 335339 [details]
Patch

[Tim Horton]

Comment on attachment 335339 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:89
> +    if (element.document().page())

You could probably make this all a lot neater if you put a helper on document that did the null checking for you. Then you'd not need to sprinkle it throughout.

[Megan Gardner]

Created attachment 335365 [details]
Patch

[Tim Horton]

Comment on attachment 335365 [details]
Patch

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

> Source/WebCore/page/Page.h:328
> +#if PLATFORM(MAC)

If you do the same treatment here as you do in Document, you can avoid more #ifs elsewhere.

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:33
> -OBJC_CLASS NSAppearence;
> +@class NSAppearance;

OBJC_CLASS is generally preferred because it works in C++-only files too! There's no reason this header shouldn't be importable in C++ files.

> Source/WebKit/ChangeLog:310
> +>>>>>>> .r229431

Please clean this up

[Dean Jackson]

Comment on attachment 335365 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasStyle.cpp:118
> +CanvasStyle CanvasStyle::createFromString(const String& colorString, HTMLCanvasElement* canvas)

There isn't anything you can do about this, but an OffscreenCanvas won't have a document, and thus won't be able to follow the system appearance.

Although, maybe the system appearance query could be somewhere other than the document/page? Could it be on RenderTheme?

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:33
> -OBJC_CLASS NSAppearence;
> +@class NSAppearance;

Why change from OBJC_CLASS?

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:46
>      RetainPtr<NSAppearance> m_savedSystemAppearance;
> +    bool m_didSetSystemAppearance;

Rather than have the boolean, why not just check the nullness of m_savedSystemAppearance?

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:50
> +    if (m_didSetSystemAppearance)
> +        [NSAppearance setCurrentAppearance:m_savedSystemAppearance.get()];

So this would become

if (m_savedSystemAppearance)
   [NSApp....

[Tim Horton]

Comment on attachment 335365 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:944
> +        setStrokeStyle(CanvasStyle::createFromStringWithOverrideAlpha(color, alpha.value(), &downcast<HTMLCanvasElement>(canvasBase())));

Dino says to leave it for now, but in the future: 
"hand in the canvasBase() itself, and then on canvasBase() add a virtual useSystemApperance() that HTMLCanvasElement implements and grabs from Page"

[Megan Gardner]

Created attachment 335370 [details]
Patch

[Megan Gardner]

Created attachment 335379 [details]
Patch

[Megan Gardner]

Created attachment 335387 [details]
Patch

[Wenson Hsieh]

Comment on attachment 335387 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:6961
> +    if (useSystemAppearance == m_alwaysShowsHorizontalScroller)

I think you meant to compare against m_useSystemAppearance, not m_alwaysShowsHorizontalScroller

[Tim Horton]

Comment on attachment 335387 [details]
Patch

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

> Source/WebCore/page/Page.h:706
> +    bool m_useSystemAppearance {false};

Spaces inside the braces!

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:33
> -OBJC_CLASS NSAppearence;
> +OBJC_CLASS NSAppearance;

How did this even compile before? Maybe you don't need it?

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:36
> +    

This newline is wrong.

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:48
> +        m_savedSystemAppearance = nil;

Why bother? You're in the destructor.

> Source/WebCore/rendering/RenderThemeMac.mm:500
> +    return m_systemColorCache.ensure(cssValueID, [this, cssValueID, useSystemAppearance] () -> Color {

Probably need to (perhaps in a future patch) clear this cache when things change

> Source/WebKitLegacy/mac/WebView/WebView.mm:5195
> +#if PLATFORM(MAC)

Do you still need these PLATFORM(MAC)s?

[Megan Gardner]

Created attachment 335389 [details]
Patch

[Megan Gardner]

https://trac.webkit.org/changeset/229448/webkit

[Alex Christensen]

Comment on attachment 335389 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:442
> +@property (nonatomic, readwrite, setter=_setUseSystemAppearance:) BOOL _useSystemAppearance WK_API_AVAILABLE(macosx(WK_MAC_TBA));

Don't we need corresponding C API in WKPage.cpp?

[Tim Horton]

(In reply to Alex Christensen from comment #46)
> Comment on attachment 335389 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335389&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:442
> > +@property (nonatomic, readwrite, setter=_setUseSystemAppearance:) BOOL _useSystemAppearance WK_API_AVAILABLE(macosx(WK_MAC_TBA));
> 
> Don't we need corresponding C API in WKPage.cpp?

Hopefully not.