812517 - Rendering of webpages creates jiggly/snapping effect when scrolling & zooming (text is still dancing)

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Peter La]

This is probably a low-level rendering issue, but I feel it really impacts the feeling of QUALITY for the browsing experience.  It is worse with light text on dark backgrounds.

Example.

Go to www.starcraft2.com's mainpage (after splash page) and scroll up and down through the list of news items.  Try it again slightly zoomed in.

It is expected that the page is rendered smoothly as it scrolls, especially the text, but it currently exhibits a weird jiggly/pixel-snapping behaviour that looks very low-quality.  It makes for a terrible browsing experience, and gives the feeling that the product is poorly made.

Go to the same page on iPhone and examine the behaviour as you scroll.  The rendering looks SOLID.

This is not a framerate issue.  It's something about the way it's rendered, there is some pixel snapping happening that makes it very jarring.

Comment 1

[Peter La]

Tested on Nov. 15 Unagi nightly build.

Comment 2

[Peter La]

The same effect happens when zooming in and out.

Example.

Go to www.thestar.com and zoom in and out on the front page.  Notice the snapping behaviour on the content being rendered is very rough at the end of a pinch or 'reverse-pinch' gesture.  Terrible!

Comment 3

[Ben Francis [:benfrancis]]

"This is probably a low-level rendering issue"

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

roc, have all the bugs landed that were supposed to fix this?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

roc, have all the bugs landed that were supposed to fix this?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On the Gecko side, we've landed all the patches that were supposed to make this sort of thing better, but I think we could still have issues in some cases.

However, on Firefox for Android, I don't see the described problem on www.starcraft2.com. On Android FF, www.thestart.com redirects to a mobile site which isn't zoomable. I wonder what could be different that would make it look worse on B2G.

I'll try it on B2G when a B2G phone is free. Is the problem reproducible on a B2G desktop build?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> However, on Firefox for Android, I don't see the described problem on
> www.starcraft2.com. On Android FF, www.thestart.com redirects to a mobile
> site which isn't zoomable. I wonder what could be different that would make
> it look worse on B2G.
> 

Last I saw, the android frontend has some hackery built in to nudge scroll offsets around outside of gecko.  That's certainly a possible workaround but it would be nice to have gecko own all that code.

> I'll try it on B2G when a B2G phone is free. Is the problem reproducible on
> a B2G desktop build?

Yep.  Simple STR
 (1) Load nytimes.com
 (2) Double-click middle section of articles to zoom in on them
 (3) Click and pan around in a circle

The text dances around.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Not a perf issue.)

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> I'll try it on B2G when a B2G phone is free. Is the problem reproducible on
> a B2G desktop build?

I should add though, that you need to test in OS X or x11 desktop builds with bug 799768 applied.  omtc isn't supported on windows.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Simple testcase

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As I sort of expected, in nsGfxScrollFrame.cpp AlignWithLayerPixels is unable to scroll by a whole number of layer pixels when we're zoomed in. An example of debug spew:
Layer pixel alignment failed: aDesired=26580, aLower=26550, aUpper=26609, aAppUnitsPerPixel=60, aRes=0.507937, aCurrent=26400
So currentLayerVal is 223.49228 and desiredLayerVal is 225.016091. delta is 1.523811. That rounds up to 2 and down to 1, but 223.49228 + 2 is greater than aUpper/60=225.2616 and 223.49228 + 1 is less than aLower/60=224.7621. Basically we're trying to set scrollTop to 443 but scrolling by a whole number of layer pixels can't land us in the range 443 +/- 0.5. Known problem.

The simple fix here is to modify TabChild's call to nsGlobalWindow::ScrollTo to allow it to use a much larger range, assuming we don't really care exactly what scrollLeft/scrollTop are after the scroll call. I'll need to check that assumption though.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> The simple fix here is to modify TabChild's call to nsGlobalWindow::ScrollTo
> to allow it to use a much larger range

Woohoo!  So this was user error.  Thanks for looking.

> assuming we don't really care
> exactly what scrollLeft/scrollTop are after the scroll call. I'll need to
> check that assumption though.

We don't.  APZC adjusts itself internally whenever content sends scroll offset values apzc didn't expect.  (Since this can happen at any time if content calls scrollBy/To() itself.)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Add nsIScrolFrame::ScrollToCSSPixelsApproximate

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Call ScrollToCSSPixelsApproximate when TabChild scrolls

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These patches seem to fix things nicely.

I'm a little concerned that perhaps pages won't track the user's finger precisely when we're not scrolling by exactly the amount requested. Indeed, when I scroll on b2g-desktop by dragging with the mouse, the scroll position does not track the mouse cursor. However, this also happens without these patches, so maybe one really needs to test this on device. If there is a problem there, it probably needs to be fixed in APZC.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If the scroll position passed in the FrameMetrics tracks the finger movement accurately, then I'm pretty sure this code will do the right thing. The only issue would be if the feedback of the scroll position to APZC confuses things.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> As I sort of expected, in nsGfxScrollFrame.cpp AlignWithLayerPixels is
> unable to scroll by a whole number of layer pixels when we're zoomed in. An
> example of debug spew:

Is this one of the cases where we flush thebes layer retained content?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, we are doing a lot of re-rendering without these patches.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

These patches are working great for killing dancing text.  Thanks!

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 684178 [details] [diff] [review]
Part 1: Add nsIScrolFrame::ScrollToCSSPixelsApproximate

>layout/generic/nsGfxScrollFrame.cpp
>+  printf("Layer pixel alignment failed: aDesired=%d, aLower=%d, aUpper=%d, aAppUnitsPerPixel=%d, aRes=%f, aCurrent=%d\n",
>+         aDesired, aLower, aUpper, aAppUnitsPerPixel, aRes, aCurrent);

You forgot to remove this.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 684179 [details] [diff] [review]
Part 2: Call ScrollToCSSPixelsApproximate when TabChild scrolls

>dom/ipc/TabChild.cpp
>+static void
>+ScrollWindowTo(nsIDOMWindow* aWindow, const mozilla::gfx::Point& aPoint)
>+{
>+    nsGlobalWindow* window = static_cast<nsGlobalWindow*>(aWindow);
>+    nsIScrollableFrame *sf = window->GetScrollFrame();

s/nsIScrollableFrame */nsIScrollableFrame* /

>+    if (sf) {
>+        sf->ScrollToCSSPixelsApproximate(gfxPoint(aPoint.x, aPoint.y));
>+    }

I'm curious why you chose gfxPoint for the API rather than gfx::Point?
Double precision doesn't seem necessary for an approximate point and
converting what is a fractional CSS pixels value to begin with
(FrameMetrics::mScrollOffset) from gfx::Point to gfxPoint looks a bit odd.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #21)
> I'm curious why you chose gfxPoint for the API rather than gfx::Point?
> Double precision doesn't seem necessary for an approximate point and
> converting what is a fractional CSS pixels value to begin with
> (FrameMetrics::mScrollOffset) from gfx::Point to gfxPoint looks a bit odd.

No particular reason. I'll change it to gfx::Point if you like.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/523f2e2189d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56607b87bcb

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/523f2e2189d7
https://hg.mozilla.org/mozilla-central/rev/e56607b87bcb

Comment 25

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> No particular reason. I'll change it to gfx::Point if you like.

I don't have a strong preference for either type per se, I just didn't see
any reason to involve a second type to pass along the gfx::Point value.

It's a general problem with these generic gfx types in Layout though that
it's sometimes hard to say what kind of value it is.  For example, the
APIs here could be improved if we had a CSSPoint type.  That would make
it unnecessary to have "CSSPixels" in the method name or documenting what
kind of point it is - the compiler would enforce it's the right type.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 684179 [details] [diff] [review]
Part 2: Call ScrollToCSSPixelsApproximate when TabChild scrolls

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Nasty visual effects when scrolling in B2G browser, and slower than necessary
Testing completed (on m-c, etc.): only just landed on m-c
Risk to taking this patch (and alternatives if risky): Very low risk to non-B2G, since we just add a new API and call it from code that is only used on B2G currently
String or UUID changes made by this patch: none

Need approval for both patches in this bug.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The risk for b2g is low as well, and there's no alternative for fixing these awful artifacts.

Comment 29

[Alex Keybl [:akeybl]]

No impact to Desktop/Mobile, so no need to track for their releases. That being said, this meets the criteria of b-b+ for perf/usability, so marking as a P3.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c114c7a5a35
https://hg.mozilla.org/releases/mozilla-beta/rev/b9aac74460e7

(This is also in my queue to land on Aurora once it reopens)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Bustage fix for beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/5bd2ea37c907

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/643559cc0fcc
https://hg.mozilla.org/releases/mozilla-aurora/rev/b416c87a63df