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

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: RyanVM, Assigned: botond)

Tracking

({regression})

Trunk
mozilla44
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ fixed, firefox44+ fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8662347 [details]
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.
Flags: needinfo?(botond)
(Reporter)

Comment 1

3 years ago
Created attachment 8662348 [details]
new hover behavior

Comment 2

3 years ago
I see the same thing on Windows 7.
Assignee: nobody → botond
Whiteboard: [gfx-noted]
(Assignee)

Comment 3

3 years ago
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.
Flags: needinfo?(botond)
Duplicate of this bug: 1206154
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.
Created attachment 8663030 [details]
not a testcase

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

Comment 8

3 years ago
Created attachment 8663195 [details]
Display list dump, including inactive layer trees, before
(Assignee)

Comment 9

3 years ago
Created attachment 8663196 [details]
Display list dump, including inactive layer trees, after
(Assignee)

Comment 10

3 years ago
Created attachment 8663197 [details]
Display list dump, including inactive layer trees, after
Attachment #8663196 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
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
(Assignee)

Comment 13

3 years ago
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.)
Depends on: 1206915
(Assignee)

Comment 14

3 years ago
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.
(Assignee)

Comment 15

3 years ago
Created attachment 8665158 [details]
HTML display list dump with textures, before
Attachment #8663195 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8665159 [details]
HTML display list dump with textures, after
Attachment #8663197 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
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
(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.
(Assignee)

Comment 19

3 years ago
(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.
(Assignee)

Comment 20

3 years ago
Created attachment 8666090 [details]
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
Attachment #8666090 - Flags: review?(mstange)
Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

https://reviewboard.mozilla.org/r/20439/#review18339
Attachment #8666090 - Flags: review?(mstange) → review+
(Assignee)

Comment 22

3 years ago
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.
(Assignee)

Comment 23

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

Bug 1205630 - Reftest. r=mstange
Attachment #8666090 - Attachment description: MozReview Request: Bug 1205630 - Translate a fixed background display item's clip rect correctly when setting it on the layer. r=mstange → MozReview Request: Bug 1205630 - Reftest. r=mstange
(Assignee)

Comment 24

3 years ago
Oops, I forgot MozReview clobbers the first review request when I post a second one...
(Assignee)

Comment 25

3 years ago
Created 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
Attachment #8666242 - Flags: review?(mstange)
(Assignee)

Comment 26

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

Bug 1205630 - Reftest. r=mstange
(Assignee)

Comment 27

3 years ago
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.
Attachment #8666242 - Flags: review?(mstange) → review+
(Assignee)

Comment 28

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

Updated test patch to add a comment explaining what is being tested.
Attachment #8666090 - Flags: review+ → review?(mstange)
(Assignee)

Comment 30

3 years ago
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
Attachment #8666242 - Flags: review+ → review?(mstange)
(Assignee)

Comment 31

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

Bug 1205630 - Reftest. r=mstange
(Assignee)

Comment 32

3 years ago
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+.
Attachment #8666242 - Flags: review?(mstange) → review+
(Assignee)

Comment 33

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

Updated to fix a typo in reftest.list.
(Assignee)

Comment 34

3 years ago
New Try push that includes this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e65f2343a1
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?
Attachment #8666090 - Flags: review?(mstange) → review+
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
Attachment #8666242 - Flags: review+
(Assignee)

Comment 37

3 years ago
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.
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.
(Reporter)

Comment 40

3 years ago
[Tracking Requested - why for this release]: Visual regression w/ lwthemes applied
status-firefox44: --- → affected
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
(Assignee)

Comment 41

3 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
(Assignee)

Comment 43

3 years ago
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
Attachment #8666242 - Flags: review+
(Assignee)

Comment 44

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

Bug 1205630 - Reftest. r=mstange
(Assignee)

Comment 45

3 years ago
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
(Assignee)

Comment 46

3 years ago
(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.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1211520
https://hg.mozilla.org/mozilla-central/rev/50bb473484fb
https://hg.mozilla.org/mozilla-central/rev/bae7847cafbd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

3 years ago
Duplicate of this bug: 1212025
(Assignee)

Comment 51

3 years ago
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.
Attachment #8666242 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8666090 - Flags: approval-mozilla-aurora?
Tracking since this is a recent regression.
status-firefox42: --- → unaffected
tracking-firefox43: ? → +
tracking-firefox44: ? → +
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.
Attachment #8666242 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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 :)
Duplicate of this bug: 1213309
Thanks Tomcat!  Yes I missed it, glad you caught that and uplifted the tests too.
Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Tests - already landed. Just tweaking the flag.
Attachment #8666090 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.