window.scrollTo should cause urlbar to hide in Fennec

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
3 months ago

People

(Reporter: mbrubeck, Unassigned)

Tracking

(Blocks 1 bug, {testcase})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Posted file test case (obsolete) —
Mobile web sites often use window.scrollTo(0,1) to hide the urlbar on page load.  This is especially important for sites like Google Maps because of bug 653956.

Steps to reproduce:
1. Open http://limpet.net/w3/scrollTo.html or the attached test case.

Expected results: The urlbar should scroll out of view on page load.
Actual results: The urlbar usually does not scroll out of view on page load.
This seems to be a timing/race issue.  If I wrap a sufficiently large (e.g. 100ms) setTimeout around the hideTitlebar call in Browser.receiveMessage, then it works fine.  Calling hideTitlebar while the page is loading does not seem to work correctly.
This is a race with the scrollTo call in firstPaintListener:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#1528
Status: NEW → ASSIGNED
Posted patch patch (obsolete) — Splinter Review
This fixed the bug.  Not asking for review yet; need to write some tests and make sure I didn't regress anything.
tracking-fennec: ? → ---
Whiteboard: [has patch][needs tests][fennec-6?]
Whiteboard: [has patch][needs tests][fennec-6?] → [has patch][fennec-6?]
Now with a test!

There is still a case that doesn't work, which we need to fix.  When the displayport is big enough to include the whole scrolled area, then we don't get a "scroll" event at all, so the front-end is never notified to hide the titlebar.  Any ideas, Ben?
Attachment #529582 - Attachment is obsolete: true
Attachment #529701 - Flags: review?(ben)
Posted file test case v2
Attachment #529487 - Attachment is obsolete: true
Blocks: 638271
Depends on: 612604
Comment on attachment 529701 [details] [diff] [review]
part 1 (checked in)

Love it. Great job Matt.
Attachment #529701 - Flags: review?(ben) → review+
Comment on attachment 529701 [details] [diff] [review]
part 1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/5d1e8ed138db
Attachment #529701 - Attachment description: part 1 → part 1 (checked in)
Posted patch part 2 (obsolete) — Splinter Review
(In reply to comment #4)
> There is still a case that doesn't work, which we need to fix.  When the
> displayport is big enough to include the whole scrolled area, then we don't get
> a "scroll" event at all, so the front-end is never notified to hide the
> titlebar.  Any ideas, Ben?

This fixes the above case.  The problems was actually that scrollTo() was called after resetMetadata() but before updateMetadata() - so at the time, the page had the default CSS viewport rather than the intended viewport.  The default viewport was tall enough to include the entire document, so scrollTo() had no effect.

resetMetadata is not actually needed.  The original code to reset viewport metadata to null before pageload was added in bug 567186, to try to reduce the number of resizes during load.  While it does reduce resizes in some cases, it actually increases them in others (for example, any time you navigate to a page that sets its own viewport width).  All tests still pass with this patch applied.

Also fixed in this patch: I forgot to actually add the new test to the Makefile in part 1, and I removed the finish() call during testing and forgot to add it back.

There's still a problem when navigating from a page with a different viewport width.  Very early in page load, the old page's viewport will still be in effect.  Ideally, scrollTo() in content would still generate some sort of message to chrome even when the CSS viewport can't scroll, so that we can hide the titlebar even if we don't scroll the content.  I'll file a followup bug for that.
Attachment #529859 - Flags: review?(ben)
Depends on: 654741
(In reply to comment #9)
> Ideally, scrollTo() in content would still generate some sort of message to
> chrome even when the CSS viewport can't scroll, so that we can hide the
> titlebar even if we don't scroll the content.  I'll file a followup bug for
> that.

Filed bug 654741.
Matt,

I'm confused how this fixes the problem. Wouldn't scrollTo still be called before updateMetadata()? Why would removing resetMetadata() make scrollTo happen later?

If this works because the previous loaded page just happens to have a small enough viewport, this seems like a bad fix.
Comment on attachment 529859 [details] [diff] [review]
part 2

Hmm, good point - this does depend on the viewport of the previous page.  It happens to work when opening the test case in a new tab (which is why it fixes the automated test), but not when navigating from a wide-viewport page to a narrow-viewport page.

Perhaps we should send updated viewport info along with the "scroll" message...
Attachment #529859 - Flags: review?(ben)
It looks like we do manage send the viewport metadata before the "load" event, but it doesn't take effect until we get a response from the parent process, which doesn't happen until after the "load" event:

child got "DOMWindowCreated"
  child sent ViewportMetadata
child got "DOMMetaAdded"
  child sent ViewportMetadata
parent received ViewportMetadata
  parent sent SetWindowSize
got "load"
parent received ViewportMetadata
  parent sent SetWindowSize
child received SetWindowSize
child received SetWindowSize

We could fix this case by moving more of the viewport calculation into the child process, so that it takes effect immediately without waiting for an IPC round trip.
This is an excellent idea, IMO.
Comment on attachment 529859 [details] [diff] [review]
part 2

Requesting review on this anyway, since it removes some unneeded code and enables these test cases (so we can ensure additional changes don't regress them).
Attachment #529859 - Flags: review?(ben)
Comment on attachment 529859 [details] [diff] [review]
part 2

As we discussed on IRC, please try on device and try to see if page loading feels worse. r+. Is there a bug filed for the work you are doing on moving viewport stuff to the content process, or will that happen here?
Attachment #529859 - Flags: review?(ben) → review+
(In reply to comment #16)
> Is there a bug filed for the work you are doing on moving
> viewport stuff to the content process, or will that happen here?

I'll add that patch to this bug, since the relevant discussion is here already.
Try server should be running some Talos tests for Mobile
Comment on attachment 529859 [details] [diff] [review]
part 2

I'm not checking this in, because it does sometimes lead to a more flickery experience on device (Galaxy Tab).
Attachment #529859 - Attachment is obsolete: true
Comment on attachment 529859 [details] [diff] [review]
part 2

Checked in only the test-enabling part of this patch:
http://hg.mozilla.org/mozilla-central/rev/62352944d597
Just a snapshot.  This is currently very broken.
Blocks: 614353
No longer blocks: 614353
Blocks: 674693
Whiteboard: [has patch][fennec-6?]
Assignee: mbrubeck → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
Closing all opened bug in a graveyard component
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.