Closed Bug 1315113 Opened 5 years ago Closed 5 years ago

gradient border-image gets a mini-me (or extra colorful stripes) on retina or with full-page-zoom

Categories

(Core :: Graphics, defect)

50 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox49 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: manishearth, Assigned: mstange)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(7 files)

Attached file testcase.html
Discovered in https://github.com/servo/servo/pull/13989#issuecomment-258251834

The attached testcase shows a mini version of the border image gradient at the bottom of the border in Firefox. Interestingly, this doesn't happen in Stylo.

It seems to only happen on retina displays. If layout.css.devPixelsPerPx is set to 1, it disappears.
Attached image Firefox screenshot
Attached image Chrome screenshot
Attachment #8807353 - Attachment mime type: text/plain → text/html
Is this a regression w.r.t. Firefox 49?

I can reproduce a version of this glitch locally in Nightly on (normal-resolution) Linux, if I zoom in or out on the testcase using full-page-zoom. But I cannot reproduce on Release.
Flags: needinfo?(manishearth)
Yeah, this is a regression.
Flags: needinfo?(manishearth)
[Tracking Requested - why for this release]: Regression in Firefox 50, w.r.t. 49.

Tentatively, it looks like the full-page-zoom-in issues that I see on this testcase (extra solid-red stripes) started with this push:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2355c90ee8c8c9e62084a23e81f4e2bf9677f2b2&tochange=ac598dfc00b28c3217e0354ccc88bcb9de1bb304
"Edwin Flores — Bug 1291528 - Don't use fast path in nsCSSRendering::PaintGradient if source rect is empty - r=mstange
"Edwin Flores — Bug 1291528 - Fix gradient scaling in nsCSSRendering::PaintGradient - r=mstange"

I also see a glitch (extra solid-green stripes) when zooming *out* from the attached testcase.  I thought that was the same glitch, but it turns out that regressed a while earlier:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=54e8922be9ef98d9ee0ade82480d2876ae9bd1df&tochange=0d58d0626631c57f3312331bd8ef2a1469e0a590
"Edwin Flores — Bug 1274624 - Speed up tiled axis-aligned linear gradient painting - r=mstange"

I suspect these two separate regressions are really versions of the same bug, though. (The later change presumably just made us start hitting it in more cases, or something.)  Tagging as a regression with both bugs in the "blocks" field for now.  Also, both of those changes are in Firefox 50, so it looks like we're about to ship this as a regression in that release (darn).
Blocks: 1274624, 1291528
Flags: needinfo?(edwin)
Summary: border-image gets a mini-me in some cases (on retina) → gradient border-image gets a mini-me (or extra colorful stripes) on retina or with full-page-zoom
Here's a screencast of how this bug manifests for me on Linux.

The first half shows zooming out, which is the older regression (from bug 1274624).  The second half shows zooming in, which is the newer regression (from bug 1291528).
(Requesting tracking for Firefox 51, because we probably won't have a chance to fix this for 50 [which gets released in a couple days].  But we might get a chance to fix it in 51, in which case we'd only have shipped it for 1 release.)
Track 51+ as regression.
Markus, Edwin is on PTO this week - anything you can suggest we do here?
Flags: needinfo?(mstange)
Keywords: regression
Whiteboard: [gfx-noted]
Version: unspecified → 50 Branch
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
lineStart and lineEnd are computed by ComputeLinearGradientLine, which computes them based on srcSize. aSrc exists in a different coordinate space. Mixing the two is not a good idea.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf305a8e4bd662348780b772d58225b604586a4b
Flags: needinfo?(edwin)
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

https://reviewboard.mozilla.org/r/91204/#review91010

::: layout/reftests/bugs/1315113-1.html:2
(Diff revision 1)
> +<!DOCTYPE html>
> +<html reftest-zoom="2">

Perhaps it'd be worth including another reftest with reftest-zoom="0.5"?  (zooming out, rather than zooming in)

As noted in comment 5 (second half), the initial regression here *only* manifested for zoom-out operations -- so the patch's current reftest (witha  2x zoom) wouldn't have caught that regression.
Can we consider it for a ridealong/50.1?  It's a long time until 51 ships.
Flags: needinfo?(rkothari)
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

https://reviewboard.mozilla.org/r/91204/#review91018
Attachment #8808383 - Flags: review?(matt.woodrow) → review+
(Per IRC, it looks like you forgot to 'hg add' the files for the second reftest (1315113-2.html and 1315113-2-ref.html).  Do be sure to add those before using autoland here.)
Flags: needinfo?(mstange)
It looks like the patch regresses bug 1291528. I need to look into this some more.
(In reply to Milan Sreckovic [:milan] from comment #15)
> Can we consider it for a ridealong/50.1?  It's a long time until 51 ships.

Yes. It was my bad to wontfix this one. Leaving it tracked and affected for 50 so we can uplift a safe fix in say 50.1.0
Flags: needinfo?(rkothari)
The previous patch was bad, it was basically disabling the optimization if aSrc was a subrect of aIntrinsicSize.
This new patch just converts aSrc into the right coordinate space and otherwise keeps the behavior of the current code.
Flags: needinfo?(mstange)
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

https://reviewboard.mozilla.org/r/91204/#review91802
Attachment #8808383 - Flags: review?(mstange)
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

https://reviewboard.mozilla.org/r/91204/#review91804
Attachment #8808383 - Flags: review?(mstange)
Matt, could you have another look at this? The patch changed completely, so I don't feel comfortable just carrying forward the review, but I can't figure out how to re-request review with mozreview.
Flags: needinfo?(matt.woodrow)
Looks great!
Flags: needinfo?(matt.woodrow)
Is this worth uplifting for any potential 50 dot releases and/or 50.1?
Flags: needinfo?(mstange)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/0951c988b4a3
Fix coordinate space confusion in gradient rendering code. r=mattwoodrow
Yes, I think it's worth it - very low risk and moderately high reward.

I wanted to land this patch on Friday but didn't realize that autoland had failed to rebase it; that's why I only landed it today.
Flags: needinfo?(mstange)
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

Approval Request Comment
[Feature/regressing bug #]: bug 1274624
[User impact if declined]: wrong rendering on some web pages
[Describe test coverage new/current, TreeHerder]: pretty good for gradients in general, and this patch adds a test for this specific issue
[Risks and why]: very low
[String/UUID change made/needed]: none
Attachment #8808383 - Flags: approval-mozilla-release?
Attachment #8808383 - Flags: approval-mozilla-beta?
Attachment #8808383 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0951c988b4a3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

This patch fixes a regression. Beta51+ and Aurora52+. Should be in 51 beta 2.
Attachment #8808383 - Flags: approval-mozilla-beta?
Attachment #8808383 - Flags: approval-mozilla-beta+
Attachment #8808383 - Flags: approval-mozilla-aurora?
Attachment #8808383 - Flags: approval-mozilla-aurora+
Hi Manish, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(manishearth)
It doesn't seem to be completely fixed, the attached testcase now has a line going through it in Firefox (Build of tip, https://hg.mozilla.org/mozilla-central/rev/51750761f2c6).

Not sure if that's due to border images or something else.
Flags: needinfo?(manishearth)
Attachment #8811531 - Attachment description: Chrome and Firefox side by side → Chrome and Firefox side by side (Chrome in foreground)
This line is caused by bug 1316422 but we can work around it. I wouldn't uplift that workaround to release though.
Okay, if that's a separate bug, this seems fixed then.
Status: RESOLVED → VERIFIED
has problems to apply to beta, seems this need a rebase.
Flags: needinfo?(mstange)
Attached patch patch for betaSplinter Review
Flags: needinfo?(mstange)
Comment on attachment 8808383 [details]
Bug 1315113 - Fix coordinate space confusion in gradient rendering code.

Patch seems low risk, was verified by bug opener, let's take it in 50.1.0
Attachment #8808383 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.