Add a navigation button to force light or dark appearance (whatever is the opposite of the default appearance).
<rdar://problem/46153172>
Created attachment 355225 [details] Patch
Created attachment 355226 [details] Screenshot With Patch Applied
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 355225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355225&action=review Can you add tests for this? A few basic ones checking that `getDefaultAppearance` is valid and `setForcedAppearance` doesn't throw an error. Is it possible for us to check any styling on the page (maybe even a media query being active) to see if `setForcedAppearance` actually worked? > Source/JavaScriptCore/inspector/protocol/Page.json:213 > + { "name": "result", "$ref": "AppearanceName", "description": "Appearance name that is active when no appearance is forced." } NIT: I think the name `appearance` would be better > Source/JavaScriptCore/inspector/protocol/Page.json:322 > + { "name": "appearance", "$ref": "AppearanceName", "description": "Name of the appearance that is active when no appearance is forced." } NIT: this description doesn't seem accurate > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:695 > + m_frontendDispatcher->defaultAppearanceChanged(useDarkAppearance ? Inspector::Protocol::Page::AppearanceName::Dark : Inspector::Protocol::Page::AppearanceName::Light); This isn't very flexible if other appearances get added, but I think that's very unlikely to happen, so I guess I'm fine with this > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:828 > + if (appearanceName == "Light"_str) NIT: I think just `_s` is enough > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:830 > + else if (appearanceName == "Dark"_str) Ditto (828) > Source/WebCore/page/Page.cpp:2536 > + appearanceDidChange(); Should we only call this if `valueOverride !== m_useDarkAppearanceOverride`? > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:151 > + this.dispatchEventToListeners(WI.CSSManager.Event.ForcedAppearanceDidChange, {appearanceName: this._forcedAppearanceName}); Should this fire inside the callback of `PageAgent.setForcedAppearance`? It seems a bit "premature" to dispatch this event considering you haven't even called into the backend yet. > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:155 > + this.mediaQueryResultChanged(); Ditto (151) > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:158 > + canForceAppearance() NIT: make this a getter? > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:325 > + defaultAppearanceDidChange(protocolName) NIT: please add `// Called from WI.PageObserver.` > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:648 > + Light: Symbol("light"), > + Dark: Symbol("dark"), Personally, I think using a symbol is a bit unnecessary over a plain string (extra work to make it unique), but if you feel strongly about using them then that's fine > Source/WebInspectorUI/UserInterface/Protocol/PageObserver.js:52 > + WI.cssManager.defaultAppearanceDidChange(appearanceName); I'd rather you keep the naming consistent between all of the classes (e.g. `InspectorInstrumentation`, `InspectorPageAgent`, `PageObserver`, and `PageManager` should all use `defaultAppearanceChanged`) so it makes searching/following code much easier > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:77 > + WI.cssManager.addEventListener(WI.CSSManager.Event.DefaultAppearanceDidChange, this._defaultAppearanceDidChange, this); NIT: I prefer to prefix all event listeners with `_handle*` > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:605 > + if (WI.cssManager.forcedAppearanceName && this._forceAppearanceButtonNavigationItem) If the user forces Dark mode, then changes to Dark mode via System Preferences, we'd still have the navigation item enabled? This is a tricky situation. On one hand, if the user has forced an appearance, the item should stay active no matter what (e.g. if they go from light to dark to light, it should still be enabled after the last switch). On the other, if the user switches to dark mode, they have to click the item twice to force enable light mode. Frankly, I'm not sure which is more useful/important/sensible. Could we do something like un-force if the default appearance changes to be the forced appearance, but still save that value so that if they change the default appearance back, it would re-force using the saved value? Additionally, when in the changed appearance mode, clicking the navigation item would override the saved value with the current opposite of the default appearance? > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:627 > + this._forceAppearanceButtonNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, this._toggleAppearance, this); Ditto (77)
(In reply to Timothy Hatcher from comment #3) > Created attachment 355226 [details] > Screenshot With Patch Applied I love this icon 😍 Have you tried looking at it in non-retina? I feel like the three-dots might not be clear (or even visible) 😞
Comment on attachment 355225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355225&action=review >> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:695 >> + m_frontendDispatcher->defaultAppearanceChanged(useDarkAppearance ? Inspector::Protocol::Page::AppearanceName::Dark : Inspector::Protocol::Page::AppearanceName::Light); > > This isn't very flexible if other appearances get added, but I think that's very unlikely to happen, so I guess I'm fine with this Yeah, we would need to cross that bridge when needed. But it likely won't happen soon. >> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:828 >> + if (appearanceName == "Light"_str) > > NIT: I think just `_s` is enough Cool! >> Source/WebCore/page/Page.cpp:2536 >> + appearanceDidChange(); > > Should we only call this if `valueOverride !== m_useDarkAppearanceOverride`? Yeah, added. >> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:151 >> + this.dispatchEventToListeners(WI.CSSManager.Event.ForcedAppearanceDidChange, {appearanceName: this._forcedAppearanceName}); > > Should this fire inside the callback of `PageAgent.setForcedAppearance`? It seems a bit "premature" to dispatch this event considering you haven't even called into the backend yet. PageAgent does not fire an event for forced appearance in this patch, only when the default changes. I moved this code into a then() on PageAgent.setForcedAppearance() promise. >> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:155 >> + this.mediaQueryResultChanged(); > > Ditto (151) Ditto. >> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:158 >> + canForceAppearance() > > NIT: make this a getter? This matches the canForcePseudoClasses() below. Also questions should be functions, not getters. >> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:325 >> + defaultAppearanceDidChange(protocolName) > > NIT: please add `// Called from WI.PageObserver.` Added. >> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:648 >> + Dark: Symbol("dark"), > > Personally, I think using a symbol is a bit unnecessary over a plain string (extra work to make it unique), but if you feel strongly about using them then that's fine Directly using protocol types in the managers has been an issue for us for a while. This forces us to map the enums and have a hard separation between manager and agent. View and model code should not touch agent types. >> Source/WebInspectorUI/UserInterface/Protocol/PageObserver.js:52 >> + WI.cssManager.defaultAppearanceDidChange(appearanceName); > > I'd rather you keep the naming consistent between all of the classes (e.g. `InspectorInstrumentation`, `InspectorPageAgent`, `PageObserver`, and `PageManager` should all use `defaultAppearanceChanged`) so it makes searching/following code much easier Done. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:77 >> + WI.cssManager.addEventListener(WI.CSSManager.Event.DefaultAppearanceDidChange, this._defaultAppearanceDidChange, this); > > NIT: I prefer to prefix all event listeners with `_handle*` Meh, most things are event listeners really. Also code calls this function directly, and even isn't a param. So I left this as-is. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:605 >> + if (WI.cssManager.forcedAppearanceName && this._forceAppearanceButtonNavigationItem) > > If the user forces Dark mode, then changes to Dark mode via System Preferences, we'd still have the navigation item enabled? This is a tricky situation. On one hand, if the user has forced an appearance, the item should stay active no matter what (e.g. if they go from light to dark to light, it should still be enabled after the last switch). On the other, if the user switches to dark mode, they have to click the item twice to force enable light mode. Frankly, I'm not sure which is more useful/important/sensible. > > Could we do something like un-force if the default appearance changes to be the forced appearance, but still save that value so that if they change the default appearance back, it would re-force using the saved value? Additionally, when in the changed appearance mode, clicking the navigation item would override the saved value with the current opposite of the default appearance? I handle it as you stated and I think that is sufficient. Users are not likely going to be toggling both system setting and our button at the same time, so adding more complexity to unforce and reforce is not worth it.
Created attachment 355249 [details] Patch
(In reply to Devin Rousso from comment #5) > Comment on attachment 355225 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=355225&action=review > > Can you add tests for this? A few basic ones checking that > `getDefaultAppearance` is valid and `setForcedAppearance` doesn't throw an > error. Is it possible for us to check any styling on the page (maybe even a > media query being active) to see if `setForcedAppearance` actually worked? I'll try to write a test.
Comment on attachment 355249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355249&action=review This looks nice. All it needs are tests for the new protocol commands + events. > Source/JavaScriptCore/inspector/protocol/Page.json:210 > + "name": "getDefaultAppearance", This also is not used, should we just get rid of it? > Source/JavaScriptCore/inspector/protocol/Page.json:324 > + { > + "name": "forcedAppearanceDidChange", > + "description": "Fired when page's forced appearance changes.", > + "parameters": [ > + { "name": "appearance", "$ref": "Appearance", "description": "Name of the appearance that is forced." } > + ] > + }, This is not added to the PageObserver below. Lets remove it? > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:606 > + if (WI.cssManager.forcedAppearance && this._forceAppearanceButtonNavigationItem) > + return; If I'm on a page with default Light appearance the button is created with "Default Light, Toggle Dark". If I then navigate to a page with a default Dark appearance, the button does not look like it is updated. So are its tooltips incorrect? > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:620 > + console.error("Uknown default appearance name:", defaultAppearance); Typo: "Uknown"
Created attachment 355304 [details] Patch
Added a test.
Comment on attachment 355249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355249&action=review >> Source/JavaScriptCore/inspector/protocol/Page.json:210 >> + "name": "getDefaultAppearance", > > This also is not used, should we just get rid of it? Yep, will remove. >> Source/JavaScriptCore/inspector/protocol/Page.json:324 >> + }, > > This is not added to the PageObserver below. Lets remove it? Yep, will remove. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:606 >> + return; > > If I'm on a page with default Light appearance the button is created with "Default Light, Toggle Dark". > If I then navigate to a page with a default Dark appearance, the button does not look like it is updated. So are its tooltips incorrect? A page's default appearance is the same between navigations. It only changes when the user picks a new appearance in system preferences. A document might not use dark appearance, and still be "light", unless it uses supported-color-schemes and/or the prefers-color-scheme media query. But the page is still default "dark".
Created attachment 355305 [details] Patch
Can we get this reviewed and landed?
Comment on attachment 355305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355305&action=review r=me, super awesome job! It'd be great for WebInspector development if there was a way to do a similar thing for WebInspector within WebInspector, just like how we can force the layout direction. You could do this for dark mode via System Preferences or by inspector^2, but that's a bit more annoying :P > Source/JavaScriptCore/inspector/protocol/Page.json:206 > + { "name": "appearance", "$ref": "Appearance", "description": "Appearance name to force. Empty string disables the override." } Considering that we are using a specified type `Appearance`, perhaps we should add a `Default` to the list of values. If this value was a _real_ enum, an "empty string" would be invalid, so even though that works here because it's just a `String`, it'd be more "correct" (somewhat similar to `WI.LayoutDirection`). You'd also be able to use this in the frontend by passing actual specific values instead of empty strings. > Source/JavaScriptCore/inspector/protocol/Page.json:312 > + "name": "defaultAppearanceDidChange", NIT: I hate it when events have "Did" in the name, as it's almost always unnecessary. I'd prefer this be called `defaultAppearanceChanged`. > Source/JavaScriptCore/inspector/protocol/Page.json:315 > + { "name": "appearance", "$ref": "Appearance", "description": "Name of the appearance that is active (not considering any forced appearance.)" } NIT: the "." should go after the ")" > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:821 > +void InspectorPageAgent::setForcedAppearance(ErrorString&, const String& appearance) See Page.json:206 for using the actual protocol type instead of a `String` > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:151 > + PageAgent.setForcedAppearance(protocolName).then(() => { Should we do any error logging? I'd also prefer a callback if there's no reason to use the promise (e.g. returning it for chaining). > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:159 > + return window.PageAgent && !!PageAgent.setForcedAppearance && this._defaultAppearance; I think the new approach for feature checking is to use `InspectorBackend.domains.Page.setForcedAppearance`. > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:644 > + DefaultAppearanceDidChange: "css-manager-default-appearance-did-change", > + ForcedAppearanceDidChange: "css-manager-forced-appearance-did-change", See Page.json:312 > LayoutTests/inspector/css/force-page-appearance.html:30 > + InspectorTest.expectEqual(getProperty("color").rawValue, "rgb(0, 0, 0)"); These checks could use comments, as otherwise it's a bit confusing to read the expected value and understand what's going on.
Comment on attachment 355305 [details] Patch Clearing flags on attachment: 355305 Committed r238570: <https://trac.webkit.org/changeset/238570>
All reviewed patches have been landed. Closing bug.