902591 - [Mac] gradients broken in some cases with OMTC enabled

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[blissdev]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130807040232

Steps to reproduce:

background-image gradient on input button or submit with border

Example:
http://codepen.io/blissdev/pen/uIkaz (add class fixed to button to remove border)

Shots of it working in other browsers (and previous FF versions <25):
http://www.browserstack.com/screenshots/4a67b72522c719966261f9a79b84fb3a9c3c2c2a


Actual results:

background is flat without gradient


Expected results:

background has gradient

Comment 1

[Loic]

It seems to be OK with FF26 on Win 7: http://i.imgur.com/j3xMvdF.jpg
Maybe issue specific to Firefox for OSX?

Did it use to work with previous versions of Firefox?

Could you test with HWA disabled, please.
http://support.mozilla.org/en-US/kb/forum-response-disable-hardware-acceleration

Comment 2

[blissdev]

I noticed this about 1-2 weeks ago starting on FF25. I turned off HWA and it is still happening.

Also, I updated the test case with a more obvious gradient.

Could it be related to this bug? https://bugzilla.mozilla.org/show_bug.cgi?id=895135

Comment 3

[philippe (part-time)]

I've noticed this as well - I first saw it with the 08/04 nightly (25a) and now with the 08/06 (26a) build.

Any <button>, <input type="button">, <input type="submit"> that has a border and a gradient fails to paint the gradient.

I checked the 24.0a2 (2013-08-05) build, and that works correctly.

Now here is a weird thing: I have, on my local server, a very long page that contains multiple buttons. Sometimes - *sometimes* - the gradient paints correctly, but on some buttons the gradient is missing completely, on others the gradient is only painted halfway. Scrolling up and down that page and I see the gradients painting changing.

Comment 4

[philippe (part-time)]

Attachment: test case

Comment 5

[philippe (part-time)]

regression window seems to be:

works
2013-07-22-17-26-07-mozilla-central
http://hg.mozilla.org/mozilla-central/rev/e3c19a339b36

fails
2013-07-23-03-02-05-mozilla-central
http://hg.mozilla.org/mozilla-central/rev/5ceea82a79c7

Comment 6

[Loic]

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e3c19a339b36&tochange=5ceea82a79c7

Comment 7

[Loic]

It could be bug 893298, no?

Comment 8

[philippe (part-time)]

It could be, but why doesn't it affect Windows builds? CC'ing the patch author of bug 893298. Hmm, and confirming this as a bug.

Comment 9

[blissdev]

I also noticed an extremely similar issue with anchors styled with gradients, wherein making one of several changes (removing border radius, removing border, reverting to inline display) fix it. But perhaps it should be a separate bug, any thoughts?

http://codepen.io/blissdev/pen/oyhFL

Comment 10

[philippe (part-time)]

(In reply to blissdev from comment #9)

I can't reproduce that with my test cases. But then I noticed that codepen use -moz-linear-gradient() instead of linear-gradient - which has been supported by Firefox for quite a while. If, in your test case, I change the -moz-linear-gradient to the unprefixed one in the inspector, your test case displays correctly.

Comment 11

[blissdev]

(In reply to philippe (part-time) from comment #10)

Ah, so that may just be a SASS/Compass bug, where they need to be prioritized in a different order. Thanks.

Comment 12

[betravis]

The bug is still in place even after reverting the changes from bug 893298, and its predecessor 835873. Were there other patches that touched button layout or painting?

Comment 13

[philippe (part-time)]

(In reply to betravis from comment #12)

Thanks for checking.

Reviewing the pushlog in comment 6, I noticed this:

b9efc80b5c82	Jeff Muizelaar — Bug 896489. Enable Azure CoreGraphics for content. r=joe This only has an impact on OSX 10.7 and 10.8 because it's only hooked up through OMTC right now.

And sure enough, testing on 10.6, my (attached) test case appears to work…

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does flipping the "gfx.content.azure.enabled" pref change the behavior?

Comment 15

[philippe (part-time)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #14)
> Does flipping the "gfx.content.azure.enabled" pref change the behaviour?


Apparently not, even after restarting the browser.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

So disabling off main thread compositing fixes this for me. Disabling azure content does not.

Comment 18

[philippe (part-time)]

Duplicated bug 905246 has a test case where the combination of 'width' + 'height' also triggers this bug.

Comment 20

[u279076]

Nominating for tracking since this is a regression in Firefox 25.

Comment 21

[Alex Keybl [:akeybl]]

Unless somebody can prove this is a major edge case, agreed this should be tracking for 25.

Comment 22

[Matt Woodrow (:mattwoodrow)]

I can confirm the regression range from comment 6, and when testing with the first broken nightly (2013-07-23-03-02-05-mozilla-central), changing gfx.content.azure.enabled to false fixes the issue.

Just rebuilding m-c to see if the pref change has the same effect there, as this isn't what Jeff saw initially.

I wonder if it's possible that something else changed after the initial regression range that made this more confusing in some way.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Ah. We don't actually check if Azure content is enabled for mac, we just use it regardless. So flipping the enabled pref has no effect, and the only way to disable azure content is to also disable OMTC.

This changed here: http://hg.mozilla.org/mozilla-central/diff/0428fd3a0d3a/gfx/layers/client/ContentClient.cpp

After the initial regression, but before comments 12+.

Comment 24

[Nick Cameron [:nrc]]

Attachment: revert changes to ContentClientIncremental

Comment 25

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=66e297207e70

Comment 26

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae388d62a87

Comment 27

[Nick Cameron [:nrc]]

Whoops! Wrong patch - backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b54782ad3ff

Comment 28

[Nick Cameron [:nrc]]

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

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4c63d61ecba

Comment 30

[Jeff Muizelaar [:jrmuizel]]

As I understand this is not yet fixed right?

Comment 31

[Alex Keybl [:akeybl]]

(In reply to Jeff Muizelaar [:jrmuizel] on PTO till October 21 from comment #30)
> As I understand this is not yet fixed right?

Is that true nrc? Do you expect to have a fix landed this week?

Comment 32

[Nick Cameron [:nrc]]

(In reply to Alex Keybl [:akeybl] from comment #31)
> (In reply to Jeff Muizelaar [:jrmuizel] on PTO till October 21 from comment
> #30)
> > As I understand this is not yet fixed right?
> 
> Is that true nrc? Do you expect to have a fix landed this week?

My patch is landed and hasn't been backed out. If that hasn't fixed it then we need to address that. I'm travelling and don't have access to a mac - Matt or Marcus, could you investigate please?

Comment 33

[Benoit Girard (:BenWa)]

Verified still broken.

Comment 34

[philippe (part-time)]

Still an issue with 27.0a1 (2013-10-08)

Comment 35

[Markus Stange [:mstange]]

I'll take a look.

Comment 36

[Markus Stange [:mstange]]

Attachment: repeating-gradients-cg

The code from bug 893977 that constructs a repeating linear gradient doesn't work in all cases (e.g. for 0,0 -> 0,100 with fill rect 50,50,100,100)

Luckily, roc has already written something that works in bug 508730, so I'm taking his code from the Quartz cairo backend and reusing it here.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

I wrote something similar in bug 913407. I'm on PTO right now, can mattwoodrow and you figure out the right thing to do?

Comment 38

[Markus Stange [:mstange]]

Comment on attachment 814622 [details] [diff] [review]
repeating-gradients-cg

Sure. I prefer this version because we've been using it for almost 4 years now and because it doesn't use trigonometric functions. Matt, what do you think?

Comment 39

[Markus Stange [:mstange]]

Oh, I had misread Jeff's patch in bug 913407; it doesn't actually use trigonometric functions. It does use sqrt() (in hypot()), though, and this one does not.

Comment 40

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 814622 [details] [diff] [review]
repeating-gradients-cg

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

This approach makes a lot more sense to me, and it seems safer since it's what we were using previously.

A high level explaining what we're doing would be really good though.

Something like: "Repeat the gradient line such that lines extended perpendicular to the gradient line at both start and end would completely enclose the drawing extents"

Comment 41

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/165c6496cb41

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/165c6496cb41

Comment 43

[u279076]

Ioana, please have someone on your team confirm this is now working in the latest Nightly.

Comment 44

[metavida]

Am I reading this ticket correctly that this bug, which affects FF25, will not be completely fixed until FF27? This bug affects so many websites that it seems the fix should be backported much more aggressively.

A very very quick sampling of websites currently affected by this bug:
Hipmunk:
* FF25: https://www.monosnap.com/image/4o7TQklJLBKrWvXE1v6m04kF3
* Chrome30: https://www.monosnap.com/image/bQMTyJARJLsKJtJzuI9NDrSSH
Firefox Release Notes:
* FF25: https://www.monosnap.com/image/iIbY8pSVbBBQidWJm9dKwvh9h
* Chrome30: https://www.monosnap.com/image/JKSLhY9Yfz4pg77dEHnCpUyAl

Comment 45

[Mikhail Kalashnik]

Agree with metavida, it should be fixed in 25 before it becomes stable and would be released.

Comment 46

[blissdev]

Maybe it hasn't been merged in yet, but this is *not* fixed for me in today's nightly.

Comment 47

[Mihai Morar, (:MihaiMorar)]

I had reproduced this issue on Nightly 26 (2013-08-08) on Mac OS 10.8.4 but I can confirm the fix is Verified on Latest Nightly 27.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0
BuildID: 20131013030202

Comment 48

[Alex Keybl [:akeybl]]

mstrange/matt, can you all please nominate for uplift to Aurora/Beta using the patch flags? Thanks!

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 814622 [details] [diff] [review]
repeating-gradients-cg

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 896489
User impact if declined: wrong gradient rendering on many websites
Testing completed (on m-c, etc.): 1 day on m-c in this form, but over 3 years in similar form
Risk to taking this patch (and alternatives if risky): low due to similarity to original code. alternative: disable content Azure on Mac (i.e. back out bug 896489)
String or IDL/UUID changes made by this patch: none

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1afcd8a4d5b
https://hg.mozilla.org/releases/mozilla-beta/rev/e5848db83ea7

Comment 51

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on Latest Aurora 26 and FF 25b8 too using Mac OSX 8.4

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
BuildID: 20131015052812

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
BuildID: 20131016004005