The default bug view has changed. See this FAQ.

subpixel shifting causes visible shift of some lines on second scroll operation to hit bottom of page

RESOLVED FIXED in Firefox 16

Status

()

Core
Layout: View Rendering
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dbaron, Assigned: roc)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla17
x86_64
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ wontfix, firefox16+ fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Steps to reproduce:
 1. load bug 763838 (or anything else long, really)
 2. hit the [End] key
 3. hit the up arrow key
 4. hit the down arrow key
 5. hit the down arrow key again

Expected results:
 after step 5: no change

Actual results:
 after step 5: some lines shift and others don't

I see this on Linux, and have been for a while (months?), and find it pretty annoying.
(Reporter)

Comment 1

5 years ago
Regression window in mozilla-central nightlies is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=654ac86492e8&tochange=e6a9572b48f7
(Reporter)

Updated

5 years ago
Blocks: 681192
(Reporter)

Comment 2

5 years ago
Er, now that I look through the other dependencies of bug 681192, this is probably a duplicate of bug 756400 or bug 767710 (neither of which has "bottom" in the bug summary, but both probably should).
This is probably what's happening. So let's say the maximum scroll position is 6010.4 pixels. Hitting end scrolls us to 6010.4. Hitting the up arrow goes to say 6000.4, but we then round that to the nearest dev pixel and scroll to 6000. Down arrow goes to 6010, down arrow again goes to 6010.4.

Is there a reason we don't want to restrict to dev pixels even when scrolling to the end?
(In reply to Timothy Nikkel (:tn) from comment #3)
> This is probably what's happening. So let's say the maximum scroll position
> is 6010.4 pixels. Hitting end scrolls us to 6010.4. Hitting the up arrow
> goes to say 6000.4, but we then round that to the nearest dev pixel and
> scroll to 6000.

Such rounding should not happen, we should be staying at 6000.4.
Created attachment 640925 [details] [diff] [review]
fix

I need to come up with a test for this.
Assignee: nobody → roc
Attachment #640925 - Flags: review?(tnikkel)
(Reporter)

Comment 6

5 years ago
Does this also fix it if you scroll from the top rather than using the [End] key?  (comment 4 wouldn't, I think, but this seems different.)
Comment on attachment 640925 [details] [diff] [review]
fix

>+  // Convert the result to layer pixels --- coordinates in the space of
>+  // a ThebesLayer.
>+  // aCurrentLayerOffset is the coordinates of the top-left of the scrolled frame,
>+  // in ThebesLayer space.
>+  double layerVal = (aRes*aDesired)/aAppUnitsPerPixel + aCurrentLayerOffset;

Adding aCurrentLayerOffset is the right thing to do here, not subtracting?
Attachment #640925 - Flags: review?(tnikkel) → review+
(In reply to David Baron [:dbaron] from comment #6)
> Does this also fix it if you scroll from the top rather than using the [End]
> key?  (comment 4 wouldn't, I think, but this seems different.)

I'm not sure exactly what you mean. When you scroll down gently to the end, the last scroll operation may indeed scroll by a subpixel amount causing lines to move by a pixel relative to each other. However, that would normally happen in the same repaint as the whole page moves by some number of pixels, so I would expect it to be quite difficult to see.
(In reply to Timothy Nikkel (:tn) from comment #7)
> >+  // Convert the result to layer pixels --- coordinates in the space of
> >+  // a ThebesLayer.
> >+  // aCurrentLayerOffset is the coordinates of the top-left of the scrolled frame,
> >+  // in ThebesLayer space.
> >+  double layerVal = (aRes*aDesired)/aAppUnitsPerPixel + aCurrentLayerOffset;
> 
> Adding aCurrentLayerOffset is the right thing to do here, not subtracting?

Yes. The comment was supposed to make that clear.
http://hg.mozilla.org/integration/mozilla-inbound/rev/e42786561b60
https://hg.mozilla.org/integration/mozilla-inbound/rev/9acc5029800e

Backed out due to test failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e42786561b60
status-firefox15: --- → affected
tracking-firefox15: --- → ?

Updated

5 years ago
tracking-firefox15: ? → +
Keywords: regression
I pretty much rewrote this patch...
https://tbpl.mozilla.org/?tree=Try&rev=4d5112d7c5cc
Created attachment 647027 [details] [diff] [review]
fix v2

This is the patch, completely rewritten.
Attachment #640925 - Attachment is obsolete: true
Attachment #647027 - Flags: review?(tnikkel)
Created attachment 647028 [details] [diff] [review]
Testcase for this bug
Attachment #647028 - Flags: review?(tnikkel)
Created attachment 647029 [details] [diff] [review]
Mark Mac test as passing

This patch makes the existing test pass on Mac.
Attachment #647028 - Flags: review?(tnikkel) → review+
Comment on attachment 647027 [details] [diff] [review]
fix v2

>diff --git a/gfx/2d/BaseRect.h b/gfx/2d/BaseRect.h
>--- a/gfx/2d/BaseRect.h
>+++ b/gfx/2d/BaseRect.h

This hunk seems unrelated? I can't see it being used anywhere in this patch.

Sorry for the delay.
Attachment #647027 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #16)
> >diff --git a/gfx/2d/BaseRect.h b/gfx/2d/BaseRect.h
> >--- a/gfx/2d/BaseRect.h
> >+++ b/gfx/2d/BaseRect.h
> 
> This hunk seems unrelated? I can't see it being used anywhere in this patch.

Good point. Removed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6424adfb7ac2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7566f863512b
https://hg.mozilla.org/integration/mozilla-inbound/rev/313b5b97e7e3
Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e63fd4fc05a0

for crashes like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14126158&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14126208&tree=Mozilla-Inbound
Thanks.

Probably just a silly bug. We do this:
    if (f->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER) {
      nsTArray<DisplayItemData>* array = GetDisplayItemDataArrayForFrame(f);
And assume array is non-null. Of course it's possible for a frame to have NS_FRAME_HAS_CONTAINER_LAYER and not have any display items ... the root frame will probably fall into this category!

https://tbpl.mozilla.org/?tree=Try&rev=ac925b233cac
Created attachment 649033 [details] [diff] [review]
fix crash
Attachment #649033 - Flags: review?(tnikkel)
Attachment #649033 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89ae41eed63
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15fb3603bfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/14d17919e235
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea008b301da
This caused bug 776381 to go perma-orange (native Android mochitest-7), so has unfortunately had to be backed out.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android%20Tegra%20250%20mozilla-inbound%20opt%20test%20mochitest-7&rev=6ea008b301da

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8871b8e913
Depends on: 776381
It seems test_offsets.js fails because the mochitest viewport has been zoomed, causing the scrolling code to select a subpixel offset for the scroll operation. Rounding the change in getBoundingClientRect().top/.left should hide the subpixel choices and make the test pass.

https://tbpl.mozilla.org/?tree=Try&rev=281dc4834850
Created attachment 649150 [details] [diff] [review]
fix test
Attachment #649150 - Flags: review?(tnikkel)
Attachment #649150 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8364cb62506e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2127fc9bd2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ee6a1ae6cfc
https://hg.mozilla.org/integration/mozilla-inbound/rev/970496fa31dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efd09dda9e1
https://hg.mozilla.org/mozilla-central/rev/8364cb62506e
https://hg.mozilla.org/mozilla-central/rev/b2127fc9bd2b
https://hg.mozilla.org/mozilla-central/rev/1ee6a1ae6cfc
https://hg.mozilla.org/mozilla-central/rev/970496fa31dd
https://hg.mozilla.org/mozilla-central/rev/6efd09dda9e1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Blocks: 776381
No longer depends on: 776381
Comment on attachment 647027 [details] [diff] [review]
fix v2

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

Not very risky, fixes a subtle but annoying issue. Also speeds up scrolling in some situations.
Attachment #647027 - Flags: approval-mozilla-aurora?
Comment on attachment 647028 [details] [diff] [review]
Testcase for this bug

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

Not very risky, fixes a subtle but annoying issue. Also speeds up scrolling in some situations.
Attachment #647028 - Flags: approval-mozilla-aurora?
Comment on attachment 647029 [details] [diff] [review]
Mark Mac test as passing

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

Not very risky, fixes a subtle but annoying issue. Also speeds up scrolling in some situations.
Attachment #647029 - Flags: approval-mozilla-aurora?
Comment on attachment 649033 [details] [diff] [review]
fix crash

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

Not very risky, fixes a subtle but annoying issue. Also speeds up scrolling in some situations.
Attachment #649033 - Flags: approval-mozilla-aurora?
Comment on attachment 649150 [details] [diff] [review]
fix test

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

Not very risky, fixes a subtle but annoying issue. Also speeds up scrolling in some situations.
Attachment #649150 - Flags: approval-mozilla-aurora?
status-firefox16: --- → affected
tracking-firefox16: --- → +
Attachment #647027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #647028 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #647029 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #649033 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #649150 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 782546
This has been approved for Aurora for 5 days, please land to that branch.  Wontfixing for Beta at this point since we've already gone to build for beta 5 and we won't take this so late in the Beta cycle.
status-firefox15: affected → wontfix
https://hg.mozilla.org/releases/mozilla-aurora/rev/169ec60d32c9
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee69702b3f71
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca10ea6d11ea
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1c542259ffc
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb0208e24e22
status-firefox16: affected → fixed
You need to log in before you can comment on or make changes to this bug.