810274 - Text bounces when you scroll page with mouse

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Go to http://benhollis.net/ and scroll page with mouse to see that text content moves oddly. It not happens when you scroll page with scrollbar

Comment 1

[Alice0775 White]

I can reproduce.
http://hg.mozilla.org/mozilla-central/rev/90cea19e27e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121109030635

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/6fe7dd2f8f57
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120510021321
Bad:
http://hg.mozilla.org/mozilla-central/rev/b7b6565d12a0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120510050721
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6fe7dd2f8f57&tochange=b7b6565d12a0


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5304fd23df9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120509222721
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/67091352b7d2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120509223521
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b5304fd23df9&tochange=67091352b7d2

Last good: c3d3bfb3b68d
First bad: 9d9a3edaa0b9

Triggered by
9d9a3edaa0b9	Robert O'Callahan — Bug 681192. Part 11: Don't snap scrollrange endpoints to device pixels anymore. r=matspal

Comment 2

[Alex Keybl [:akeybl]]

Longstanding regression, no need to track for a specific release.

Comment 3

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=eefa824e99a9

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Part 1 experiment: include bounds with layer pixels as possible destinations to minimize subpixel scrolls - subpixel scrolls >= 0.5

Something we should change for this bug is the multiple subpixel scroll positions after the last pixel-aligned point has been passed.  We should pick a time to jump to the bound instead of every subpixel position along the way.

This patch was used in the try server run above.  It tried to obscure the
subpixel scroll by making sure that subpixel scrolls where >= 0.5 pixels,
moving approximately at least half the items on the page.  However, the change
in line spacing seems just as visible when both lines are moving as when only
one line is moving and this patch has the effect of sometimes requiring a
scroll of > 1.0 pixel, which makes some lines jump by 2 pixels unnecessarily.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Part 1: include bounds with layer pixels as possible destinations to minimize subpixel scrolls - use bounds if nearer

So I propose this option.  It also ensures we have at most one subpixel scroll,
but picks the frame closest to our continuous animation goal, with the
intention of keeping each line moving as smoothly as possible.  The
disadvantage of this approach is that if the bound is only just beyond the
pixel-aligned position, then the subpixel scroll can happen quite late.
Changes in bug 767710 have improved that situation considerably.

I've taken out the nearestLayerVal != desiredLayerVal, because if they were
equal then aligned would be desired which is now always in
[aDestLower, aDestUpper].

See part 2 for the reason for clamping aDesired to the allowed range.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Part 2: allow smooth scroll step to nearest pixel boundary short of desired position

The code in nsGfxScrollFrameInner::AsyncScrollCallback() wasn't setting up the
range for "any pixel boundary in the right direction" as the comment said, but
instead limiting each frame to move at least the desired distance, so rounding
was always up (until within the final range).

That reduced the number of subpixel scrolls a little (probably by accident)
but is now unnecessary.

This patch has the side-effect of preventing scrolling from continuing in
one direction after an event has requested scrolling in the other (like the
comment said this already did, but didn't).  That comes with the caveat that
aPt is not necessarily within aRange.

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Can these patches be landed or we're waiting for better fix with no side-effects?

Comment 8

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5359a2b9768c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad42847dc2b

Comment 9

[Karl Tomlinson (:karlt)]

Backed out due to:

Visual Studio compile error:
e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/layout/generic/nsGfxScrollFrame.cpp(1815) : error C2061: syntax error : identifier 'typeof'

Jeff, should we move AbsReturnType out of namespace detail, or do you have a better idea?

There are also some assertion failures that I need to investigate in crashtests/323386-1.html and reftests/bugs/582146-1.html:

###!!! ABORT: clamped(): max must be greater than or equal to min: 'max >= min', file ../../dist/include/nsAlgorithm.h, line 66
ClampAndAlignWithPixels [layout/generic/nsGfxScrollFrame.cpp:1798]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0b4d1cd7b6

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Karl Tomlinson (:karlt) from comment #9)
> Visual Studio compile error:
> e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/layout/generic/
> nsGfxScrollFrame.cpp(1815) : error C2061: syntax error : identifier 'typeof'
> 
> Jeff, should we move AbsReturnType out of namespace detail, or do you have a
> better idea?

Why do you need anything like typeof at all here?  aBoundUpper, aBoundLower, desired, and aligned are all nscoord.  If nscoord is float, the cast is a no-op.  If nscoord is int32_t, you have |int32_t - int32_t| on the left, which is |int32_t|, and |uint32_t| on the left.  |int32_t  < uint32_t| promotes the left-hand side to uint32_t exactly like the cast would do.  So isn't it unnecessary too?

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> |int32_t  < uint32_t| promotes the left-hand side to uint32_t exactly like
> the cast would do.  So isn't it unnecessary too?

Yes, but such implicit casts would change the interpretation of the LHS, sometimes unexpectedly, so some compilers give a warning.  gcc gives
"comparison between signed and unsigned integer expressions [-Wsign-compare]"

http://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Warning-Options.html#index-Wsign_002dcompare-406

It was possible to suppress this warning with merely parentheses, if the left hand side was an non-trivial expression,

void f(int32_t a, int32_t b, uint32_t c) {
  a < c; /* warning */
  (a) < c; /* warning */
  a - b < c; /* warning */
  (a - b) < c; /* no warning */
}

but this bug has been fixed in gcc 4.7.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50012

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: part 0: ensure scrolled range is non-negative even in the presence of nscoord overflow

The assertion failures are due to moving from nsRect::ClampPoint(), which uses
max of min, to clamped(), which fatally asserts that min <= max.
min > max because GetScrollRange() using a difference of values where one
value has used saturating arithmetic and the other hasn't.

I'm not sure this is the ideal solution, but probably the simplest change.

Instead of trying to detect all integer overflow at the source, this merely
keeps the sizes non-negative.  That may not mean they are less than
nscoord_MAX.

If we give in and accept that integer arithmetic may result in the scrolled
rect being smaller than the scroll port, then we shouldn't assert.

nsGfXScrollFrameInner::ReflowFinished() makes a similar calculation with
GetScrolledRect().XMost() - mScrollPort.width, which may result in some -ve
attribute values for curpos and maxpos, if certain child elements exist.
nsGfxScrollFrameInner::LayoutScrollbars() may make similar calculations and
call nsBoxFrame::LayoutChildAt() and AdjustScrollbarRectForResizer() with a
rect having a -ve dimension if various child frames exist.  Perhaps we could
fix up those situations but I'm not aware of any problem caused to date.

I'll have to reduce assertion counts in tests.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: part 0: ensure scrolled range is non-negative even in the presence of nscoord overflow

Had the warning test reversed.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: part 0.5: expose return type of Abs()

Even if we didn't have the visual studio problem, the return type of a function is part of the interface and so should be public.  One other use could be declaring a variable to store the result.

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: Part 1.1: work around Visual Studio not understanding typeof(Abs(t))

For folding into part 1, for separate review.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 758382 [details] [diff] [review]
part 0.5: expose return type of Abs()

Review of attachment 758382 [details] [diff] [review]:
-----------------------------------------------------------------

The sanctioned, standard way to do this in C++11 is with decltype(Abs(...)).  clang's had that since 2.9, gcc since 4.3, and MSVC since 2010.  We recently upped our Windows requirements to 2010, so can you use that instead?  Does a try-push using decltype break anything?  I'd rather see that than this, ideally, but won't complain too hard about this if it's unavoidable.

decltype of course requires building as C++11, but I think basically everything in the tree but the JS engine does that now, and that's our problem to deal with, not yours.  :-)

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 758382 [details] [diff] [review]
part 0.5: expose return type of Abs()

decltype() works fine, thanks.  I didn't know typeof was non-standard.

https://tbpl.mozilla.org/?tree=Try&rev=81286c7514e2

Comment 18

[Mike Hommey [:glandium]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> decltype of course requires building as C++11, but I think basically
> everything in the tree but the JS engine does that now, and that's our
> problem to deal with, not yours.  :-)

We still support building with gcc 4.4 on linux distros, and that means building without -std=gnu++0x because of libstdc++ headers bug. We can however start reconsidering in Firefox 25. But not before.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 758380 [details] [diff] [review]
part 0: ensure scrolled range is non-negative even in the presence of nscoord overflow

This part has landed with adjustments in reftest assertions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9419b209de19
https://hg.mozilla.org/integration/mozilla-inbound/rev/bec3013abff6

Comment 20

[Karl Tomlinson (:karlt)]

Parts 1 and 2 landed but were backed out again for lack on decltype with 4.4 (comment 18).

https://tbpl.mozilla.org/php/getParsedLog.php?id=23844558&tree=Mozilla-Inbound#error0

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4add9815dd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/db258cb900a7

Backout merge was
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3eb4b73292

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9419b209de19
https://hg.mozilla.org/mozilla-central/rev/bec3013abff6

Comment 22

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43d505a966f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc34df66e54d

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43d505a966f3
https://hg.mozilla.org/mozilla-central/rev/fc34df66e54d

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

I'm quite confused about the change in patch 1.

If we have a scrollable frame, whose scroll range is 0~100.4, and the initial scroll position is 0, ClampAndAlignWithPixels will align every position passed in to the integral pixel, like 1, 2, 3. However, if you try to scroll to the end, the position then becomes 100.4, and after that, this function align every position to X.4, like 1.4, 2.4, 3.4, until the next time you scroll to the start boundary.

I wonder if this is really the desired behavior. It seems weird that the exact scroll position is path-dependent. We probably want to always align the scroll position to an integral pixel unless we are near the boundary, instead of trying to align the delta to be integral.

Thoughts?

Note that, this is currently not detectable from the content, because scroll properties are all using an integral type. But there is a plan to change them to use double in bug 1217330, which would expose this issue.

Comment 25

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #24)
> I wonder if this is really the desired behavior. It seems weird that the
> exact scroll position is path-dependent. We probably want to always align
> the scroll position to an integral pixel unless we are near the boundary,
> instead of trying to align the delta to be integral.

That may be a good idea.

I suppose the downside is that scrolling up and down repeatedly at the end of the range may require repainting the entire window every time. With the current approach, we only have to repaint every time the page scrolls through the entire range.

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> I suppose the downside is that scrolling up and down repeatedly at the end
> of the range may require repainting the entire window every time. With the
> current approach, we only have to repaint every time the page scrolls
> through the entire range.

Yes, that was my understanding of the reason for the current behavior.

It would be nice to snap the end point to device pixels so that we never scroll to sub-device-pixel positions.  I don't recall the reason for sub-device-pixel end points: perhaps something about needing to handle 1-pixel viewports as a special case.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Yeah, I then have the same question. Is it possible to just get rid of sub-device-pixel scrolling, so that we never need to repaint the entire window for that?

Comment 28

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

If we can't scroll to sub-device-pixel positions at the end of the scroll range, there can be 1 pixel wide/high content at the end of the scroll range (positioned at a subpixel offset) that we can't scroll into view. This could the caret, for horizontal scrolling (or vertical with the appropriate writing-mode), or it could be a 1px border.

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

What about extending the scroll range to the next whole device pixel?

Comment 30

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

Then you have the opposite problem, you can render 1px of nothing below the bottom edge of the content.

Comment 31

[Karl Tomlinson (:karlt)]

If snapping end to nearest device pixel, then 1 pixel wide objects at the end should always snap into full view, assuming consistent logic for the snapping.

Perhaps objects less than one-pixel in thickness can be problematic though.

If the document background could be extended to cover the 1 pixel of nothing, then perhaps that wouldn't be so bad.

Comment 32

[Chris Lord [:cwiiis]]

Would it be so terrible if you couldn't expose the last fractional pixel of content? Doing so will make the entire content re-render and shift slightly, which looks quite distracting/bad, and I challenge anyone to do anything useful with less than a pixel. Assuming the caret is at least 1 pixel, it will always at least be visible, and if there's no padding after it it'd be just as hard to see whether it consists of 1 whole pixel or a sub-pixel. What do other browsers do in this situation?

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

I have no idea how to test the behavior for sub-pixel rendering. It seems if you do not zoom the content, Chrome does not have sub-pixel scrolling, and if you zoom to 500%, you'll have a 0.2px step for scrollLeft/scrollTop. So it looks like they always snap the scroll position to device pixel.