Closed Bug 1148418 Opened 5 years ago Closed 4 years ago

Scrolling leaves horizontal artifacts on page

Categories

(Core :: Graphics, defect)

36 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 + wontfix
firefox39 + verified
firefox40 --- verified
firefox41 --- verified
firefox-esr31 --- unaffected

People

(Reporter: stuartmemo, Assigned: jwatt)

References

Details

(Keywords: regression, Whiteboard: [bugday-20150603])

Attachments

(5 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.104 Safari/537.36

Steps to reproduce:

1. Visit http://www.bbc.co.uk/guides/zqjc4wx
2. Scroll down and back up page


Actual results:

Horizontal artifacts appear 50% of time along the division of Step 1 & Step 2.


Expected results:

No artifacts.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=90d9478a29e2&tochange=e497957b79b2
Component: Untriaged → Graphics
Flags: needinfo?(jwatt)
Product: Firefox → Core
via local build(windows7),
Last Good: 4019b458cab7
First Bad: e497957b79b2

Triggered by:
e497957b79b2	Jonathan Watt — Bug 1103623 - Port most remaining gfxContext::Fill() calls to Moz2D. r=mattwoodrow
Blocks: 1103623
There are some snapping changes that I saw when looking at the patch for bug 1103623
Assignee: nobody → jwatt
[Tracking Requested - why for this release]: This is a content drawing regression that should be pretty easy to fix.
Tracking for 38 and 39.
This is a regression we'd like to fix in 38 but if we are going to do that, we'd need it in the next week (for beta 5 which goes to build on Thursday).  If we can't land a fix by the 15th then we may not have time to test this sufficiently for uplift to 38.  Does that sound like a reasonable timeline?
Flags: needinfo?(bugzmuiz)
Jeff, could you have a look to this bug?
Flags: needinfo?(jmuizelaar)
Sylvestre, did you try reaching out to Jonathan Watt? I might get a chance to look at this but I have other 37/38 stuff that I'm working on.
Flags: needinfo?(jmuizelaar)
This is too late for 38 but we will take a patch for 39.
Flags: needinfo?(jwatt)
Needinfo ping.
Flags: needinfo?(jwatt)
I'm not currently able to reproduce this. I've tried scrolling with the trackpad, with the scrollbar and using keyboard shortcuts. Can others still repro the bug?
I can still reproduce the problem.
https://hg.mozilla.org/mozilla-central/rev/127a78bac3f1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 ID:20150515030202
It seems to depend on the width of the browser.
Width < approx 970px : does not happen
970px < Width < 1280px(limited by my monitor) : reproduce the problem
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> There are some snapping changes that I saw when looking at the patch for bug
> 1103623

I don't see any snapping changes in:

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

What were you referring to?
Flags: needinfo?(jmuizelaar)
(In reply to Alice0775 White from comment #14)
> It seems to depend on the width of the browser.
> Width < approx 970px : does not happen
> 970px < Width < 1280px(limited by my monitor) : reproduce the problem

Thanks for investigating further, but unfortunately I still can't reproduce the issue. I've tried with local builds, Nightly builds from ftp.mozilla.org, various profiles I use, a clean profile, but no luck.

I'm still on OS X 10.9. Not sure if that could be why?

Not being able to reproduce I have to rely on the analysis that led to e497957b79b2 being blamed in comment 3 and close inspection of that changeset. I don't see anything wrong with it.
Attached patch artifactSplinter Review
Flags: needinfo?(jwatt)
Attachment #8606794 - Flags: review?(matt.woodrow)
Attachment #8606794 - Flags: review?(matt.woodrow) → review+
I don't think this patch is going to help by the way, since it's in selection code which doesn't seem relevant here.
Another glitch on windows7.

Steps to reproduce:
1. Visit http://www.bbc.co.uk/guides/zqjc4wx
2. Scroll down approx 800px
3. Enter Customize mode and Exit Customize mode
   

Actual results:
Black line apperes at the border of the section 2

Expected results:
No border should be.


And I got a same regression range
Huzza, I can repro with the dev pixel ratio changed to 1.

The change that is causing the issues is the nsDisplayBackgroundColor::Paint change. Simplified, this causes the bug:

diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp
--- a/layout/base/nsDisplayList.cpp
+++ b/layout/base/nsDisplayList.cpp
@@ -2914,22 +2914,23 @@ nsDisplayBackgroundColor::Paint(nsDispla
   gfxContext* ctx = aCtx->ThebesContext();
   nsRect borderBox = nsRect(ToReferenceFrame(), mFrame->GetSize());
 
-  gfxRect bounds =
-    nsLayoutUtils::RectToGfxRect(borderBox, mFrame->PresContext()->AppUnitsPerDevPixel());
+  Rect bounds = NSRectToSnappedRect(borderBox,
+                                    mFrame->PresContext()->AppUnitsPerDevPixel(),
+                                    *aCtx->GetDrawTarget());
 
   ctx->SetColor(mColor);
   ctx->NewPath();
-  ctx->Rectangle(bounds, true);
+  ctx->Rectangle(ThebesRect(bounds));
   ctx->Fill();
 }
 
Other than the precision of the types being used, these code paths should be logically equivalent, I think. :/
Flags: needinfo?(jmuizelaar)
Yup, basically nsDisplayBackgroundColor::Paint ends up filling a rect that is 1072 high with the old code, but 1071 high with the new.

Tracing this back, in this instance it's due to NSRectToSnappedRect storing the device pixel values in a Rect before passing that rect to MaybeSnapToDevicePixels. The 'y' and 'height' components of the rect while in the temporary double end up being -489.48333333333335 and 1071.9833333333333, which, if kept as doubles, would end up being summed in MaybeSnapToDevicePixels to 582.5 and Round()'ed to 583. In reality because the 'y' and 'height' components end up being stored in a Rect implicit rounding to float happens, so the 'y' and 'height' components of the rect taking the values -489.483337 and 1071.98328, which on being passed to MaybeSnapToDevicePixels sum to 582.499939 and Round() to 582.

Were just getting unlucky with rounding here, rather than this being a particular problem with lack of precision. I'm not sure what the real bug is, but perhaps one of the current calls to gfxContext::UserToDevicePixelSnapped needs to change to a MaybeSnapToDevicePixels call so that whatever paints/sizes the thing that is higher than the background that is being filled gets a matching size.
This is essentially a partial backout of the problematic part of e497957b79b2.
Attachment #8607011 - Flags: review?(matt.woodrow)
Comment on attachment 8607011 [details] [diff] [review]
patch nsDisplayBackgroundColor::Paint to use the higher precision code path for now

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

Can we just extract the snapping from gfxContext and use that rather than using gfxContext directly?

We're going to have to do something like that eventually to get rid of gfxContext, so might as well do it now?
That raises questions such as, where would the code live? Would it be gfxRect based, or a double based Rect equivalent in Moz2D land? How about the interaction with DrawTarget interfaces? For now I'm focused on other things, so I'd rather land this patch, uplift to branches, and spend time thinking about these questions later.
Comment on attachment 8607011 [details] [diff] [review]
patch nsDisplayBackgroundColor::Paint to use the higher precision code path for now

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

ok.
Attachment #8607011 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/3634dbfb36a8
https://hg.mozilla.org/mozilla-central/rev/bcaec6363e04
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Jonathan, if this is ready to go to aurora and beta, can you request uplift? Thanks.
Flags: needinfo?(jwatt)
Comment on attachment 8607011 [details] [diff] [review]
patch nsDisplayBackgroundColor::Paint to use the higher precision code path for now

Approval Request Comment
[Feature/regressing bug #]: bug 1103623
[User impact if declined]: Some 1px wide line rendering artifacts possible on some pages
[Describe test coverage new/current, TreeHerder]: Been on m-c for a while
[Risks and why]: low - reverting the code that caused the issue
[String/UUID change made/needed]: none
Flags: needinfo?(jwatt)
Attachment #8607011 - Flags: approval-mozilla-beta?
Attachment #8607011 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bugzmuiz)
Comment on attachment 8607011 [details] [diff] [review]
patch nsDisplayBackgroundColor::Paint to use the higher precision code path for now

Approved for uplift to aurora and beta. Fixes a regression, has been on m-c for a week.
Attachment #8607011 - Flags: approval-mozilla-beta?
Attachment #8607011 - Flags: approval-mozilla-beta+
Attachment #8607011 - Flags: approval-mozilla-aurora?
Attachment #8607011 - Flags: approval-mozilla-aurora+
Note that both parts in comment 29 need to land together (the latter should just be rolled into the former).
QA Whiteboard: [good first verify]
Reproduced the bug with Nightly 39.0a1 ( 2015-03-27 ) on Windows 7 64 Bit with the comment 0's instruction!

Verified as fixed with Firefox Beta 39.0b2 (Build ID: 20150601171003), Firefox Aurora 40.0a2 (Build ID: 20150602163057) and latest Nightly 41.0a1 (Build ID: 20150602055237) 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0

[bugday-20150603]
I have reproduced this bug with Nightly 39.0a1 ( 2015-03-27 ) with instruction from comment 0 and Linux x64.

Verified as fixed with latest Nightly 41.0a1 (2015-06-02) (Build ID: 20150602055237), Firefox Aurora 40.0a2 (2015-06-02) (Build ID: 20150602074125) and Firefox Beta 39.0b2 (Build ID: 20150601171003)

Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0

[bugday-20150603]
As per Comment 36 and Comment 37, Marking The bug as Verified!
QA Whiteboard: [good first verify] → [good first verify][bugday-20150603]
Whiteboard: [bugday-20150603]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.