Accept options in history APIs to allow scroll restoration to be disabled

Accept options in history APIs to allow scroll restoration to be disabled

The option is a dictionary with a single boolean flag for controlling scroll
restoration. It is accepted by both history.[push|replace]State and exposed
via history.options as readonly. There is a new boolean field added to
|HistoryItem| which identifies whether scroll (and scale) should be restored
the entry is navigated to.

This is all hidden behind an experimental runtime enabled feature. 

Intent to implement: https://groups.google.com/a/chromium.org/d/topic/blink-dev/U1e2lmGs4tM/discussion

Chromium side CL: https://crrev.com/959783003

BUG=444094
TEST=LayoutTests/fast/loader/scroll-position-restoration-for-history-api.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195279

[majidvp, 2015-02-26 17:42:10]

majidvp@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2015-02-26 18:04:02]

Aren't the tests you removed still relevant?

I think they need to be rewritten to do actual navigation, and not use
pushState.

https://codereview.chromium.org/927213004/diff/20001/Source/core/loader/Histo...
File Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/927213004/diff/20001/Source/core/loader/Histo...
Source/core/loader/HistoryItem.h:141: // True if history item was created via JS
History API
Add period at EOL.

[majidvp, 2015-02-26 18:31:14]

On 2015/02/26 18:04:02, tdresser wrote:
> Aren't the tests you removed still relevant?
> 
> I think they need to be rewritten to do actual navigation, and not use
> pushState.

The actual navigation is covered by these tests: 
fast/loader/scroll-position-restored-on-back-at-load-event.html
fast/loader/scroll-position-restored-on-reload-at-load-event.html
http/tests/navigation/scrollstate-after-location-reload.html
http/tests/navigation/scrollstate-after-http-equiv-refresh-fragment-identifier.html
 
The removed tests (which are identical btw!) only covered pushState case
so removing them should not be an issue.

>
https://codereview.chromium.org/927213004/diff/20001/Source/core/loader/Histo...
> File Source/core/loader/HistoryItem.h (right):
> 
>
https://codereview.chromium.org/927213004/diff/20001/Source/core/loader/Histo...
> Source/core/loader/HistoryItem.h:141: // True if history item was created via
JS
> History API
> Add period at EOL.

[tdresser, 2015-02-26 18:33:51]

Then LGTM!

[majidvp, 2015-03-02 14:36:00]

majidvp@chromium.org changed reviewers:
+ japhet@chromium.org

[majidvp, 2015-03-02 14:36:01]

+ japhet@: for OWNER's review.

[Nate Chapin, 2015-03-02 18:45:01]

On 2015/03/02 14:36:01, majidvp wrote:
> + japhet@: for OWNER's review.

Mechanically, this is fine.

What do the other browsers do?

[majidvp, 2015-03-02 23:12:03]

On 2015/03/02 18:45:01, Nate Chapin wrote:
> On 2015/03/02 14:36:01, majidvp wrote:
> > + japhet@: for OWNER's review.
> 
> Mechanically, this is fine.
> 
> What do the other browsers do?

Firefox and Safari both restore the scroll position but as far as I understand
this is 
not what most developers who use [replace|push]State APIs (i.e. single-page
applications)
expect or want. The HTML5 history spec leaves it up to the user-agents to decide
which states
to persist which includes scroll position. See: 
http://www.w3.org/TR/2011/WD-html5-20110113/history.html#the-session-history-...

Ideally it should be possible for developers to indicates if they want the
browser to persist scroll/scale for them
but that requires the spec to be updated which will take time. As I understand
this is a short term fix
until such time that the spec is updated.

Once disabled, the existing behaviour can be implemented in JS without too much
effort.
I also left a more detailed comment under the original bug as well.

[Nate Chapin, 2015-03-05 18:56:52]

On 2015/03/02 23:12:03, majidvp wrote:
> On 2015/03/02 18:45:01, Nate Chapin wrote:
> > On 2015/03/02 14:36:01, majidvp wrote:
> > > + japhet@: for OWNER's review.
> > 
> > Mechanically, this is fine.
> > 
> > What do the other browsers do?
> 
> Firefox and Safari both restore the scroll position but as far as I understand
> this is 
> not what most developers who use [replace|push]State APIs (i.e. single-page
> applications)
> expect or want. The HTML5 history spec leaves it up to the user-agents to
decide
> which states
> to persist which includes scroll position. See: 
>
http://www.w3.org/TR/2011/WD-html5-20110113/history.html#the-session-history-...
> 
> Ideally it should be possible for developers to indicates if they want the
> browser to persist scroll/scale for them
> but that requires the spec to be updated which will take time. As I understand
> this is a short term fix
> until such time that the spec is updated.
> 
> Once disabled, the existing behaviour can be implemented in JS without too
much
> effort.
> I also left a more detailed comment under the original bug as well.

Given that this change would make us the odd one out and therefore the risk of
compat issues, I'm not comfortable approving this as-is.

If you want to land behind a flag for experimentation, we could discuss that.

Regardless, you probably want to get feedback from the blink API owners sooner
rather than later.

[majidvp, 2015-03-05 22:51:42]

On 2015/03/05 18:56:52, Nate Chapin wrote:
> On 2015/03/02 23:12:03, majidvp wrote:
> > On 2015/03/02 18:45:01, Nate Chapin wrote:
> > > On 2015/03/02 14:36:01, majidvp wrote:
> > > > + japhet@: for OWNER's review.
> > > 
> > > Mechanically, this is fine.
> > > 
> > > What do the other browsers do?
> > 
> > Firefox and Safari both restore the scroll position but as far as I
understand
> > this is 
> > not what most developers who use [replace|push]State APIs (i.e. single-page
> > applications)
> > expect or want. The HTML5 history spec leaves it up to the user-agents to
> decide
> > which states
> > to persist which includes scroll position. See: 
> >
>
http://www.w3.org/TR/2011/WD-html5-20110113/history.html#the-session-history-...
> > 
> > Ideally it should be possible for developers to indicates if they want the
> > browser to persist scroll/scale for them
> > but that requires the spec to be updated which will take time. As I
understand
> > this is a short term fix
> > until such time that the spec is updated.
> > 
> > Once disabled, the existing behaviour can be implemented in JS without too
> much
> > effort.
> > I also left a more detailed comment under the original bug as well.
> 
> Given that this change would make us the odd one out and therefore the risk of
> compat issues, I'm not comfortable approving this as-is.
> 
> If you want to land behind a flag for experimentation, we could discuss that.
> 
> Regardless, you probably want to get feedback from the blink API owners sooner
> rather than later.

Hi Nate, 
I agree that there are some compat issues. I am planning to get feedback
from API owners but need to gather some initial data points to be able to help
assess benefits 
vs. potential compat issues.  After discussing with Rick, I believe that we may
decide to 
implement this behind an experimental flag at some point to help with obtaining 
data
and getting feedback. I will update you as soon as I have a better idea of which
path we will be
taking.

[majidvp, 2015-05-05 18:45:09]

On 2015/03/05 22:51:42, majidvp wrote:
> On 2015/03/05 18:56:52, Nate Chapin wrote:
> > On 2015/03/02 23:12:03, majidvp wrote:
> > > On 2015/03/02 18:45:01, Nate Chapin wrote:
> > > > On 2015/03/02 14:36:01, majidvp wrote:
> > > > > + japhet@: for OWNER's review.
> > > > 
> > > > Mechanically, this is fine.
> > > > 
> > > > What do the other browsers do?
> > > 
> > > Firefox and Safari both restore the scroll position but as far as I
> understand
> > > this is 
> > > not what most developers who use [replace|push]State APIs (i.e.
single-page
> > > applications)
> > > expect or want. The HTML5 history spec leaves it up to the user-agents to
> > decide
> > > which states
> > > to persist which includes scroll position. See: 
> > >
> >
>
http://www.w3.org/TR/2011/WD-html5-20110113/history.html#the-session-history-...
> > > 
> > > Ideally it should be possible for developers to indicates if they want the
> > > browser to persist scroll/scale for them
> > > but that requires the spec to be updated which will take time. As I
> understand
> > > this is a short term fix
> > > until such time that the spec is updated.
> > > 
> > > Once disabled, the existing behaviour can be implemented in JS without too
> > much
> > > effort.
> > > I also left a more detailed comment under the original bug as well.
> > 
> > Given that this change would make us the odd one out and therefore the risk
of
> > compat issues, I'm not comfortable approving this as-is.
> > 
> > If you want to land behind a flag for experimentation, we could discuss
that.
> > 
> > Regardless, you probably want to get feedback from the blink API owners
sooner
> > rather than later.
> 
> Hi Nate, 
> I agree that there are some compat issues. I am planning to get feedback
> from API owners but need to gather some initial data points to be able to help
> assess benefits 
> vs. potential compat issues.  After discussing with Rick, I believe that we
may
> decide to 
> implement this behind an experimental flag at some point to help with
obtaining 
> data
> and getting feedback. I will update you as soon as I have a better idea of
which
> path we will be
> taking.

Nate: PTAL. I have updated the patch to allow scroll restoration to
become user-defined following our recent intent-to-implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/U1e2lmGs4tM

It is enabled behind an experimental runtime enabled feature flag which
will be removed one the API change is adopted by the spec.

[Nate Chapin, 2015-05-07 20:26:59]

Please link to the intent to implement thread in the patch description.

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
Source/core/frame/History.cpp:87:
options.setWillRestoreScroll(!historyItem->shouldRestoreScroll());
The name of this function implies it's a getter, but it's setting state?

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
Source/core/frame/History.cpp:156: shouldRestoreScroll =
!options.willRestoreScroll();
This inversion doesn't make sense, given the names.

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/History.h
File Source/core/frame/History.h (right):

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
Source/core/frame/History.h:1: /*
This seems accidental

https://codereview.chromium.org/927213004/diff/90001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/927213004/diff/90001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.h:171: void updateForSameDocumentNavigation(const
KURL&, SameDocumentNavigationSource, PassRefPtr<SerializedScriptValue>, bool
shouldRestoreScroll, FrameLoadType);
This would ideally be an enum.

[majidvp, 2015-05-08 21:00:47]

Nate: PTAL.

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
Source/core/frame/History.cpp:87:
options.setWillRestoreScroll(!historyItem->shouldRestoreScroll());
On 2015/05/07 20:26:59, Nate Chapin wrote:
> The name of this function implies it's a getter, but it's setting state?

Done.

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
Source/core/frame/History.cpp:156: shouldRestoreScroll =
!options.willRestoreScroll();
On 2015/05/07 20:26:59, Nate Chapin wrote:
> This inversion doesn't make sense, given the names.

Done.

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/History.h
File Source/core/frame/History.h (right):

https://codereview.chromium.org/927213004/diff/90001/Source/core/frame/Histor...
Source/core/frame/History.h:1: /*
On 2015/05/07 20:26:59, Nate Chapin wrote:
> This seems accidental

Done.

[Nate Chapin, 2015-05-08 23:19:54]

Seems reasonable, just one nitpick. I'm inclined to wait a couple days before
this gets committed if that's ok, just to give folks on the whatwg thread time
to respond to the enum idea.

https://codereview.chromium.org/927213004/diff/110001/public/web/WebHistoryIt...
File public/web/WebHistoryItem.h (right):

https://codereview.chromium.org/927213004/diff/110001/public/web/WebHistoryIt...
public/web/WebHistoryItem.h:53: enum WebHistoryScrollRestorationType {
This and the corresponding type in FrameLoaderTypes.h should be added to
Source/web/AssertMatchingEnums.cpp

[majidvp, 2015-05-11 20:29:43]

On 2015/05/08 23:19:54, Nate Chapin wrote:
> Seems reasonable, just one nitpick. I'm inclined to wait a couple days before
> this gets committed if that's ok, just to give folks on the whatwg thread time
> to respond to the enum idea.

Sure.

>
https://codereview.chromium.org/927213004/diff/110001/public/web/WebHistoryIt...
> File public/web/WebHistoryItem.h (right):
> 
>
https://codereview.chromium.org/927213004/diff/110001/public/web/WebHistoryIt...
> public/web/WebHistoryItem.h:53: enum WebHistoryScrollRestorationType {
> This and the corresponding type in FrameLoaderTypes.h should be added to
> Source/web/AssertMatchingEnums.cpp

Done. Also updated few layout tests to correctly account for |history.options|
and its default value.

[majidvp, 2015-05-11 20:31:14]

majidvp@chromium.org changed reviewers:
+ vollick@chromium.org

[majidvp, 2015-05-11 20:31:15]

+vollick@: For Source/platforms/RuntimeEnabledFeatures.in

[Ian Vollick, 2015-05-11 20:58:13]

On 2015/05/11 at 20:31:15, majidvp wrote:
> +vollick@: For Source/platforms/RuntimeEnabledFeatures.in

Source/platform lgtm

[Nate Chapin, 2015-05-12 17:58:18]

I don't think we need to wait any longer for whatwg, given that this is
experimental behind a flag. LGTM

[majidvp, 2015-05-12 23:32:14]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2015-05-12 23:32:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, vollick@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/927213004/#ps210001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-12 23:32:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927213004/210001

[commit-bot: I haz the power, 2015-05-12 23:36:08]

Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195279