Closed
Bug 1148418
Opened 9 years ago
Closed 9 years ago
Scrolling leaves horizontal artifacts on page
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
mozilla41
People
(Reporter: stuartmemo, Assigned: jwatt)
References
Details
(Keywords: regression, Whiteboard: [bugday-20150603])
Attachments
(5 files)
17.46 KB,
image/png
|
Details | |
222.96 KB,
image/png
|
Details | |
1.02 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
Details | Diff | Splinter Review | |
1.58 KB,
patch
|
mattwoodrow
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
status-firefox-esr31:
--- → unaffected
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Comment 2•9 years ago
|
||
regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=90d9478a29e2&tochange=e497957b79b2
Component: Untriaged → Graphics
Flags: needinfo?(jwatt)
Product: Firefox → Core
Comment 3•9 years ago
|
||
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
Comment 4•9 years ago
|
||
There are some snapping changes that I saw when looking at the patch for bug 1103623
Updated•9 years ago
|
Assignee: nobody → jwatt
Comment 5•9 years ago
|
||
[Tracking Requested - why for this release]: This is a content drawing regression that should be pretty easy to fix.
tracking-firefox38:
--- → ?
tracking-firefox39:
--- → ?
Comment 7•9 years ago
|
||
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)
Updated•9 years ago
|
Comment 9•9 years ago
|
||
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)
Comment 10•9 years ago
|
||
This is too late for 38 but we will take a patch for 39.
Updated•9 years ago
|
Flags: needinfo?(jwatt)
Needinfo ping.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 12•9 years ago
|
||
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?
Comment 13•9 years ago
|
||
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
Updated•9 years ago
|
status-firefox40:
--- → ?
status-firefox41:
--- → affected
Comment 14•9 years ago
|
||
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
Assignee | ||
Comment 15•9 years ago
|
||
(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)
Assignee | ||
Comment 16•9 years ago
|
||
(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.
Assignee | ||
Comment 17•9 years ago
|
||
Flags: needinfo?(jwatt)
Attachment #8606794 -
Flags: review?(matt.woodrow)
Updated•9 years ago
|
Attachment #8606794 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 18•9 years ago
|
||
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.
Comment 19•9 years ago
|
||
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
Assignee | ||
Comment 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
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.
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
This is essentially a partial backout of the problematic part of e497957b79b2.
Attachment #8607011 -
Flags: review?(matt.woodrow)
Comment 24•9 years ago
|
||
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?
Assignee | ||
Comment 25•9 years ago
|
||
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 26•9 years ago
|
||
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+
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3634dbfb36a8 https://hg.mozilla.org/mozilla-central/rev/bcaec6363e04
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 30•9 years ago
|
||
Jonathan, if this is ready to go to aurora and beta, can you request uplift? Thanks.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 31•9 years ago
|
||
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?
Updated•9 years ago
|
Flags: needinfo?(bugzmuiz)
Comment 32•9 years ago
|
||
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+
Assignee | ||
Comment 33•9 years ago
|
||
Note that both parts in comment 29 need to land together (the latter should just be rolled into the former).
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Comment 36•9 years ago
|
||
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]
Comment 37•9 years ago
|
||
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]
Comment 38•9 years ago
|
||
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]
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•