Last Comment Bug 772679 - subpixel shifting causes visible shift of some lines on second scroll operation to hit bottom of page
: subpixel shifting causes visible shift of some lines on second scroll operati...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 782546
Blocks: 681192 776381
  Show dependency treegraph
 
Reported: 2012-07-10 16:05 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2012-08-15 17:19 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed


Attachments
fix (2.25 KB, patch)
2012-07-10 22:31 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
Details | Diff | Review
fix v2 (15.46 KB, patch)
2012-07-29 17:27 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review
Testcase for this bug (2.99 KB, patch)
2012-07-29 17:27 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review
Mark Mac test as passing (1.25 KB, patch)
2012-07-29 17:28 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review
fix crash (1.27 KB, patch)
2012-08-04 14:46 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review
fix test (2.60 KB, patch)
2012-08-05 16:04 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-10 16:05:10 PDT
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.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-10 16:15:20 PDT
Regression window in mozilla-central nightlies is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=654ac86492e8&tochange=e6a9572b48f7
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-10 16:20:17 PDT
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).
Comment 3 Timothy Nikkel (:tnikkel) 2012-07-10 16:21:38 PDT
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?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-10 18:03:57 PDT
(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.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-10 22:31:00 PDT
Created attachment 640925 [details] [diff] [review]
fix

I need to come up with a test for this.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-10 23:04:35 PDT
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 7 Timothy Nikkel (:tnikkel) 2012-07-11 15:51:26 PDT
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?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-11 19:10:19 PDT
(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.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-11 19:11:02 PDT
(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.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-12 21:08:17 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/e42786561b60
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-12 22:08:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9acc5029800e

Backed out due to test failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e42786561b60
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-25 23:13:10 PDT
I pretty much rewrote this patch...
https://tbpl.mozilla.org/?tree=Try&rev=4d5112d7c5cc
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-29 17:27:12 PDT
Created attachment 647027 [details] [diff] [review]
fix v2

This is the patch, completely rewritten.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-29 17:27:55 PDT
Created attachment 647028 [details] [diff] [review]
Testcase for this bug
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-29 17:28:45 PDT
Created attachment 647029 [details] [diff] [review]
Mark Mac test as passing

This patch makes the existing test pass on Mac.
Comment 16 Timothy Nikkel (:tnikkel) 2012-08-02 22:53:03 PDT
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.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-04 05:26:54 PDT
(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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-04 06:19:54 PDT
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
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-04 14:46:57 PDT
Created attachment 649033 [details] [diff] [review]
fix crash
Comment 23 Ed Morley [:emorley] 2012-08-05 06:35:51 PDT
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
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-05 16:03:31 PDT
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
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-05 16:04:15 PDT
Created attachment 649150 [details] [diff] [review]
fix test
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-09 18:00:29 PDT
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.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-09 18:00:34 PDT
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.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-09 18:00:39 PDT
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.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-09 18:00:44 PDT
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.
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-09 18:00:48 PDT
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.
Comment 33 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-15 14:29:27 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.