Bug 107026 – Hide the location bar on window.scrollTo(0,[01])

• Status: CLOSED
• Resolution: WONTFIX
• Component: New Bugs

[John Knottenbelt]

Allow Javascript-initiated scrolls to be detected.

[John Knottenbelt]

Created attachment 182997 [details]
Patch

[John Knottenbelt]

This change is needed by https://bugs.webkit.org/show_bug.cgi?id=107027

[Alexey Proskuryakov]

Comment on attachment 182997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182997&action=review

> Source/WebCore/page/ChromeClient.h:181
> +    virtual void scrollFromJavaScript(const IntPoint&) const { }; // Currently only Chromium has non empty implementation.

This is a fairly weak abstraction. What does a "scroll from JavaScript" mean? Does this include programmatic navigations to anchors, for example? Or dispatching a synthetic click event on a scroll bar?

Also, you are saying that this is a de facto standard behavior. So, it's presumably already implemented without a Chromium-only hack?

[John Knottenbelt]

Comment on attachment 182997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182997&action=review

>> Source/WebCore/page/ChromeClient.h:181
>> +    virtual void scrollFromJavaScript(const IntPoint&) const { }; // Currently only Chromium has non empty implementation.
> 
> This is a fairly weak abstraction. What does a "scroll from JavaScript" mean? Does this include programmatic navigations to anchors, for example? Or dispatching a synthetic click event on a scroll bar?
> 
> Also, you are saying that this is a de facto standard behavior. So, it's presumably already implemented without a Chromium-only hack?

Thanks for reviewing. You are quite correct, there are more sources of programmatic scroll than I originally considered. I think that existing browsers implement this behaviour by means of a platformWidget for the ScrollView, which implements (amongst other things) ScrollView::setScrollPosition. The platformWidget can then differentiate between user and programmatic scroll.

If, as in Chrome's case, there is no hostWidget, ScrollView::setScrollPosition will eventually call through to HostWindow::scroll, so that is a potential place to hook in. Unfortunately, this won't happen if the scroll position hasn't changed.

[John Knottenbelt]

Created attachment 184015 [details]
Patch

[WebKit Review Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16038654

[John Knottenbelt]

Comment on attachment 182997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182997&action=review

>>> Source/WebCore/page/ChromeClient.h:181
>>> +    virtual void scrollFromJavaScript(const IntPoint&) const { }; // Currently only Chromium has non empty implementation.
>> 
>> This is a fairly weak abstraction. What does a "scroll from JavaScript" mean? Does this include programmatic navigations to anchors, for example? Or dispatching a synthetic click event on a scroll bar?
>> 
>> Also, you are saying that this is a de facto standard behavior. So, it's presumably already implemented without a Chromium-only hack?
> 
> Thanks for reviewing. You are quite correct, there are more sources of programmatic scroll than I originally considered. I think that existing browsers implement this behaviour by means of a platformWidget for the ScrollView, which implements (amongst other things) ScrollView::setScrollPosition. The platformWidget can then differentiate between user and programmatic scroll.
> 
> If, as in Chrome's case, there is no hostWidget, ScrollView::setScrollPosition will eventually call through to HostWindow::scroll, so that is a potential place to hook in. Unfortunately, this won't happen if the scroll position hasn't changed.

It seems that platformWidget scrolling, or delegatesScrolling() is the only existing way to get all scroll events to the embedder. delegatesScrolling() is part of the implementation of TILED_BACKING_STORE, which is not used by Chromium. Also, Chromium doesn't use platformWidget for scrolling. 

Existing code paths (apart from the two mentioned above) squash any notifications where the scroll is to the same offset, but any programmatic scroll (even to the same position) should cause the URL bar to hide. 

Adding a method to ChromeClient seems to me to be a reasonable way to get these programmatic scroll events out to the embedder, unless there is better way?

[Early Warning System Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16045698

[Early Warning System Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16044676

[Build Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16065017

[kov's GTK+ EWS bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16036709

[Peter Beverloo (cr-android ews)]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16063135

[EFL EWS Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16036727

[James Robinson]

(In reply to comment #7)
> (From update of attachment 182997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182997&action=review
> 
> >>> Source/WebCore/page/ChromeClient.h:181
> >>> +    virtual void scrollFromJavaScript(const IntPoint&) const { }; // Currently only Chromium has non empty implementation.
> >> 
> >> This is a fairly weak abstraction. What does a "scroll from JavaScript" mean? Does this include programmatic navigations to anchors, for example? Or dispatching a synthetic click event on a scroll bar?
> >> 
> >> Also, you are saying that this is a de facto standard behavior. So, it's presumably already implemented without a Chromium-only hack?
> > 
> > Thanks for reviewing. You are quite correct, there are more sources of programmatic scroll than I originally considered. I think that existing browsers implement this behaviour by means of a platformWidget for the ScrollView, which implements (amongst other things) ScrollView::setScrollPosition. The platformWidget can then differentiate between user and programmatic scroll.
> > 
> > If, as in Chrome's case, there is no hostWidget, ScrollView::setScrollPosition will eventually call through to HostWindow::scroll, so that is a potential place to hook in. Unfortunately, this won't happen if the scroll position hasn't changed.
> 
> It seems that platformWidget scrolling, or delegatesScrolling() is the only existing way to get all scroll events to the embedder. delegatesScrolling() is part of the implementation of TILED_BACKING_STORE, which is not used by Chromium. Also, Chromium doesn't use platformWidget for scrolling. 
> 
> Existing code paths (apart from the two mentioned above) squash any notifications where the scroll is to the same offset, but any programmatic scroll (even to the same position) should cause the URL bar to hide. 

Please add tests to cover the behavior(s) you care about so we can make sure we have consistent behavior.


> 
> Adding a method to ChromeClient seems to me to be a reasonable way to get these programmatic scroll events out to the embedder, unless there is better way?

[Build Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16040900

[Build Bot]

Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16033983

[John Knottenbelt]

Created attachment 184513 [details]
Patch

[John Knottenbelt]

Fixed build error. 

I've added layout tests at  https://bugs.webkit.org/show_bug.cgi?id=107835
and DumpRenderTree support code at https://bugs.webkit.org/show_bug.cgi?id=107834

Both of the above depend on the Chromium layer at 
  https://bugs.webkit.org/show_bug.cgi?id=107027

Do you prefer to have it all in one big patch? Or should I break it up another way?

Thanks

[John Knottenbelt]

Created attachment 184514 [details]
Patch

[John Knottenbelt]

Any feedback, comments much appreciated.

[Alexey Proskuryakov]

I appreciate the improvements you made based on feedback, but I still feel that the concept of "javascript initiated scroll" is fairly weak, and cannot be clearly defined. JavaScript can intercept and affect nearly everything.

A user action to scroll down can be intercepted to do a scroll up, and I don't know if that would be user initiated of JS initiated.

Conversely, a user may enable continuous scrolling (e.g. in a book reader app), which would be implemented in JS and/or CSS, but conceptually remain a result of direct user action.

In fact, CSS scrolling is an interesting thing to consider in the context of this patch too - I don't think that the delegate function would be invoked.

This comment is not meant to block this patch, it's not the weakest abstraction we have.

[Ryosuke Niwa]

Comment on attachment 184514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184514&action=review

> Source/WebCore/page/ChromeClient.h:181
> +    // Notify the ChromeClient that a programmatic (non user-initiated) scroll has taken place.

Please get rid of this comment. If you think this comment is helpful, then please rename the function to reflect the semantics of this comment first.

[John Knottenbelt]

Created attachment 186640 [details]
Patch

[John Knottenbelt]

Comment on attachment 184514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184514&action=review

>> Source/WebCore/page/ChromeClient.h:181
>> +    // Notify the ChromeClient that a programmatic (non user-initiated) scroll has taken place.
> 
> Please get rid of this comment. If you think this comment is helpful, then please rename the function to reflect the semantics of this comment first.

Done.

[John Knottenbelt]

Created attachment 186643 [details]
Patch

[John Knottenbelt]

Thanks, Alexey. I agree with your comments, but I'm not sure what I can do to address them.  

I have changed the title of the bug to "programmatic", instead of "Javascript-initiated" to try to make it a little bit clearer what we are looking for. 

As you have mentioned, there are programmatic scrolls that can result from user action. In some cases, the embedder should be able to determine that programmatic scroll has happened during a user gesture, and if it so chooses ignore them. It won't be possible in all cases, however (e.g. the book reader app / infinite style scrolling). 

(In reply to comment #21)
> I appreciate the improvements you made based on feedback, but I still feel that the concept of "javascript initiated scroll" is fairly weak, and cannot be clearly defined. JavaScript can intercept and affect nearly everything.
> 
> A user action to scroll down can be intercepted to do a scroll up, and I don't know if that would be user initiated of JS initiated.
> 
> Conversely, a user may enable continuous scrolling (e.g. in a book reader app), which would be implemented in JS and/or CSS, but conceptually remain a result of direct user action.
> 
> In fact, CSS scrolling is an interesting thing to consider in the context of this patch too - I don't think that the delegate function would be invoked.
> 
> This comment is not meant to block this patch, it's not the weakest abstraction we have.

[Build Bot]

Comment on attachment 186643 [details]
Patch

Attachment 186643 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16372890

[Alexey Proskuryakov]

Programmatic scroll is an existing concept in WebCore, see FrameView::inProgrammaticScroll(). I'm not sure if the goal should be to unify the concepts, or to disambiguate them.

[John Knottenbelt]

Alexey, our compositor applies user scroll updates in WebViewImpl::applyScrollAndScale, we are calling mainFrameImpl()->frameView()->scrollBy(scrollDelta) and/or Page::setPageScaleFactor which calls into FrameView::setScrollPosition. 

But, FrameView::setScrollPosition() is always considered to be a m_inProgrammaticScroll. So in this sense, we have lost the distinction between programmatic and compositor-originated scroll.

However, if we changed the compositor to call another API, we could preserve this m_inProgrammaticScroll flag's meaning for us. Do you have any suggestions as to what API would be better for us to call than the above? 

The motivation for this change is so that Chromium on Android can hide the URL bar when the user calls window.scrollTo(0,0), so it seems I would still need to get an event out to Chromium on programmatic scrolls so that the URL bar can be hidden when the user calls window.scrollTo(0, 0) or window.scrollTo(0, 1). I'm not sure how I can do this without adding an additional escape hatch in the ChromeClient, calling it from setScrollPosition.

At the moment, I am able to tell whether the scroll is compositor-originated scrolls by setting a flag in when entering WebViewImpl::applyScrollAndScale, and clearing it on exit. Any programmatic scroll events that are fired while the flag is set can be ignored.

[James Robinson]

(In reply to comment #29)
> Alexey, our compositor applies user scroll updates in WebViewImpl::applyScrollAndScale, we are calling mainFrameImpl()->frameView()->scrollBy(scrollDelta) and/or Page::setPageScaleFactor which calls into FrameView::setScrollPosition. 
> 
> But, FrameView::setScrollPosition() is always considered to be a m_inProgrammaticScroll. So in this sense, we have lost the distinction between programmatic and compositor-originated scroll.
> 

That sounds like a bug - interactions from chromium's scrolling thread are user interactions and should not be considered programatic scrolls.

> However, if we changed the compositor to call another API, we could preserve this m_inProgrammaticScroll flag's meaning for us. Do you have any suggestions as to what API would be better for us to call than the above? 

Can you file a new bug on this issue where we can figure it out?

[John Knottenbelt]

Thanks James, filed https://bugs.webkit.org/show_bug.cgi?id=109712

(In reply to comment #30)
> (In reply to comment #29)
> > Alexey, our compositor applies user scroll updates in WebViewImpl::applyScrollAndScale, we are calling mainFrameImpl()->frameView()->scrollBy(scrollDelta) and/or Page::setPageScaleFactor which calls into FrameView::setScrollPosition. 
> > 
> > But, FrameView::setScrollPosition() is always considered to be a m_inProgrammaticScroll. So in this sense, we have lost the distinction between programmatic and compositor-originated scroll.
> > 
> 
> That sounds like a bug - interactions from chromium's scrolling thread are user interactions and should not be considered programatic scrolls.
> 
> > However, if we changed the compositor to call another API, we could preserve this m_inProgrammaticScroll flag's meaning for us. Do you have any suggestions as to what API would be better for us to call than the above? 
> 
> Can you file a new bug on this issue where we can figure it out?

[John Knottenbelt]

Created attachment 188607 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188607 [details]
Patch

Attachment 188607 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16589211

[Build Bot]

Comment on attachment 188607 [details]
Patch

Attachment 188607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16590205

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 188607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188607&action=review

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:304
> +    ChromeClientChromium* client = static_cast<ChromeClientChromium*>(
> +        m_page->chrome()->client());

This static_cast looks pretty dubious....

> Source/WebKit/chromium/src/WebViewImpl.cpp:4209
> +    m_insideCompositor = true;

These sort of state variable tend to cause maintenance  problems.  We have a bunch of them in FrameLoader and they cause a mess.  It's better for the controlflow to be explicit rather than implicit.

[James Robinson]

It's also not clear that we want to support this at all in chromium.  I don't think we should worry too much about the implementation until that decision is final.

[John Knottenbelt]

Comment on attachment 188607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188607&action=review

>> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:304
>> +        m_page->chrome()->client());
> 
> This static_cast looks pretty dubious....

I copied this from PopupContainer::chromeClientChromium(). I'm looking for a way to get the event to WebViewImpl, where the decision can be made on whether to request the URL bar to show or hide. 

The reasons why I have it in WebViewImpl:
  - so that we can know if I'm in a compositor scroll or not (m_insideCompositor), but this will go away once https://bugs.webkit.org/show_bug.cgi?id=109712 is solved.
  - so that we can call a method on the WebViewClient to show or hide the URL bar.
  - so that we can know if the URL bar is currently showing or not (might be needed to the decision to show or hide it).

I notice that there are already some methods on ChromeClient itself: setToolbarsVisible, setStatusbarVisible, setScrollbarsVisible, setMenuBarVisible. These are encapsulated in the WebCore::WindowFeatures class. Apparently they are designed to mimic the IE window features (e.g. http://stackoverflow.com/questions/2909645/open-new-popup-window-without-address-bars-in-firefox-ie ) that you can specify for popup windows. I wonder, should we try to integrate with this feature?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:4209
>> +    m_insideCompositor = true;
> 
> These sort of state variable tend to cause maintenance  problems.  We have a bunch of them in FrameLoader and they cause a mess.  It's better for the controlflow to be explicit rather than implicit.

I think we can avoid this state variable if we fix https://bugs.webkit.org/show_bug.cgi?id=109712 first.

[Adam Barth]

(In reply to comment #38)
> (From update of attachment 188607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188607&action=review
> 
> >> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:304
> >> +        m_page->chrome()->client());
> > 
> > This static_cast looks pretty dubious....
> 
> I copied this from PopupContainer::chromeClientChromium(). I'm looking for a way to get the event to WebViewImpl, where the decision can be made on whether to request the URL bar to show or hide. 
> 
> The reasons why I have it in WebViewImpl:
>   - so that we can know if I'm in a compositor scroll or not (m_insideCompositor), but this will go away once https://bugs.webkit.org/show_bug.cgi?id=109712 is solved.
>   - so that we can call a method on the WebViewClient to show or hide the URL bar.
>   - so that we can know if the URL bar is currently showing or not (might be needed to the decision to show or hide it).
> 
> I notice that there are already some methods on ChromeClient itself: setToolbarsVisible, setStatusbarVisible, setScrollbarsVisible, setMenuBarVisible. These are encapsulated in the WebCore::WindowFeatures class. Apparently they are designed to mimic the IE window features (e.g. http://stackoverflow.com/questions/2909645/open-new-popup-window-without-address-bars-in-firefox-ie ) that you can specify for popup windows. I wonder, should we try to integrate with this feature?

It's likely you should add a virtual function to ChromeClient.  ("Chrome" in this name has nothing to do with Chrome the browser and instead realizes to the browser's non-content area.)  You shouldn't need a static_cast.

> >> Source/WebKit/chromium/src/WebViewImpl.cpp:4209
> >> +    m_insideCompositor = true;
> > 
> > These sort of state variable tend to cause maintenance  problems.  We have a bunch of them in FrameLoader and they cause a mess.  It's better for the controlflow to be explicit rather than implicit.
> 
> I think we can avoid this state variable if we fix https://bugs.webkit.org/show_bug.cgi?id=109712 first.

Great!

[John Knottenbelt]

Comment on attachment 188607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188607&action=review

>>>> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:304
>>>> +        m_page->chrome()->client());
>>> 
>>> This static_cast looks pretty dubious....
>> 
>> I copied this from PopupContainer::chromeClientChromium(). I'm looking for a way to get the event to WebViewImpl, where the decision can be made on whether to request the URL bar to show or hide. 
>> 
>> The reasons why I have it in WebViewImpl:
>>   - so that we can know if I'm in a compositor scroll or not (m_insideCompositor), but this will go away once https://bugs.webkit.org/show_bug.cgi?id=109712 is solved.
>>   - so that we can call a method on the WebViewClient to show or hide the URL bar.
>>   - so that we can know if the URL bar is currently showing or not (might be needed to the decision to show or hide it).
>> 
>> I notice that there are already some methods on ChromeClient itself: setToolbarsVisible, setStatusbarVisible, setScrollbarsVisible, setMenuBarVisible. These are encapsulated in the WebCore::WindowFeatures class. Apparently they are designed to mimic the IE window features (e.g. http://stackoverflow.com/questions/2909645/open-new-popup-window-without-address-bars-in-firefox-ie ) that you can specify for popup windows. I wonder, should we try to integrate with this feature?
> 
> It's likely you should add a virtual function to ChromeClient.  ("Chrome" in this name has nothing to do with Chrome the browser and instead realizes to the browser's non-content area.)  You shouldn't need a static_cast.

Sounds good to me. I think that integrating with the WindowFeatures / setToolbarsVisible etc methods at this time is not a good idea, as this will likely impact window.open and window.showModalDialog APIs, which is not the intention of this work.

[John Knottenbelt]

Created attachment 190072 [details]
Patch

[John Knottenbelt]

This patch changes approach to intercepting only window.scrollTo API, as this is what developers are actually using to hide the URL bar. 

Regarding testing, does it sound like a reasonable approach to add a counter to DumpRenderTree to the count the number of calls to WebViewClient:: hideTopControls()? I can then write a layout test that verifies that the counter is incremented or not in the various situations.

[Eric Seidel (no email)]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

> Source/WebCore/page/DOMWindow.cpp:1441
> +    if (!x && (!y || y == 1)) {

So scrollTo(0,2) won't hide? I thought scrollTo was supposed to act as though the user had scrolled?

[John Knottenbelt]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:4240
> +    // FIXME: Also receive the value of m_topControlsHeight.

Since m_topControlsHeight doesn't change very often (at all?) I think having it be passed in in applyScrollAndScale may not be best. Does it make sense to make a separate public method to receive this value?

> Source/WebKit/chromium/src/WebViewImpl.cpp:4454
> +    // The idea is to get the top of the screen showing the given scroll offset,

Sorry, this comment has some bits taken from previous patch. There is no "given scroll offset", as we just want to hide the URL bar. However, we shouldn't hide the URL bar if it's not possible for the user scroll to scroll by the height of the URL bar.

[John Knottenbelt]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

>> Source/WebCore/page/DOMWindow.cpp:1441
>> +    if (!x && (!y || y == 1)) {
> 
> So scrollTo(0,2) won't hide? I thought scrollTo was supposed to act as though the user had scrolled?

In this patch, the idea is that the API is window.scrollTo(0,0) and window.scrollTo(0,1) hides the URL bar. This is why window.scrollTo(0,2) doesn't hide it, because that's not the API. We know that developers are using window.scrollTo(0,0) and window.scrollTo(0,1). I've not seen any uses of window.scrollTo(0,2) to deliberately hide the URL bar. 

It's possible to make window.scrollTo(X,Y) hide the URL bar, as if a user had scrolled for any X,Y. The question must then be asked, should window.scrollBy, element.scrollIntoView, document.body.scrollTop = etc, also hide the URL bar? It seems to me that all the above, and more, should behave as if the user had scrolled it. Viewed like this, then there is no API, as such, to hide the URL bar, rather we are just making the WebKit scrolls behave in the same way as user scrolls. I'd be happy to try to implement this if it is felt that this is better and more consistent than the API approach in the first paragraph.

[Eric Seidel (no email)]

(In reply to comment #45)
> (From update of attachment 190072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review
>
> It's possible to make window.scrollTo(X,Y) hide the URL bar, as if a user had scrolled for any X,Y. The question must then be asked, should window.scrollBy, element.scrollIntoView, document.body.scrollTop = etc, also hide the URL bar? It seems to me that all the above, and more, should behave as if the user had scrolled it. Viewed like this, then there is no API, as such, to hide the URL bar, rather we are just making the WebKit scrolls behave in the same way as user scrolls. I'd be happy to try to implement this if it is felt that this is better and more consistent than the API approach in the first paragraph.

I suspect we'll end up matching whatever behavior other mobile browsers have.  I haven't done enough research to know what that is.

[Eric Seidel (no email)]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

>>> Source/WebCore/page/DOMWindow.cpp:1441
>>> +    if (!x && (!y || y == 1)) {
>> 
>> So scrollTo(0,2) won't hide? I thought scrollTo was supposed to act as though the user had scrolled?
> 
> In this patch, the idea is that the API is window.scrollTo(0,0) and window.scrollTo(0,1) hides the URL bar. This is why window.scrollTo(0,2) doesn't hide it, because that's not the API. We know that developers are using window.scrollTo(0,0) and window.scrollTo(0,1). I've not seen any uses of window.scrollTo(0,2) to deliberately hide the URL bar. 
> 
> It's possible to make window.scrollTo(X,Y) hide the URL bar, as if a user had scrolled for any X,Y. The question must then be asked, should window.scrollBy, element.scrollIntoView, document.body.scrollTop = etc, also hide the URL bar? It seems to me that all the above, and more, should behave as if the user had scrolled it. Viewed like this, then there is no API, as such, to hide the URL bar, rather we are just making the WebKit scrolls behave in the same way as user scrolls. I'd be happy to try to implement this if it is felt that this is better and more consistent than the API approach in the first paragraph.

If we chose to implement the behavior where these APIs tried to act like a user scroll, than presumably this would be done at a different level.  Since Mobile Safari doesn't add this line of code here (as far as I know), presumably they also do the hook at a different level.  That's not to suggest that this choice in behavior is incorrect however.  The way you've added it here it seems more like adding a deprecated quirk to support a website.  If we believe this is an important feature for the mobile web (hiding the URL bar on JS-based scroll) then it seems we should do it in a less hacky way for the long-term.

[Eric Seidel (no email)]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:4456
> +    // if possible. This isn't possible if the document isn't big enough. We assume
> +    // that the location bar is showing for this check, since hiding it is idempotent.

I assume this matches mobile Safari?  (Assuming that's the goal?)

[John Knottenbelt]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

>> Source/WebKit/chromium/src/WebViewImpl.cpp:4456
>> +    // that the location bar is showing for this check, since hiding it is idempotent.
> 
> I assume this matches mobile Safari?  (Assuming that's the goal?)

Yes, this matches Mobile Safari and Android Browser. 

If we have a short document, e.g. http://jsbin.com/ohitug/2 , that cannot be scrolled by the user to hide the URL bar, window.scrollTo shouldn't be able to hide it either. 

If, on the otherhand, window.scrollTo can hide the URL bar, the user ought to be able (if nothing is expressly trying to block him/her) to do scroll back up to reveal the URL bar again.

[John Knottenbelt]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

>>>> Source/WebCore/page/DOMWindow.cpp:1441
>>>> +    if (!x && (!y || y == 1)) {
>>> 
>>> So scrollTo(0,2) won't hide? I thought scrollTo was supposed to act as though the user had scrolled?
>> 
>> In this patch, the idea is that the API is window.scrollTo(0,0) and window.scrollTo(0,1) hides the URL bar. This is why window.scrollTo(0,2) doesn't hide it, because that's not the API. We know that developers are using window.scrollTo(0,0) and window.scrollTo(0,1). I've not seen any uses of window.scrollTo(0,2) to deliberately hide the URL bar. 
>> 
>> It's possible to make window.scrollTo(X,Y) hide the URL bar, as if a user had scrolled for any X,Y. The question must then be asked, should window.scrollBy, element.scrollIntoView, document.body.scrollTop = etc, also hide the URL bar? It seems to me that all the above, and more, should behave as if the user had scrolled it. Viewed like this, then there is no API, as such, to hide the URL bar, rather we are just making the WebKit scrolls behave in the same way as user scrolls. I'd be happy to try to implement this if it is felt that this is better and more consistent than the API approach in the first paragraph.
> 
> If we chose to implement the behavior where these APIs tried to act like a user scroll, than presumably this would be done at a different level.  Since Mobile Safari doesn't add this line of code here (as far as I know), presumably they also do the hook at a different level.  That's not to suggest that this choice in behavior is incorrect however.  The way you've added it here it seems more like adding a deprecated quirk to support a website.  If we believe this is an important feature for the mobile web (hiding the URL bar on JS-based scroll) then it seems we should do it in a less hacky way for the long-term.

Android Browser, and I believe Mobile Safari, hook in using the platformWidget abstraction. This is called by ScrollView::setScrollPosition. Android Browser, at least, requires some further changes to DOMWindow::scrollY so that negative scroll offsets are not exposed back to Javascript like http://gitorious.org/webkit-clutter/webkit-clutter/commit/f87a88934bd32fb1f6a0befbda004cfcc9d79aa2?format=diff (sorry I can't find a better public reference), which never made it to upstream WebKit.

I agree that window.scrollTo(0,0) and window.scrollTo(0,1) sounds like a deprecated quirk, but many mobile websites are using exactly this method to hide the URL bar.

In previous patches, I have been trying to align the scroll behaviours more generally by hooking in at FrameView::setScrollPosition. From your comments, it's sounds like something like this, or similar approach, might be better received.

[Eric Seidel (no email)]

(In reply to comment #50)
> (From update of attachment 190072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review
>
> I agree that window.scrollTo(0,0) and window.scrollTo(0,1) sounds like a deprecated quirk, but many mobile websites are using exactly this method to hide the URL bar.

I guess that part is unclear to me.  What are they replaced by?

[Simon Fraser (smfr)]

(In reply to comment #45)
> (From update of attachment 190072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review
> 
> >> Source/WebCore/page/DOMWindow.cpp:1441
> >> +    if (!x && (!y || y == 1)) {
> > 
> > So scrollTo(0,2) won't hide? I thought scrollTo was supposed to act as though the user had scrolled?
> 
> In this patch, the idea is that the API is window.scrollTo(0,0) and window.scrollTo(0,1) hides the URL bar. This is why window.scrollTo(0,2) doesn't hide it, because that's not the API. We know that developers are using window.scrollTo(0,0) and window.scrollTo(0,1). I've not seen any uses of window.scrollTo(0,2) to deliberately hide the URL bar. 
> 
> It's possible to make window.scrollTo(X,Y) hide the URL bar, as if a user had scrolled for any X,Y. The question must then be asked, should window.scrollBy, element.scrollIntoView, document.body.scrollTop = etc, also hide the URL bar? It seems to me that all the above, and more, should behave as if the user had scrolled it. Viewed like this, then there is no API, as such, to hide the URL bar, rather we are just making the WebKit scrolls behave in the same way as user scrolls. I'd be happy to try to implement this if it is felt that this is better and more consistent than the API approach in the first paragraph.

I wouldn't call scrollTo(0, 1) an "API". Arguably it's just an iOS bug that you're copying. I really don't want the WebCore codebase hard-coding this bug.

[James Robinson]

(In reply to comment #52)
> (In reply to comment #45)
> > (From update of attachment 190072 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review
> > 
> > >> Source/WebCore/page/DOMWindow.cpp:1441
> > >> +    if (!x && (!y || y == 1)) {
> > > 
> > > So scrollTo(0,2) won't hide? I thought scrollTo was supposed to act as though the user had scrolled?
> > 
> > In this patch, the idea is that the API is window.scrollTo(0,0) and window.scrollTo(0,1) hides the URL bar. This is why window.scrollTo(0,2) doesn't hide it, because that's not the API. We know that developers are using window.scrollTo(0,0) and window.scrollTo(0,1). I've not seen any uses of window.scrollTo(0,2) to deliberately hide the URL bar. 
> > 
> > It's possible to make window.scrollTo(X,Y) hide the URL bar, as if a user had scrolled for any X,Y. The question must then be asked, should window.scrollBy, element.scrollIntoView, document.body.scrollTop = etc, also hide the URL bar? It seems to me that all the above, and more, should behave as if the user had scrolled it. Viewed like this, then there is no API, as such, to hide the URL bar, rather we are just making the WebKit scrolls behave in the same way as user scrolls. I'd be happy to try to implement this if it is felt that this is better and more consistent than the API approach in the first paragraph.
> 
> I wouldn't call scrollTo(0, 1) an "API". Arguably it's just an iOS bug that you're copying. I really don't want the WebCore codebase hard-coding this bug.

I tend to agree, but it's been copied by the Android browser (and possibly others) and is being used by many popular web sites.  We need to either hardcode this specific call or come up with some more general model.  It appears that doing something is necessary for compatibility with the mobile web.

[Simon Fraser (smfr)]

Comment on attachment 190072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190072&action=review

> Source/WebCore/page/Chrome.h:129
> +    void hideLocationBar() const;

I don't think a function like this belongs in Chrome. It makes assumptions about app behavior. If you want to add some "did scroll to" callback that's OK, but don't make it about the location bar.

[Anders Carlsson]

Given that this was a Chromium feature, I think we can close this bug.