whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.03k stars 2.63k forks source link

The example doing "history.scrollRestoration = 'manual';" hints that it is enough to give manual scrolling behavior for all the history entries #404

Open smaug---- opened 8 years ago

smaug---- commented 8 years ago

... but in case of fragment navigation that doesn't seem to be the case. Fragment navigation always scrolls to the #id, assuming I'm not missing anything in the spec.

@majido

majido commented 8 years ago

In normal case, if we are traversing to a entry that has persisted user state (e.g, scroll position), restoring that takes precedent over fragment navigation. See history traversal:

  1. If the specified entry is not an entry with persisted user state, but its URL has a fragment identifier, scroll to the fragment identifier.

I think entries with manual scroll restoration mode should behave similarly i.e., not restoring scroll position takes precedent over scrolling to fragment.

If I am not mistaken, on first time page load there is never any persisted user state which means that we always scroll to fragments rather that restore scroll position. On first page loads the scroll restoration mode value is the default value auto which should behave the same.

smaug---- commented 8 years ago

Note, Chrome scrolls to #fragment even with 'manual'. But ok, I prefer too that 'manual' takes precedence over #fragment. Which means we need to clarify what happens in the spec and then the example is fine.

majido commented 8 years ago

Note, Chrome scrolls to #fragment even with 'manual'. But ok, I prefer too that 'manual' takes precedence over #fragment.

If that happens on history traversals then it is a bug but that is the expected behaviour on first page load because fragment scrolling occurs before any script has a chance to execute so the scrollRestoration is going to be auto at that point which should not interfere with fragment scrolling.

smaug---- commented 8 years ago

It happens with history traversals.

majido commented 8 years ago

Which means we need to clarify what happens in the spec and then the example is fine.

I think we should rewrite step 8 to gated on entry not having persisted user state and having scroll restoration mode auto.

As you pointed out scrolling to fragment also happens in step 6 of "update the session history with the new page". We may need to update that to respect the entry's scrolling restoration mode too.

In this case, the step 6 executes inside a loop (Fragment Identifier Loop) which also allows UA to spin its event loop. So in theory it is possible for script to run and change scroll restoration mode of the current entry! I think if entry's scroll restoration mode is 'manual' then we should simply not enter the Fragment Identifier Loop. That gives the most reliable behaviour from developer's perspective.

foolip commented 8 years ago

@majido, are you interested in patching the spec? It also sounds like there's a Blink bug on history traversal, were you able to confirm that?

majido commented 8 years ago

Oh, this fell off my radar. Is it possible to assign the issue to me? In any case, I will send a pull-request for the spec update as suggested in my previous comment.

I am not able to reproduce the bug that @smaug---- has reported in Chrome M48. Here is the step I take:

  1. Navigate to: http://output.jsbin.com/zasipe#validanchor (first load scrolls to fragment)
  2. Open devtools and execute 'history.scrollRestoration = "manual"'
  3. Go to any page (including about:blank)
  4. Click back.
  5. The page does not scroll to anchor as history.scrollRestoration is manual. This is the expected correct behaviour.

Also, here is the particular test that verifies this in Blink:

smaug---- commented 8 years ago

In Chrome (1) Load http://mxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp (2) execute: history.scrollRestoration = "manual" (3) scroll down a bit and click on the left side '1000' (4) scroll up and click on '100' (5) go back. Scrolling is restored to ~'1000'

majido commented 8 years ago

Thanks for the repro steps. Filed a blink bug to track it.

domenic commented 8 years ago

@majido what is the conclusion here? Is there still work to do on the spec, or was it just a Blink bug that is now fixed?