Background image linear Gradient shows lighter color line at bottom of element (regression)

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: yoasif, Assigned: eflores)

Tracking

({regression})

51 Branch
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments)

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

Steps to reproduce:

I created a button to use in an internal webapp, and found that my buttons rendered oddly in my primary browser (Firefox nightly).

Test case available here:

http://codepen.io/anon/pen/VjGxrj

I used mozregression and I found that the issue was introduced in bug 1274624.


Actual results:

The button appears with a lighter colored line at the bottom of the linear gradient. Increasing or decreasing the text size in my browser seems to "fix" it. 


Expected results:

The button shouldn't show a lighter colored line at the bottom of the gradient. 

A screenshot is attached of the good and bad cases.
With non-retina display, it doesn't happen with default zoom level, but doing "zoom in" several times causes the lighter line on older builds.
maybe you're using retina display?
Component: Untriaged → Graphics
Product: Firefox → Core
See Also: → 1274624
Edwin, what do you think for this regression?
Flags: needinfo?(edwin)
Whiteboard: [gfx-noted]
Assignee: nobody → edwin
Tooru, yes, I am on a retina display (MacBook Pro (Retina, 15-inch, Mid 2015).
Posted patch 1291528.patchSplinter Review
Whoops; was using the wrong length for the gradient line.

This patch uses the aSrc size instead of srcSize (which is derived from aIntrinsicSize).
Flags: needinfo?(edwin)
Attachment #8778229 - Flags: review?(mstange)
Here is a reftest, but it fails quite hard.

The repeat-optimised path paints the gradient from #00f to #f00, but the old path goes from #0600f8 to #f90005.

Reading the spec, it seems that the new behaviour is correct. Should we just fuzz the difference?
Attachment #8778230 - Flags: feedback?(mstange)
Comment on attachment 8778229 [details] [diff] [review]
1291528.patch

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

Would using aDest be wrong? Intuitively that seems like the better fit. But I'm not quite sure about the meaning of aSrc to be honest.
If using aDest makes the new path render the same (slightly wrong) way as the old path (and there are no other reasons against using aDest), then let's do that. I don't think the spec defines what happens in the face of pixel snapping. It defines what to render in the ideal infinite resolution case, but as soon as you reduce the resolution to something finite, it's really up to you how to deal with that.
> Would using aDest be wrong? Intuitively that seems like the better fit. But
> I'm not quite sure about the meaning of aSrc to be honest.

They're essentially equivalent. They are the same rectangle with the same contents: we construct the gradient in |aSrc| space, and then translate/scale the gfxPattern matrix so that the contents of |aSrc| cover exactly |aDest| in device space.

Using aDest wouldn't make a difference and wouldn't fit very well with how the method is structured.
Comment on attachment 8778229 [details] [diff] [review]
1291528.patch

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

(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> and then
> translate/scale the gfxPattern matrix so that the contents of |aSrc| cover
> exactly |aDest| in device space.

Oh ok, great.
Attachment #8778229 - Flags: review?(mstange) → review+
Comment on attachment 8778230 [details] [diff] [review]
1291528-test.patch

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

Yeah let's fuzz it.
Attachment #8778230 - Flags: feedback?(mstange) → feedback+
Pushed by eflores@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e95a2eb907
Add reftest to ensure that linear gradient drawing paths agree - r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/adc1960091eb
Fix gradient scaling in nsCSSRendering::PaintGradient - r=mstange
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4794e6af5f35
Backed out changeset adc1960091eb 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d90faf1d38fc
Backed out changeset 40e95a2eb907 for many animation related crashes. r=backout
Apparently aSrc can be empty. That doesn't make sense, but I'm reluctant to simply ignore it.

This patch just makes us use the slow path in this case.
Flags: needinfo?(edwin)
Attachment #8781501 - Flags: review?(mstange)
Attachment #8781501 - Flags: review?(mstange) → review+
Pushed by eflores@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d8eb3f9527
Add reftest to ensure that linear gradient drawing paths agree - r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6e18982d37
Fix gradient scaling in nsCSSRendering::PaintGradient - r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef0c7958e58
Don't use fast path in nsCSSRendering::PaintGradient if source rect is empty - r=mstange
https://hg.mozilla.org/mozilla-central/rev/f1d8eb3f9527
https://hg.mozilla.org/mozilla-central/rev/4c6e18982d37
https://hg.mozilla.org/mozilla-central/rev/9ef0c7958e58
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8778229 [details] [diff] [review]
1291528.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1274624.
[User impact if declined]: Incorrect gradient rendering.
[Describe test coverage new/current, TreeHerder]: Has been on m-c for about a month.
[Risks and why]: Very low.
[String/UUID change made/needed]: None.
Attachment #8778229 - Flags: approval-mozilla-aurora?
Comment on attachment 8778230 [details] [diff] [review]
1291528-test.patch

As above.
Attachment #8778230 - Flags: approval-mozilla-aurora?
Comment on attachment 8781501 [details] [diff] [review]
1291528-bustage.patch

As above.
Attachment #8781501 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1280288
Keywords: regression
Comment on attachment 8778229 [details] [diff] [review]
1291528.patch

Fixes a regression, has been on Nightly for a few weeks, Aurora50+
Attachment #8778229 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8778230 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8781501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hello Asif, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(yoasif)
Ritu, looks good in the nightly I'm on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:51.0) Gecko/20100101 Firefox/51.0

Thanks!
Depends on: 1315113
Clearing NI.
Trying to clear NI again.
Flags: needinfo?(yoasif)
You need to log in before you can comment on or make changes to this bug.