1205630 - Adjacent tabs show a visual overlap when hovered over when a lwtheme is applied

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Attachment: old hover behavior

lwtheme I've been using to test this:
https://addons.mozilla.org/en-US/firefox/addon/firefox-dark-165861/

mozregression narrowed this regression down to bug 1166301. I'm on Windows 10, in case that matters.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: new hover behavior

Comment 2

[Guilherme Lima]

I see the same thing on Windows 7.

Comment 3

[Botond Ballo [:botond]]

Also repros on Linux. Caused by the "clip fixed background images at the layer level rather than the display item level" patch of bug 1166301.

Comment 4

[Botond Ballo [:botond]]

I think the fixed background in question is coming from here: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/browser/themes/linux/browser-lightweightTheme.css#24

Comment 6

[Daniel Holbert [:dholbert]]

See also bug 1206154 (screenshot: attachment 8663022 [details])

In that bug, I'm seeing issues without any need to hover, and without any adjacent tab. So, slightly different symptoms, but I'm assuming it's a dupe of this bug since the regression range points to bug 1166301, and it seems likely to be the same underlying rendering glitch.

Comment 7

[Markus Stange [:mstange]]

Attachment: not a testcase

I thought this might reproduce the problem, but it doesn't.

Comment 8

[Botond Ballo [:botond]]

Attachment: Display list dump, including inactive layer trees, before

Comment 9

[Botond Ballo [:botond]]

Attachment: Display list dump, including inactive layer trees, after

Comment 10

[Botond Ballo [:botond]]

Attachment: Display list dump, including inactive layer trees, after

Comment 11

[Botond Ballo [:botond]]

I've been investigating this pretty much all day, without much luck so far. I'm going to brain-dump what I've discovered.

Attached are before-and-after display list dumps, taken with the patch in bug 1105832 applied, which shows layer dumps for inactive layer trees (these are relevant because some of the fixed Background display items on the page are painted into inactive layers).

The dumps are of the parent process while viewing an empty about:home page in the only open tab, with this theme [1] (from bug 1206154) installed. The rendering glitch described in bug 1206154 can be observed in the "after" case, in the background of the tab.

There are three fixed Background display items on the page: one for the "tab-background-middle" element, and one each for the "::before" pseudo-elements of the "tab-background-start" and "tab-background-end" elements. The rendering glitch appears to be caused by incomplete or otherwise incorrect rendering of these latter two background display items. The offending horizontal line segments that constitute the glitch come from the top border of the toolbar containing the address bar, and are supposed to be covered up by these background, but are not.

These latter two Background items are inside SVGEffects items, which are created due to the "clip-path" property used on the "::before" pseudo-elements of the "tab-background-start" and "tab-background-end" elements. The clip-paths are what give the selected tab its rounded corners.

The SVGEffects display items cause their subtrees to be rendered into an inactive layer tree - this is why showing inactive layer trees in the layer dump is important.

The only differences between the before and after dumps are that in the "after" dump:

  - The Background items have empty clips after optimization.

  - The layers created for the Background items have larger visible and valid
    regions (due to the absence of the clip on the display item), and have a
    clip on the layer instead.

  - The layers created for the Background items are not marked "opaqueContent".

This is consistent with what my patch is doing (moving the clip from the display item to the layer). It's not immediately clear to me how these differences give rise to the rendering glitch.

Otherwise the display list dumps are identical. In both cases, the two Background items inside SVGEffects are rendered into an inactive layer, while the one that's not inside an SVGEffects is rendered into a retained layer.

As my next step in tracking down the rendering glitch, I tried getting a Moz2D recording as described here [2], but attempting to do so led to a startup crash here [3]. Perhaps not surprising as [2] describes Moz2D recordings as only working on Windows, although I have a distinct recollection of getting one on B2G. cc'ing Nical in case he has any insights into this.

I also tried using the Gecko profiler's layer texture visualization feature, but I couldn't figure out how to enable it 
- the checkbox "Layers and Textures" in the profiler settings dialog was disabled, and the instructions here [4] seem to only apply to B2G. cc'ing :BenWa in case he has any insights into that.

If anyone has any tips for how to get Moz2D recordings or layer texture visualization working, or has any ideas based on just the display list dumps, please let me know.

[1] https://addons.mozilla.org/en-US/firefox/addon/furfox-tail-twister/
[2] https://wiki.mozilla.org/Platform/GFX/Moz2D#Making_a_recording
[3] https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/gfx/thebes/gfxContext.cpp#213
[4] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler#Viewing_Textures

Comment 13

[Botond Ballo [:botond]]

As I haven't been able to get Moz2D recordings or the profiler's "Layers & Textures" feature working, I'm going to debug this using the Layout's "paint dumping" debugging feature. To do that, I first need to get that working properly in bug 1206915. (IIUC "Layers & Textures" is implemented using that, so it's not too surprising that that's also broken.)

Comment 14

[Botond Ballo [:botond]]

Ok, so after much hacking on the paint dumping code, I was able to produce before-and-after display list dumps in HTML format *with display item and layer textures*. I'm going to post them for reference. The dumps are pointing me in a direction to continue investigating: they tell me that one of the fixed background display items that was being painted in before, is not being painted any more.

Comment 15

[Botond Ballo [:botond]]

Attachment: HTML display list dump with textures, before

Comment 16

[Botond Ballo [:botond]]

Attachment: HTML display list dump with textures, after

Comment 17

[Botond Ballo [:botond]]

After a lot more debugging with rr, it looks like the problem is that we're taking the |clipIsEmpty = true| path [1] in the BasicLayerManager::PainerLayer() call for the inactive layer built for the fixed-background display item.

The incoming value of gfxContext::GetClipExtents() is (0, 0, 30, 31), while the layer's effective clip rect is (210, 0, 30, 31). Then gfxContext::SnappedRectangle() is called with the effective clip rect, and then gfxContext::Clip() which changes GetClipExtents() to (0, 0, 0, 0), resulting in an empty clip.

I didn't follow the implementations of gfxContext::SnappedRectangle() and gfxContext::Clip() in detail, but if what they effectively do is intersect the rect with the existing clip extents in the same coordinate space, then the result makes sense, and the problem is likely that the effective clip rect is in the wrong coordinate space.

[1] https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/gfx/layers/basic/BasicLayerManager.cpp?offset=0#966

Comment 18

[Markus Stange [:mstange]]

(In reply to Botond Ballo [:botond] from comment #17)
> After a lot more debugging with rr, it looks like the problem is that we're
> taking the |clipIsEmpty = true| path [1] in the
> BasicLayerManager::PainerLayer() call for the inactive layer built for the
> fixed-background display item.

Great work!

> and the problem is likely that the effective clip
> rect is in the wrong coordinate space.

I agree.

Comment 19

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #18)
> > and the problem is likely that the effective clip
> > rect is in the wrong coordinate space.
> 
> I agree.

Specifically, when setting the clip on the layer from the DisplayItemClip, we were failing to translate it by ContainerLayerParameters::mOffset the way other similar code (e.g. for OwnLayers) does.

Comment 20

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

https://reviewboard.mozilla.org/r/20439/#review18339

Comment 22

[Botond Ballo [:botond]]

On Markus' suggestion, I wrote a reftest for this. I had to make it a XUL reftest because I can't seem to be able to create the required conditions for triggering the bug in HTML.

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=mstange

Comment 24

[Botond Ballo [:botond]]

Oops, I forgot MozReview clobbers the first review request when I post a second one...

Comment 25

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=mstange

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

MozReview continues to be very confused... carrying r+ for fix.

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Updated test patch to add a comment explaining what is being tested.

Comment 29

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d541a83772cf

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=mstange

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Carrying r+.

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Updated to fix a typo in reftest.list.

Comment 34

[Botond Ballo [:botond]]

New Try push that includes this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e65f2343a1

Comment 35

[Markus Stange [:mstange]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

https://reviewboard.mozilla.org/r/20439/#review18405

::: layout/reftests/xul/inactive-fixed-bg-bug1205630-ref.html:26
(Diff revision 4)
> +  position: fixed;

same here

::: layout/reftests/xul/inactive-fixed-bg-bug1205630.xul:22
(Diff revision 4)
> +      position: fixed;

This might be confusing, better remove it. It's not necessary to reproduce the bug, is it?

Comment 36

[Markus Stange [:mstange]]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

https://reviewboard.mozilla.org/r/20475/#review18407

Comment 37

[Botond Ballo [:botond]]

Markus and I discussed this on IRC; in turns out the "position:fixed" is necessary because XUL follows slightly different rules than HTML. I also had to disable the test on certain B2G configurations, like other xul reftests.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9db06671fc0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca5a29b914f

Comment 39

[Phil Ringnalda (:philor)]

Both csets backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7d130726b975 - either the reftest doesn't work on Mac and Windows (https://treeherder.mozilla.org/logviewer.html#?job_id=14839800&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=14826689&repo=mozilla-inbound), or the reftest works but shows that the patch doesn't work.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Visual regression w/ lwthemes applied

Comment 41

[Botond Ballo [:botond]]

In both cases the differences are a pixel at the edge of the circle being an ever so slightly different shade (e.g. (236, 23, 43) vs. (236, 24, 44)), for a total of 1 pixel on Windows and 3 on Mac. I assume this is an artefact of layerization, which is happening now and wasn't before. Markus, is it appropriate to fuzz here?

Comment 42

[Markus Stange [:mstange]]

It would be better to use a rectangle clip-path so that we don't have to deal with anti-aliasing at all.
Or you could use "filter: brightness(100%)" instead of a clip-path.

Comment 43

[Botond Ballo [:botond]]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Comment 44

[Botond Ballo [:botond]]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=mstange

Comment 45

[Botond Ballo [:botond]]

Updated reftest to use a rectangular clip-path, as suggested in comment 42.

Let's see if it now passes on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb89febbe23

Comment 46

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #45)
> Let's see if it now passes on all platforms:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb89febbe23

Looks like it does. Landing.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50bb473484fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/bae7847cafbd

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50bb473484fb
https://hg.mozilla.org/mozilla-central/rev/bae7847cafbd

Comment 51

[Botond Ballo [:botond]]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1166301.

[User impact if declined]:
  With a lightweight theme applied, the edges of tabs are
  rendered incorrectly.

[Describe test coverage new/current, TreeHerder]:
  Tested locally with various themes, also has an automated
  test (reftest) in the tree.

[Risks and why]:
  Low risk, the fix is a small, localized change.

[String/UUID change made/needed]:
  None.

Comment 52

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression.

Comment 53

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8666242 [details]
MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange

Minor fix for recent regression, has reftests. Approved for uplift to aurora.

Comment 54

[Carsten Book [:Tomcat]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #53)
> Comment on attachment 8666242 [details]
> MozReview Request: Bug 1205630 - Translate a fixed background display item's
> clip rect correctly when setting it on the layer. r=mstange
> 
> Minor fix for recent regression, has reftests. Approved for uplift to aurora.

i guess you missed the approval flag for the test :) landed the test also as test-only :)

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea5e14f1c519
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2d2548a5697

Comment 57

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Tomcat!  Yes I missed it, glad you caught that and uplifted the tests too.

Comment 58

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Tests - already landed. Just tweaking the flag.