scrollTop and scrollLeft might contain wrong values for scripts on pages

VERIFIED FIXED

Status

defect
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: MikeK, Assigned: mfinkle)

Tracking

(Blocks 1 bug)

Trunk
All
Other
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b4+)

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

Due to the caching of browser output in fennec to enable fast panning and zooming, the browser will in some cases give wrong values to scripts running on web pages.  

An example is when you show a web page and then scroll one line down, the top of the page will still be "visibel" as far as the browser is concerned, hence scrollTop will still contain 0 - but the top will not be visble to the end user, as we have just scrolled down one line.

For description of scrollTop see: https://developer.mozilla.org/en/DOM/element.scrollTop

For description of scrollLeft see: https://developer.mozilla.org/en/DOM/element.scrollleft

For an introduction to the panning and zooming in fennec see: https://wiki.mozilla.org/Mobile/Fennec/Architecture#Panning.2FZooming

Another issue to consider here is that a script might be able to get the visible vindow size (the size of the "browser" window), we need to verify that this is reported correctly as the size of the fennec window. And if not, to fix it.

One interface a script might use can be found here: http://dev.w3.org/csswg/cssom-view/
No longer depends on: 499212
Posted patch wip (obsolete) — Splinter Review
This bug is the root cause of many bugs we currently have and some of them looks formfill related but appears because the underlying engine is also waiting for right scrollTop/scrollLeft positions.

This patch sync the browser and the content view and try to avoid as much as possible the perf impact by ignoring all resulting scroll/MozAfterPaint events.

I still need to set up a good test for measuring performances impact of thus type of change. One of Mark's suggestion is to use xresponse to simulate mousedown, mousemove, mouseup and measuring the perfs.
From IRC:

vingtetun: a normal build vs a build with patch in bug 479862 give me a perf regression of -.6700% (which can be noise i guess)

This test was made using xresponse to "time" the panning performance.
Blocks: 559873
Blocks: 560431
Comment on attachment 428336 [details] [diff] [review]
wip

>diff -r d61fdff66712 chrome/content/browser.js

>   dragStop: function dragStop(dx, dy, scroller) {

>-    this.bv.resumeRendering();
>+    // Sync browser with view and ignore resulting paint/scroll events
>+    let bv = this.bv;
>+    Util.executeSoon(function() {
>+      bv.ignorePageScroll(true);
>+      bv.ignorePagePaint(true);
>+    });
>+    Browser.scrollBrowserToContent();
>+    Util.executeSoon(function() {
>+      bv.ignorePagePaint(false);
>+      bv.ignorePageScroll(false);
>+    });
>+
>+    bv.resumeRendering();

This doesn't seem right. By using executeSoon, you essentialy get this for code execution:

Browser.scrollBrowserToContent(); // executed right away
bv.ignorePageScroll(true); // delayed by first executeSoon
bv.ignorePagePaint(true); // delayed by first executeSoon
bv.ignorePageScroll(false); // delayed by second executeSoon
bv.ignorePagePaint(false); // delayed by second executeSoon
Blocks: 560016
Blocks: 547307
Whiteboard: mobile-triage
Whiteboard: mobile-triage
tracking-fennec: --- → ?
Assignee: 21 → webapps
tracking-fennec: ? → 2.0b1+
Depends on: 576192
tracking-fennec: 2.0b1+ → 2.0+
Duplicate of this bug: 559873
From the duplicate bug:
1. Open http://www.hskupin.info/photos/
2. Scroll down and click on one of the images

An overlay will open and the image is displayed way up the page so you have to
scroll a lot. Instead the image should be displayed in the area which is
currently visible. That's the way how it works in Firefox.
tracking-fennec: 2.0+ → 2.0b3+
If this bug is important, and I think it is, we should start working on it. Ben, if you can't start on it, we need to reassign.
Sure. While we are doing this, it would be nice to just set displayport and leave it alone. Instead of updating displayport, we should update the content scroll offset.
we need a plan for this for beta 3
Assignee: ben → mark.finkle
Depends on: 612599, 612604
Given the dependencies, this won't land for b3
tracking-fennec: 2.0b3+ → 2.0b4+
Blocks: 616348
Blocks: 617355
Duplicate of this bug: 617355
Posted patch patch (obsolete) — Splinter Review
This is an updated patch based loosely on the previous WIP. I do see some of the gray lines mentioned in bug 612599 with this patch active.
Attachment #428336 - Attachment is obsolete: true
The website in comment 8 works well with this patch, except for the gray lines.
Vivien - Can you pick this up?
Assignee: mark.finkle → 21
Blocks: 617952
Assignee: 21 → mark.finkle
(In reply to comment #16)
> The website in comment 8 works well with this patch, except for the gray lines.

Well, there is also a problem where the scroll snaps back to the top. I think I need to ignore a "scroll" event.
Posted patch patch 2 (obsolete) — Splinter Review
This patch seems to fix the original problem and also not break left/right panning too.
Attachment #501384 - Attachment is obsolete: true
Attachment #505056 - Flags: review?(21)
Posted patch patch 3Splinter Review
Same as before, but now we check to make sure the "Content:ScrollTo" / "Content:ScrollBy" message will actually cause a "scroll" event to be fired. If we don't the ignoreScroll flag will be out of sync.
Attachment #505056 - Attachment is obsolete: true
Attachment #505065 - Flags: review?(21)
Attachment #505056 - Flags: review?(21)
Comment on attachment 505065 [details] [diff] [review]
patch 3

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js
>       case "Content:ScrollBy":
>+        if (!json.dx && json.dy)
>+          return;
>+

I think you mean if (!json.dx && !json.dy) ?

r+ with comments addressed. 
This is so good to see this bug fixed!
Attachment #505065 - Flags: review?(21) → review+
(In reply to comment #21)

> >       case "Content:ScrollBy":
> >+        if (!json.dx && json.dy)
> >+          return;
> >+
> 
> I think you mean if (!json.dx && !json.dy) ?

Yep!

pushed with that change:
http://hg.mozilla.org/mobile-browser/rev/1c390a1b9699
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I need to update the cache viewport too, or we are left with a big gray block after kinetic panning stops.
Attachment #505110 - Flags: review?(ben)
Comment on attachment 505110 [details] [diff] [review]
followup patch

FWIW this could be jerky because CSS scroll offset is changed and then displayport message is processed. This should probably be done in one message if I'm right.
Attachment #505110 - Flags: review?(ben) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/ae4e1645e425

(ben mentioned on IRC that he saw no jerky jiggle)
Verified:
Mozilla/5.0 (Android; Linux armv71; rv2.0b10pre) Gecko/20110120 Firefox/4.0b10pre Fennec/4.0b4pre
Something (possibly this patch) seems to have regressed scrolling by clicking on in-page links (URI fragments), for example:
http://people.mozilla.com/~mbrubeck/test/in-page-link.html
Depends on: 627500
(In reply to comment #27)
> Something (possibly this patch) seems to have regressed scrolling by clicking
> on in-page links (URI fragments), for example:
> http://people.mozilla.com/~mbrubeck/test/in-page-link.html

See bug 627500.
regressed by bug 627500
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
fixed by patch in bug 627500
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Following the steps given in comment 8 I was not able to reproduce it, marking it as verified fixed.

Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110517 Firefox/6.0a1 Fennec/6.0a1 
Device: LG Optimus 2X (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.