1733797 - Location's hash setter might need to no-op when set to the same value, null, or empty string fragments

Status: RESOLVED FIXED

(Core :: DOM: Window and Location, defect, P3)

Description

[Igor R.]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:92.0) Gecko/20100101 Firefox/92.0

Steps to reproduce:

If I open this link:
https://lithic.com/legal#terms
or
https://lithic.com/legal
scrolling of the page does not work.

Actual results:

There is a scroll bar, but it doesn't not move.

Opening a link with any other #portion, e.g. https://lithic.com/legal#privacy-policy
(or clicking on such a link at the top left portion of the page) clears the scrolling problem.
However if after that, I scroll all the way up, the scrolling is again stuck.

Expected results:

I should be able to scroll the page even if it has some defect in its script, especially since it works in other browsers.

Comment 1

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::Panning and Zooming' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Botond Ballo [:botond]]

The page has a scroll event handler which repeatedly sets window.location.hash to the anchor of the section that's currently in view (so, for example, if https://lithic.com/legal#terms is loaded and you scroll down, the terms section is still in view so it performs window.location.hash = "#terms".

It looks like, in Firefox, assigning to window.location.hash a value which matches the current anchor, causes the anchor location to be scrolled into view (i.e. we scroll back to the top of the section), whereas in Chrome it does not.

I'm not sure which behaviour is correct. Moving to DOM: Navigation for further comment.

Comment 3

[Anne (:annevk)]

Thanks Botond for debugging!

Per the setter description at https://html.spec.whatwg.org/#dom-location-hash Firefox is correct. However, given that Chrome and Safari appear to have an early return, the simplest way forward would probably be changing Firefox and the standard.

Anny, is this something that would interest you?

Comment 4

[Anne (:annevk)]

We should probably investigate if this applies to other setters as well. And maybe also how this works with <a> and such for good measure.

Comment 5

[Anny Gakhokidze [:annyG]]

Not sure if I have time in the near future to work on this, but I'll add it to my list of bugs I could work on if im looking for more tasks.

Comment 6

[Chris Peterson [:cpeterson]]

@ Anny, do you mind filing the spec issue?

Nika says this bug will be easy to fix, but we should get the ball rolling on the spec change. We'll also want to write a WPT for the new scroll behavior.

If this bug won't get fixed soon, we can file a webcompat.com issue and ask the Webcompat team to reach out to lithic.com's web developers.

Nika shared this link to Chrome's early return when the new window.location.hash value which matches the current anchor:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/location.cc;l=203-204;drc=984c3018ecb2ff818e900fdb7c743fc00caf7efe

Comment 7

[Chris Peterson [:cpeterson]]

Clearing needinfo because Anny is working on some more urgent bugs.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: Bug 1733797 - Add new tests documenting Firefox's current behavior when setting location.hash to the current hash. r?smaug

These tests will be updated in the next changeset when Firefox is fixed to match the behavior of Chrome and Safari.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: Bug 1733797 - If the new location.hash is the same as the current hash, don't navigate to the anchor. r?smaug

Depends on D132545

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: Bug 1733797 - Remove unnecessary char16_t casts from Location::GetHash(). r?smaug

hash is a UTF-8 string, so we don't need to cast chars to char16_t.

Depends on D132546

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: Bug 1733797 - Remove unused `unicodeRef` variable in Location::GetHash(). r?smaug

Depends on D132547

Comment 12

[Chris Peterson [:cpeterson]]

I filed an HTML spec issue asking what should happen when setting location.hash = location.hash:

https://github.com/whatwg/html/issues/7386

Comment 13

[Anne (:annevk)]

Thanks Chris!

Comment 14

[Shannon Booth]

There is another merge request open in the HTML spec in this area, see: https://github.com/whatwg/html/pull/11245 and the impacted WPT test https://github.com/web-platform-tests/wpt/pull/52085

Comment 15

[Chris Peterson [:cpeterson]]

Shannon, thanks for the heads up! I'll expand this bug to include null and empty string fragments.

That said, I don't know when I will have a chance to address my code review feedback and handle the null and empty string fragments. If expedient, I could spin out those cases into separate bugs.

Comment 16

[Chris Peterson [:cpeterson]]

Shannon shared more details on GitHub:

The difference with Firefox is if you navigate to a URL without a fragment and then set it to the empty string. As an example:

1. Load https://en.wikipedia.org/wiki/HTML (without a # fragment).
2. Set location.hash = "" in dev tools console.'
3. The URL in Firefox changes to https://en.wikipedia.org/wiki/HTML# instead of what Chrome and WebKit do of skipping the navigation entirely, and leaving the URL as: https://en.wikipedia.org/wiki/HTML

The behavior is tested by this WPT test: https://wpt.live/html/browsers/history/the-location-interface/location_hash_set_empty_string.html

Comment 17

[Edgar Chen [:edgar]]

Hi Chris, we recently identified a web compatibility issue caused by differences in how browsers handle setting location.hash to the same value, see https://bugzilla.mozilla.org/show_bug.cgi?id=1957657#c9.
Would it be possible to address the behavior of setting the same hash value first? Perhaps we could split the remaining cases into separate follow-up bugs? Thanks!

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Edgar Chen [:edgar] from comment #17)

Would it be possible to address the behavior of setting the same hash value first? Perhaps we could split the remaining cases into separate follow-up bugs?

Hi Edgar, unfortunately, I don't know when I might have time to resume working on this bug. (I was only working on this bug in my spare time and I'll be going on PTO for the next few weeks.)

I attached a WIP patch four years ago. I didn't land the patch then, IIRC, smaug had asked me to change how the URL and hash are normalized before testing whether the new hash is the same as the current hash. But I just looked at the Phabricator reviews and I don't see such a comment from smaug. So I don't recall what the issue was at the time.

We should probably to rebase the WIP patches and test on try again.

Comment 19

[Edgar Chen [:edgar]]

Thanks for your work and the update!

Comment 20

[Chris Peterson [:cpeterson]]

Edgar, I have a rebased WIP patch I can attach, if you're going to pick up this bug: https://phabricator.services.mozilla.com/D262061

The patch still needs to address smaug's "compare fragments post-canonicalization" feedback:

https://phabricator.services.mozilla.com/D132546#inline-730945

I think that's necessary to handle URL-encoded fragments such as:

location.hash = "#te<st";
console.assert(location.hash === "#te%3Cst");

in this test:

https://searchfox.org/firefox-main/rev/60308bc3792ef201b82377682de068a5a1c72575/testing/web-platform/tests/html/browsers/history/the-location-interface/same-hash.html#14,36,56

Comment 21

[Chris Peterson [:cpeterson]]

Firefox currently fails that WPT:

https://searchfox.org/firefox-main/rev/60308bc3792ef201b82377682de068a5a1c72575/testing/web-platform/meta/html/browsers/history/the-location-interface/same-hash.html.ini#5,8,11,14

In that case, perhaps landing my WIP patch to fix the webcompat issue for non-URL-encoded fragments won't make make things worse? And then URL-encoded fragments can be fixed in a follow-up bug.

Comment 22

[Chris Peterson [:cpeterson]]

Attachment: WIP: Bug 1733797 - If the new location.hash is the same as the current hash, don't navigate to the anchor.

Comment 23

[Edgar Chen [:edgar]]

Thank you for the rebased patch! The site reported in bug 1957657 has changed and no longer hit this bug, so I think we could try to address the URL-encoded fragment here.

Comment 24

[Chris Peterson [:cpeterson]]

Attachment: Bug 1733797 - URL-encode url.hash for URIs like about:blank. r?#dom-core

Chrome and Safari URL-encode about:blank's location.hash, but Firefox
doesn't. For example, on an about:blank page, in the devtools console, set
location.hash = "#foo<bar" and then check location.hash.

This difference caused Firefox to fail the page scrolling test cases for
i.contentWindow.location.hash = "te<st" in WPT testing/web-platform/tests/html/browsers/history/the-location-interface/same-hash.html.

Comment 25

[Chris Peterson [:cpeterson]]

Attachment: Bug 1733797 - If new location.hash is the same as the current hash, don't navigate to the anchor. r?#dom-core

By not adding "#" to location.href for location.hash = "", Firefox now matches Chrome and Safari's behavior.

Comment 26

[Chris Peterson [:cpeterson]]

These new patches:

1. Don't navigate on location.hash = location.hash (including when setting to null or empty string as described in comment 16). This change fixes WPT /html/browsers/history/the-location-interface/same-hash.html.
2. URL-encode about:blank's location.hash (now matching Chrome's and Safari's behavior). This incidental change was needed to fix the encoding of URI about:blank#te<st to pass WPT /html/browsers/history/the-location-interface/same-hash.html.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/59ed8d9893a0
https://hg.mozilla.org/integration/autoland/rev/96859f6a3cd3
URL-encode url.hash for URIs like about:blank. r=necko-reviewers,farre,valentin
https://github.com/mozilla-firefox/firefox/commit/9b1b34dba9de
https://hg.mozilla.org/integration/autoland/rev/f5a172a5da82
If new location.hash is the same as the current hash, don't navigate to the anchor. r=urlbar-reviewers,farre

Comment 28

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/96859f6a3cd3
https://hg.mozilla.org/mozilla-central/rev/f5a172a5da82

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/58349 for changes under testing/web-platform/tests

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 31

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:cpeterson, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox149 to wontfix.

For more information, please visit BugBot documentation.

Comment 32

[Chris Peterson [:cpeterson]]

No need to uplift. We should let this fix ride the trains and watch for any new webcompat issues.