Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Web Inspector: REGRESSION(257744@main): Dark Mode simulation isn't maintained on page change, but remains selected in the UI #13630

Merged
merged 1 commit into from May 17, 2023
Merged

Web Inspector: REGRESSION(257744@main): Dark Mode simulation isn't maintained on page change, but remains selected in the UI #13630

merged 1 commit into from May 17, 2023

Conversation

dcrousso
Copy link
Member

@dcrousso dcrousso commented May 9, 2023 •
edited by webkit-early-warning-system

1fb980b

Web Inspector: REGRESSION(257744@main): Dark Mode simulation isn't maintained on page change, but remains selected in the UI
https://bugs.webkit.org/show_bug.cgi?id=254457
<rdar://problem/107506257>

Reviewed by Patrick Angle.

Having to force dark (or light) mode with each refresh is pretty tedious, as it's probably pretty common that developers will refresh as they make changes to the backend source.

* Source/WebCore/page/Page.cpp:
(WebCore::Page::didCommitLoad):
Don't clear the forced appearance override on main frame navigation.

* Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:
(WI.DOMTreeContentView):
(WI.DOMTreeContentView.prototype._defaultUserPreferencesDidChange):
A new `WI.DOMTreeContentView` is created each time the main frame navigates, so make sure to initialize `activated` (and `enabled`) instead of waiting for `WI.CSSManager.Event.OverriddenUserPreferencesDidChange` (and `WI.CSSManager.Event.DefaultUserPreferencesDidChange`).

* Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:
(WI.CSSManager.prototype._mainResourceDidChange):
Add compatibility behavior for releases before this change.

Canonical link: https://commits.webkit.org/264172@main

10ab51b

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🛠 gtk
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
  🧪 api-ios   🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@dcrousso dcrousso self-assigned this May 9, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 3e5b25e)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@dcrousso dcrousso added the Web Inspector Bugs related to the WebKit Web Inspector. label May 9, 2023
@dcrousso dcrousso requested a review from xeenon May 9, 2023 07:09
rcaliman-apple
rcaliman-apple approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@rcaliman-apple rcaliman-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks for fixing this. Looks good.

Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js
Comment on lines +655 to +656
this._overriddenUserPreferences.delete(InspectorBackend.Enum.Page.UserPreferenceName.PrefersColorScheme);
this.dispatchEventToListeners(WI.CSSManager.Event.OverriddenUserPreferencesDidChange);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's trying to keep the UI sync with the cleared override for older backends that don't have this fix. As in: not fixing the bug, but having a truthful UI. Am I reading this right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct! this will ensure that the prefers-color-scheme override doesn't persist across toplevel navigations for older backends

Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js Outdated
@@ -709,6 +710,7 @@ WI.DOMTreeContentView = class DOMTreeContentView extends WI.ContentView

_defaultUserPreferencesDidChange()
{
// COMPATIBILITY (macOS 13, iOS 16.0): `Page.setForcedAppearance()` was removed in favor of `Page.overrideUserPreference()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: This compatibility message is already included in the getters WI.cssManager.supportsOverrideUserPreference and WI.cssManager.supportsOverrideColorScheme. Could be dropped here.

@rcaliman-apple
Copy link
Contributor

This looks good to me, but I'm not a reviewer yet.
For this to land, it still needs official approval from a reviewer like @patrickangle or @xeenon

patrickangle
patrickangle approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@patrickangle patrickangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with Razvan, LGTM.

@dcrousso dcrousso added the merge-queue Applied to send a pull request to merge-queue label May 17, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 10ab51b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🛠 gtk
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
  🧪 api-ios   🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@dcrousso
Web Inspector: REGRESSION(257744@main): Dark Mode simulation isn't ma…
1fb980b 
…intained on page change, but remains selected in the UI

https://bugs.webkit.org/show_bug.cgi?id=254457
<rdar://problem/107506257>

Reviewed by Patrick Angle.

Having to force dark (or light) mode with each refresh is pretty tedious, as it's probably pretty common that developers will refresh as they make changes to the backend source.

* Source/WebCore/page/Page.cpp:
(WebCore::Page::didCommitLoad):
Don't clear the forced appearance override on main frame navigation.

* Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:
(WI.DOMTreeContentView):
(WI.DOMTreeContentView.prototype._defaultUserPreferencesDidChange):
A new `WI.DOMTreeContentView` is created each time the main frame navigates, so make sure to initialize `activated` (and `enabled`) instead of waiting for `WI.CSSManager.Event.OverriddenUserPreferencesDidChange` (and `WI.CSSManager.Event.DefaultUserPreferencesDidChange`).

* Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:
(WI.CSSManager.prototype._mainResourceDidChange):
Add compatibility behavior for releases before this change.

Canonical link: https://commits.webkit.org/264172@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264172@main (1fb980b): https://commits.webkit.org/264172@main

Reviewed commits have been landed. Closing PR #13630 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 17, 2023
@webkit-commit-queue webkit-commit-queue merged commit 1fb980b into WebKit:main May 17, 2023
@dcrousso dcrousso deleted the eng/254457 branch May 17, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickangle patrickangle patrickangle approved these changes

@rcaliman-apple rcaliman-apple rcaliman-apple approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@xeenon xeenon Awaiting requested review from xeenon

Assignees

@dcrousso dcrousso

Labels
Web Inspector Bugs related to the WebKit Web Inspector.
Projects
None yet
Milestone
No milestone
5 participants
@dcrousso @webkit-early-warning-system @rcaliman-apple @webkit-commit-queue @patrickangle