Bug 107026

Summary: Hide the location bar on window.scrollTo(0,[01])
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: New BugsAssignee: John Knottenbelt <jknotten>
Status: CLOSED WONTFIX    
Severity: Normal CC: abarth, andersca, anilsson, ap, bdakin, buildbot, cmarcelo, dglazkov, efidler, eric, fishd, gtk-ews, jamesr, laszlo.gombos, luiz, mvanouwerkerk, ojan.autocc, peter+ews, rniwa, simon.fraser, tdanderson, thorton, tkent+wkapi, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107834    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review-

Description John Knottenbelt 2013-01-16 09:56:30 PST
Allow Javascript-initiated scrolls to be detected.
Comment 1 John Knottenbelt 2013-01-16 09:58:16 PST
Created attachment 182997 [details]
Patch
Comment 2 John Knottenbelt 2013-01-16 10:04:23 PST
This change is needed by https://bugs.webkit.org/show_bug.cgi?id=107027
Comment 3 Alexey Proskuryakov 2013-01-17 10:21:08 PST
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?
Comment 4 John Knottenbelt 2013-01-18 09:10:27 PST
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.
Comment 5 John Knottenbelt 2013-01-22 10:46:38 PST
Created attachment 184015 [details]
Patch
Comment 6 WebKit Review Bot 2013-01-22 11:01:12 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16038654
Comment 7 John Knottenbelt 2013-01-22 11:01:24 PST
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?
Comment 8 Early Warning System Bot 2013-01-22 11:02:43 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16045698
Comment 9 Early Warning System Bot 2013-01-22 11:10:00 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16044676
Comment 10 Build Bot 2013-01-22 11:19:12 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16065017
Comment 11 kov's GTK+ EWS bot 2013-01-22 11:23:47 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16036709
Comment 12 Peter Beverloo (cr-android ews) 2013-01-22 11:36:19 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16063135
Comment 13 EFL EWS Bot 2013-01-22 11:59:15 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16036727
Comment 14 James Robinson 2013-01-22 13:20:49 PST
(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?
Comment 15 Build Bot 2013-01-22 17:18:25 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16040900
Comment 16 Build Bot 2013-01-22 19:43:46 PST
Comment on attachment 184015 [details]
Patch

Attachment 184015 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16033983
Comment 17 John Knottenbelt 2013-01-24 09:16:42 PST
Created attachment 184513 [details]
Patch
Comment 18 John Knottenbelt 2013-01-24 09:19:15 PST
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
Comment 19 John Knottenbelt 2013-01-24 09:30:47 PST
Created attachment 184514 [details]
Patch
Comment 20 John Knottenbelt 2013-02-04 09:47:46 PST
Any feedback, comments much appreciated.
Comment 21 Alexey Proskuryakov 2013-02-04 10:28:21 PST
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.
Comment 22 Ryosuke Niwa 2013-02-04 10:47:34 PST
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.
Comment 23 John Knottenbelt 2013-02-05 09:12:18 PST
Created attachment 186640 [details]
Patch
Comment 24 John Knottenbelt 2013-02-05 09:12:38 PST
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.
Comment 25 John Knottenbelt 2013-02-05 09:21:17 PST
Created attachment 186643 [details]
Patch
Comment 26 John Knottenbelt 2013-02-05 09:31:02 PST
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.
Comment 27 Build Bot 2013-02-05 12:35:31 PST
Comment on attachment 186643 [details]
Patch

Attachment 186643 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16372890
Comment 28 Alexey Proskuryakov 2013-02-08 14:30:44 PST
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.
Comment 29 John Knottenbelt 2013-02-13 08:14:09 PST
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.
Comment 30 James Robinson 2013-02-13 10:18:01 PST
(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?
Comment 31 John Knottenbelt 2013-02-13 10:42:13 PST
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?
Comment 32 John Knottenbelt 2013-02-15 11:30:09 PST
Created attachment 188607 [details]
Patch
Comment 33 WebKit Review Bot 2013-02-15 11:52:29 PST
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
Comment 34 Build Bot 2013-02-15 12:04:03 PST
Comment on attachment 188607 [details]
Patch

Attachment 188607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16590205
Comment 35 WebKit Review Bot 2013-02-15 13:30:44 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 36 Adam Barth 2013-02-15 14:57:38 PST
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.
Comment 37 James Robinson 2013-02-15 16:04:16 PST
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.
Comment 38 John Knottenbelt 2013-02-18 08:44:20 PST
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.
Comment 39 Adam Barth 2013-02-18 10:53:52 PST
(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!
Comment 40 John Knottenbelt 2013-02-21 07:28:15 PST
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.
Comment 41 John Knottenbelt 2013-02-25 09:13:02 PST
Created attachment 190072 [details]
Patch
Comment 42 John Knottenbelt 2013-02-25 09:18:03 PST
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.
Comment 43 Eric Seidel (no email) 2013-02-25 09:21:40 PST
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?
Comment 44 John Knottenbelt 2013-02-25 09:24:16 PST
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.
Comment 45 John Knottenbelt 2013-02-25 09:34:59 PST
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.
Comment 46 Eric Seidel (no email) 2013-02-25 09:37:31 PST
(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.
Comment 47 Eric Seidel (no email) 2013-02-25 09:40:20 PST
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.
Comment 48 Eric Seidel (no email) 2013-02-25 09:41:19 PST
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?)
Comment 49 John Knottenbelt 2013-02-25 09:47:12 PST
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.
Comment 50 John Knottenbelt 2013-02-25 10:06:53 PST
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.
Comment 51 Eric Seidel (no email) 2013-02-25 10:36:51 PST
(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?
Comment 52 Simon Fraser (smfr) 2013-02-25 10:40:24 PST
(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.
Comment 53 James Robinson 2013-02-25 10:47:54 PST
(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.
Comment 54 Simon Fraser (smfr) 2013-02-25 11:48:18 PST
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.
Comment 55 Anders Carlsson 2013-05-02 11:40:11 PDT
Given that this was a Chromium feature, I think we can close this bug.