-
Notifications
You must be signed in to change notification settings - Fork 3k
window.navigation needs to be [Replaceable] #11786
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.
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
Conversation
This is tested in https://wpt.fyi/results/html/dom/idlharness.https.html%3Finclude%3D(Document%7CWindow)?label=experimental&label=master&aligned subtest Landing the spec change should update the IDL in wpt, so I think we can tick the "tests" checkbox. |
https://bugs.webkit.org/show_bug.cgi?id=297721 rdar://158951219 Reviewed by NOBODY (OOPS!). With the Navigation API enabled, the ESPN title bar is empty instead of containing the tab buttons like it should. The issue is that espn.com re-assigns the "navigation" variable to hold these items in this function: preRender: function(type, navCached, defaultNavData) { ... navigation = defaultNavData.navigation ... espn_ui.Helpers.nav.items = navigation.items ... } But with the Navigation API enabled, the "navigation" variable is reserved for accessing that API and is not replaceable. So this re-assignment fails and the title bar ends up empty. The spec does not say that "navigation" should be replaceable. But this is breaking a live site and Chrome does allow it to be replaceable, so we fix this by making it replaceable in WebKit too. I have confirmed that this does fix the ESPN title bar locally. Associated spec issue: whatwg/html#11786 I've added a new layout test to check this: navigation-api/navigation-object-is-replaceable.html * LayoutTests/navigation-api/navigation-object-is-replaceable-expected.txt: Added. * LayoutTests/navigation-api/navigation-object-is-replaceable.html: Added. * Source/WebCore/page/DOMWindow.idl:
Thanks, I'll merge this then. I'm still curious about the Chromium story. |
https://bugs.webkit.org/show_bug.cgi?id=297721 rdar://158951219 Reviewed by NOBODY (OOPS!). With the Navigation API enabled, the ESPN title bar is empty instead of containing the tab buttons like it should. The issue is that espn.com re-assigns the "navigation" variable to hold these items in this function: preRender: function(type, navCached, defaultNavData) { ... navigation = defaultNavData.navigation ... espn_ui.Helpers.nav.items = navigation.items ... } But with the Navigation API enabled, the "navigation" variable is reserved for accessing that API and is not replaceable. So this re-assignment fails and the title bar ends up empty. The spec does not say that "navigation" should be replaceable. But this is breaking a live site and Chrome does allow it to be replaceable, so we fix this by making it replaceable in WebKit too. I have confirmed that this does fix the ESPN title bar locally. Associated spec issue: whatwg/html#11786 I've added a new layout test to check this: navigation-api/navigation-object-is-replaceable.html * LayoutTests/imported/w3c/web-platform-tests/interfaces/html.idl: * LayoutTests/imported/w3c/web-platform-tests/interfaces/html.idl: * LayoutTests/navigation-api/navigation-object-is-replaceable-expected.txt: Added. * LayoutTests/navigation-api/navigation-object-is-replaceable.html: Added. * Source/WebCore/page/DOMWindow.idl:
https://bugs.webkit.org/show_bug.cgi?id=297721 rdar://158951219 Reviewed by Chris Dumez and Anne van Kesteren. With the Navigation API enabled, the ESPN title bar is empty instead of containing the tab buttons like it should. The issue is that espn.com re-assigns the "navigation" variable to hold these items in this function: preRender: function(type, navCached, defaultNavData) { ... navigation = defaultNavData.navigation ... espn_ui.Helpers.nav.items = navigation.items ... } But with the Navigation API enabled, the "navigation" variable is reserved for accessing that API and is not replaceable. So this re-assignment fails and the title bar ends up empty. The spec does not say that "navigation" should be replaceable. But this is breaking a live site and Chrome does allow it to be replaceable, so we fix this by making it replaceable in WebKit too. I have confirmed that this does fix the ESPN title bar locally. Associated spec issue: whatwg/html#11786 I've added a new layout test to check this: navigation-api/navigation-object-is-replaceable.html * LayoutTests/imported/w3c/web-platform-tests/interfaces/html.idl: * LayoutTests/imported/w3c/web-platform-tests/interfaces/html.idl: * LayoutTests/navigation-api/navigation-object-is-replaceable-expected.txt: Added. * LayoutTests/navigation-api/navigation-object-is-replaceable.html: Added. * Source/WebCore/page/DOMWindow.idl: Canonical link: https://commits.webkit.org/301607@main
@annevk This is the commit that made it
|
Thanks, looks like an oversight during early development. Maybe we should double check IDL going forward, especially for new global members. |
Yea definitely. We will do what we can to minimize this type of oversight, especially around IDLs. |
I know there was some work a decade or so ago diffing Blink's IDL definitions to specs, which would've caught this. Don't know whether that still exists at all? |
See https://bugs.webkit.org/show_bug.cgi?id=297721 for context.
I would love to know why this ended up in Chromium but not the HTML standard. (I couldn't figure out how to blame a deleted file using the web interface.)
Also ideally someone else more familiar with this space would take over for tests and such.
cc @jnjaeschke @natechapin
(See WHATWG Working Mode: Changes for more details.)
/nav-history-apis.html ( diff )