878023 - [OS X] Seam of background-images sometimes visible on hovered background tabs

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of missing sliver of the background-image

This seems like a platform bug but it needs a little investigation and possibly a reduced testcase (which I may have started). Note that background tabs don't use SVG nor pseudo-elements, rather 3 background-images on .tab-background.

At first glance it appears to be a rounding error related to flex as it appears and disappears depending on the window width. I can't seem to reproduce on Windows/Linux but perhaps it's less noticeable or needs a different configuration to hit this.

Possible workaround: perhaps border-image doesn't have this problem (which would require a single sprite sheet for the 3 images IIUC).

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Markus Stange [:mstange]]

Attachment: gdb session

notes for myself

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block us from landing on m-c.

Comment 4

[Markus Stange [:mstange]]

Attachment: patch

Tryserver is closed at the moment, but I think this should do the trick.
First I considered snapping the destrect unconditionally, but then repeated backgrounds would potentially have wildly varying repeat counts depending on the zoom level.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=504071#c22.

Comment 6

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> > The problem I fixed for Mac is that when we snap the fill rect, we also need
> > to adjust the dest rect (the rect filled by a single tile) to match, by
> > applying the same transform to the dest rect that we used to snap the fill
> > rect. If we don't do this, then for example if the dest rect and the fill
> > rect were originally the same, after snapping the dest rect might not
> > exactly fill the fill rect and we get a visible seam along the edge.
> 
> Actually, this shouldn't happen. It only happens on Mac because Mac doesn't
> do clamping properly :-(.

How does "clamping properly" avoid the seam if the dest rect isn't snapped?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The important thing is that the fill-rect is snapped. If we sample with EXTEND_CLAMP, and the source image is a solid color, we'll fill the fill-rect with that color no matter what.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Any updates on this Markus?

Comment 9

[Markus Stange [:mstange]]

I haven't done any more work on this. Does the problem still occur?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Yes, I was just able to reproduce it. Having read through the comments, I thought you'd narrowed into the reason with the test case, and we were just waiting on a patch.

Comment 11

[Markus Stange [:mstange]]

Oh, I see. Yes, we know what the problem is, but fixing it will take some work and the approach in the existing patch is wrong, so I deprioritized it because I haven't noticed the problem myself recently.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Ok, cool, good to know. Thanks!

Comment 14

[Florian Bender]

Just for the record (and the lazy): 
(In reply to Florian Bender from Bug 937622 comment #0)
> STR: 
> 1. Open exactly 8 tabs in a fresh UX window. 
> 2. Hover over tab 2 and 6 (from the left) and see the tab separator (or
> whatever that stripe is) visible beneath (or above) the close icon, nicely
> centered (other tabs are as expected). 
> 3. Open a 9th tab.
> 4. Hover over tab 2, 5, 8: They have the stripes now (tab 6 and others are
> as expected). 
> 5. Open 2 more tabs (making it 11 in total).
> 6. Hover over tab 2, 6, 10 (same as above, tab 5 and 8 look normal). 
> 7. Open a 12th tab. 
> 8. Now every second tab (starting with the first) has the stripe visible on
> hover, all other tabs appear normal. 
> 9. I haven't been able to reproduce this issue when tabs overflow. 
> 
> This happens very predictably so I assume there is some Math going rogue.

… STR on a MBA with 1440px device-width.

Comment 15

[Justin Dolske [:Dolske]]

I haven't seen this myself in a long time. Sounds like it's still happening, but is infrequent or hard to trigger.

Comment 16

[Florian Bender]

I can still easily reproduce with my STR from Comment 14.

Please note that the Firefox window should be 1440px wide for the STR given.

Comment 18

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Florian Bender from comment #16)
> I can still easily reproduce with my STR from Comment 14.
> 
> Please note that the Firefox window should be 1440px wide for the STR given.

My Fx window is only 819px wide and see the duplicate bug 964936 I've just reported.

Comment 19

[Michał Gołębiowski-Owczarek [:mgol]]

Also, I can reproduce it all the time, Firefox restart doesn't change anything.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Markus Stange [:mstange] from comment #11)
> Oh, I see. Yes, we know what the problem is, but fixing it will take some
> work and the approach in the existing patch is wrong, so I deprioritized it
> because I haven't noticed the problem myself recently.

What are the odds of this fix being portable to Aurora? And, if those chances are slim, is it possible to give a general idea of what's wrong so someone can try to poke this into working correctly?

Comment 21

[Florian Bender]

FYI, if I resize my window on Mac with 11 tabs opened,

- to 1440px width: see above,
- to 1439px width: the 3rd and the 9th …
- to 1438px width: the 6th …
- to 1437px width: no …
- to 1436px width: no …
- to 1435px width: every 2nd … (but barely visible)
- to 1434px width: every … (but barely visible)
- …

… unselected tab(s) show(s) the strip on hover.

Looks like an automatable task. ;-)

One new observeration: In some cases, the strip is very visible (even more than on attachment 756503 [details]), in others it's barely noticable.

Comment 22

[Markus Stange [:mstange]]

I've thought about several ways to fix this inside Graphics (i.e. to emulate ExtendMode::CLAMP in DrawTargetCG), but all solutions I've come up with are either only applicable to very narrow cases, or destroy performance.

Switching content rendering to Skia will hopefully fix this properly at some point, but we're not there yet.

For now I think we should just change the CSS so that we don't hit this bug.

Comment 23

[Markus Stange [:mstange]]

Attachment: CSS fix

This seems to work on OS X with both HiDPI on and off and with both lightweight themes on and off.

Comment 24

[:Gijs (out for now; he/him)]

Comment on attachment 8367272 [details] [diff] [review]
CSS fix

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

Matt should probably review this.

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8367272 [details] [diff] [review]
CSS fix

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

Don't forget to include "Australis" in the commit message.

Thanks, this works well. Do you have a bug on file for tracking the graphics fix for this? You may want to note the revision of m-c before this fix to use as a testcase there.

::: browser/themes/shared/tabs.inc.css
@@ -227,1 @@
>  .tabs-newtab-button:hover {

Do you know that this doesn't happen with the new tab button? I realize we don't have child elements to fix it easily but I'm wondering if you know that it could happen there based on what you know about the cause. I don't think I've ever personally seen it there.

@@ +237,5 @@
> +  background-repeat: repeat-x;
> +  background-size: auto 100%;
> +}
> +
> +/* normal tab border and gradient on hover */

Move this comment above the ruleset above?

Comment 26

[Markus Stange [:mstange]]

Attachment: CSS fix, for checkin, r=MattN

(In reply to Matthew N. [:MattN] from comment #25)
> Comment on attachment 8367272 [details] [diff] [review]
> CSS fix
> 
> Review of attachment 8367272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't forget to include "Australis" in the commit message.

Oops, thanks.

> Thanks, this works well. Do you have a bug on file for tracking the graphics
> fix for this? You may want to note the revision of m-c before this fix to
> use as a testcase there.

Not yet, I'll file one.

> ::: browser/themes/shared/tabs.inc.css
> @@ -227,1 @@
> >  .tabs-newtab-button:hover {
> 
> Do you know that this doesn't happen with the new tab button?

Yes, the new tab button is hardcoded to an integer width (66px), so it's never subject to the conditions that make this happen. It only happens with fractional widths.

It may be worthwhile to do apply the fix to the new tab button anyway, in order to make the code consistent between it and hovered tabs. But doing that is a bit more involved.

> @@ +237,5 @@
> > +  background-repeat: repeat-x;
> > +  background-size: auto 100%;
> > +}
> > +
> > +/* normal tab border and gradient on hover */
> 
> Move this comment above the ruleset above?

Done.

Comment 27

[Markus Stange [:mstange]]

Attachment: experimental new tab button patch

This would make the new tab button consistent. It makes almost no difference in code size. And this has only been tested on Mac, we'd probably need some fixes to the Windows and Linux specific browser.css, too. I'm only uploading the patch here so that we can come back to it if we want to.

mstange-17521:mozilla mstange$ hg tip -p | diffstat
 osx/browser.css     |   23 ++++++-----------------
 shared/tabs.inc.css |   43 ++++++++++++++++++++++++++++---------------
 2 files changed, 34 insertions(+), 32 deletions(-)

Comment 28

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #26)
> > Thanks, this works well. Do you have a bug on file for tracking the graphics
> > fix for this? You may want to note the revision of m-c before this fix to
> > use as a testcase there.
> 
> Not yet, I'll file one.

Filed bug 967532.

Comment 29

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbf61b9d523

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bbf61b9d523

Comment 31

[Dão Gottwald [:dao]]

Markus, can you back this out for having caused bug 968201? Unless somebody has a better idea, it seems like we should set our hope on bug 967532.

Comment 32

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #31)
> Markus, can you back this out for having caused bug 968201?

And likely also bug 969934 and bug 970816. :-(

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Backing this out for now:

https://hg.mozilla.org/integration/fx-team/rev/6dc2808e3624

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/6dc2808e3624

Comment 35

[Flore Allemandou [:flore]]

I noticed this bug on mac. It's easier to see with a dark theme because of the contrast. 
For example: https://addons.mozilla.org/fr/firefox/addon/blue-starry-sky/ (or any dark lightweight theme).

Comment 36

[Justin Dolske [:Dolske]]

Marcus, any updates here? Or is this unlikely to make 29?

Comment 39

[Mike Conley (:mconley) (:⚙️)]

I know BenWa had mentioned seeing this, so Cc'ing.

Comment 41

[Dan]

Attachment: CSS temp workaround

This CSS, pulled from the backed out patch, will fix the issue on OS X for anyone who, like me, is bothered by this. Had to add a slight "clip-path: none" tweak to get the middle background sized correctly.

Comment 45

[mitch deoudes]

I'm seeing this on x64 Windows 7, with FF 30.0 that I just received.  Same deal - it dis/reappears when scaling the window.  Will add an attachment with pic.

I notice this & all the dupe bugs are OS X.  Do I need to file a new one for Windows?

Comment 46

[mitch deoudes]

Attachment: Image of bug on Windows 7.

Image of the bug on Windows 7, FF 30.0

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

(In reply to mitch deoudes from comment #45)
> I'm seeing this on x64 Windows 7, with FF 30.0 that I just received.  Same
> deal - it dis/reappears when scaling the window.  Will add an attachment
> with pic.
> 
> I notice this & all the dupe bugs are OS X.  Do I need to file a new one for
> Windows?

Can you see the red vertical line in the testcase https://bug878023.bugzilla.mozilla.org/attachment.cgi?id=761905 ?

Comment 48

[mitch deoudes]

> Can you see the red vertical line in the testcase
> https://bug878023.bugzilla.mozilla.org/attachment.cgi?id=761905 ?

No, I can't - I see only a horizontal red line, and the test looks the same in Chrome and FF.

And unfortunately, I've been tweaking my FF settings since I got the new version a couple hours ago, and the tab seam is no longer visible.  I attempted to disable all my changes and see if I could bring it back, but no luck.  I then tried it with a fresh profile, and no luck.  

However, the test case has only *ever* produced a horizontal red line for me: I tried it back when I first saw the bug, and initially assumed the horizontal line represented the bug, since the test didn't specify.

So unfortunately, I can only contribute the following:

  - The artifact was definitely there when the update got pushed to me, and looked just like the first attachment.  (Hence my finding this bug.)
  - It doesn't (now) appear under a fresh profile, or under my altered profile.
  - The test case appears the same now as when I had the bug - in both cases there's only a horizontal line.

To me that says that it was some interaction between my profile and FF30, but also that what you're testing for above won't catch it on Windows.  Which I guess is either helpful, or counter-helpful.  Sorry about that.

Comment 50

[Aron Gutiérrez]

mmm... I don't use Firefox any more because this ugly issue. When I was child, when the graphics interface was so ugly, all works rightly. This ugly glitch has been present for months!!! The Linux version is OK, the Windows version is OK, what about the MAC version? thadhaaa! it has a glitch like a beta version and during moths!. What about quality? WTF...

Comment 51

[Markus Stange [:mstange]]

Is this still happening? I don't see it in the tab strip any more, and looking at the testcase (attachment 761905 [details]) on 10.11, I can't even reproduce the right line at the right any more. Maybe Apple has fixed this bug in CoreGraphics.

Comment 52

[Stephen Horlander [:shorlander]]

(In reply to Markus Stange [:mstange] from comment #51)
> Is this still happening? I don't see it in the tab strip any more, and
> looking at the testcase (attachment 761905 [details]) on 10.11, I can't even
> reproduce the right line at the right any more. Maybe Apple has fixed this
> bug in CoreGraphics.

I feel like something has changed on our end because I can reproduce this with varying degrees of severity up until 39. I tested a few releases:

Ver.         Glitch
-------------------
30       =   Yes
34       =   Yes
35       =   Yes
36       =   Yes
38.0.6   =   Yes
39.0.3   =   No
40.0.3   =   No
44.0.2   =   No
44.0.2   =   No
47.0a1   =   No

Comment 53

[Markus Stange [:mstange]]

Cool, thanks for testing! Should we resolve it as WFM then?

Comment 54

[Stephen Horlander [:shorlander]]

(In reply to Markus Stange [:mstange] from comment #53)
> Cool, thanks for testing! Should we resolve it as WFM then?

WFM ;) 

Although it might be nice to have some verification from others. Should we also try and understand why it's fixed?

Comment 55

[Stefan Plewako [:stef]]

Attachment: 48a1.20160314030215.png

Comment 56

[Markus Stange [:mstange]]

Stefan, can you test whether this still happens on 48 / 49? The switch to Skia for rendering (bug 1207332) may have fixed this completely.

Comment 57

[Stefan Plewako [:stef]]

I see red on https://bugzilla.mozilla.org/attachment.cgi?id=761905 for sure (nightly, 50), didn't reproduce in UI yet.

Comment 58

[Markus Stange [:mstange]]

Just the red dot in the middle, or also a red line on the right? The red line is what showed the bug, and I don't see a red line any more.

Comment 59

[Stefan Plewako [:stef]]

Attachment: line or dot?

Comment 60

[Markus Stange [:mstange]]

Dot. Sorry for the misleading testcase - there's a transparent hole in the middle of the image that gets stretched horizontally, and the only purpose of the hole is to disable the "image is just one color" optimization that we have in the image drawing code.
The actual bug would be indicated by a vertical red line along the right edge of the green rectangle.

Ok, so this seems fixed.