Per #2713499-6: When navigating to a new page, it should be scrolled to the top + navigating to a fragment on another page broken in Firefox:

Now that we have manual scrolling in place, and it turns out to be trivial, we might as well do that when navigating to a fragment on the current page too, because that'd allow us to remove our onhashchange event handler. Let's do that in a follow-up.

CommentFileSizeAuthor
#8 2713549-followup-8.patch558 bytesWim Leers
#4 interdiff.txt1.98 KBWim Leers
#4 2713549-4.patch3.91 KBWim Leers
#3 interdiff.txt1.01 KBWim Leers
#3 2713549-3.patch3.28 KBWim Leers
#2 2713549-2.patch2.46 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2713549-2.patch2.46 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2713549-3.patch3.28 KB
interdiff.txt1.01 KB

And now with onhashchange actually removed.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2713549-4.patch3.91 KB
interdiff.txt1.98 KB
  1. Rename the new function to make more sense.
  2. Document the new function.
  3. Move the fragment navigation logic deeper, so we reuse the pre-existing event.preventDefault().
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2713567: Refactor all the controller logic into an actual object: improve understandability/maintainability

This looks great IMO. One less event handler: much simpler!

But the interdiff in #4 made me realize that we can now structure the code much more elegantly. That's out of scope here, so opened #2713567: Refactor all the controller logic into an actual object: improve understandability/maintainability.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Fixed

  • Wim Leers committed a6fdc57 on 8.x-1.x 
    Issue #2713549 by Wim Leers: Refactor onhashchange event handler away:...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Fixed » Needs review
FileSize
2713549-followup-8.patch558 bytes

Discovered at #2713567-4: Refactor all the controller logic into an actual object: improve understandability/maintainability that this broke scroll restoration. Due to the scrolling for fragment navigation (type=intra) happening before pushing state, so the wrong scroll position is recorded by the browser.

Fix attached.

  • Wim Leers committed 7d78e07 on 8.x-1.x 
    Follow-up for issue #2713549 by Wim Leers: Refactor onhashchange event...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.