Text bounces when you scroll page with mouse

VERIFIED FIXED in mozilla24

Status

()

--
major
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: Virtual, Assigned: karlt)

Tracking

({nightly-community, regression})

15 Branch
mozilla24
x86_64
All
nightly-community, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

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
Keywords: regression, regressionwindow-wanted

Comment 1

6 years ago
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
Blocks: 681192
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regressionwindow-wanted
Product: Firefox → Core
Version: Trunk → 15 Branch
tracking-firefox18: --- → ?
Summary: odd text movement on page scrolling with mouse → Text bounces when you scroll page with mouse

Comment 2

6 years ago
Longstanding regression, no need to track for a specific release.
Assignee: nobody → roc
tracking-firefox18: ? → -
(Assignee)

Updated

6 years ago
Depends on: 756400
OS: Windows 7 → All
(Assignee)

Updated

6 years ago
Assignee: roc → karlt
(Assignee)

Updated

6 years ago
Depends on: 767710
No longer depends on: 756400
(Assignee)

Comment 4

6 years ago
Created attachment 743988 [details] [diff] [review]
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.
(Assignee)

Comment 5

6 years ago
Created attachment 743995 [details] [diff] [review]
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.
Attachment #743988 - Attachment is obsolete: true
Attachment #743995 - Flags: review?(roc)
(Assignee)

Comment 6

6 years ago
Created attachment 743996 [details] [diff] [review]
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.
Attachment #743996 - Flags: review?(roc)
Can these patches be landed or we're waiting for better fix with no side-effects?
(Assignee)

Comment 9

6 years ago
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
Flags: needinfo?(jwalden+bmo)
(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?
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 11

6 years ago
(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
(Assignee)

Comment 12

6 years ago
Created attachment 758378 [details] [diff] [review]
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.
Attachment #758378 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #758378 - Flags: review?(roc)
(Assignee)

Comment 13

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

Had the warning test reversed.
Attachment #758380 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #758378 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 758382 [details] [diff] [review]
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.
Attachment #758382 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 15

6 years ago
Created attachment 758383 [details] [diff] [review]
Part 1.1: work around Visual Studio not understanding typeof(Abs(t))

For folding into part 1, for separate review.
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.  :-)
Attachment #758382 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 17

6 years ago
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
Attachment #758382 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #758383 - Attachment is obsolete: true
(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.
(Assignee)

Comment 19

6 years ago
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
Attachment #758380 - Flags: checkin+
(Assignee)

Updated

6 years ago
Depends on: 880492
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla24
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.
Flags: needinfo?(roc)
Flags: needinfo?(karlt)
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 26

3 years ago
(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.
Flags: needinfo?(karlt)
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?
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.
What about extending the scroll range to the next whole device pixel?
Then you have the opposite problem, you can render 1px of nothing below the bottom edge of the content.
(Assignee)

Comment 31

3 years ago
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.
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?
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.
You need to log in before you can comment on or make changes to this bug.