Closed Bug 1205630 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 + fixed

People

(Reporter: ryanvm, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(7 files, 3 obsolete files)

Attached image 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)
I see the same thing on Windows 7.
Assignee: nobody → botond
Whiteboard: [gfx-noted]
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.
Attached file not a testcase
I thought this might reproduce the problem, but it doesn't.
Attachment #8663196 - Attachment is obsolete: true
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
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
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.
Attachment #8663195 - Attachment is obsolete: true
Attachment #8663197 - Attachment is obsolete: true
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.
(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.
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+
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 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
Oops, I forgot MozReview clobbers the first review request when I post a second one...
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)
Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=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

MozReview continues to be very confused... carrying r+ for fix.
Attachment #8666242 - Flags: review?(mstange) → review+
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)
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)
Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=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

Carrying r+.
Attachment #8666242 - Flags: review?(mstange) → review+
Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Updated to fix a typo in reftest.list.
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+
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.
[Tracking Requested - why for this release]: Visual regression w/ lwthemes applied
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)
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+
Comment on attachment 8666090 [details]
MozReview Request: Bug 1205630 - Reftest. r=mstange

Bug 1205630 - Reftest. r=mstange
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
(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.
Duplicate of this bug: 1211520
https://hg.mozilla.org/mozilla-central/rev/50bb473484fb
https://hg.mozilla.org/mozilla-central/rev/bae7847cafbd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Duplicate of this bug: 1212025
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?
Attachment #8666090 - Flags: approval-mozilla-aurora?
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 :)
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.