Closed
Bug 1205630
Opened 9 years ago
Closed 9 years ago
Adjacent tabs show a visual overlap when hovered over when a lwtheme is applied
Categories
(Core :: Graphics, defect)
Core
Graphics
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)
981 bytes,
image/png
|
Details | |
2.03 KB,
image/png
|
Details | |
666 bytes,
text/html
|
Details | |
404.25 KB,
text/html
|
Details | |
457.88 KB,
text/html
|
Details | |
40 bytes,
text/x-review-board-request
|
mstange
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
40 bytes,
text/x-review-board-request
|
mstange
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
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•9 years ago
|
||
Comment 2•9 years ago
|
||
I see the same thing on Windows 7.
Updated•9 years ago
|
Assignee: nobody → botond
Whiteboard: [gfx-noted]
Assignee | ||
Comment 3•9 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)
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
I thought this might reproduce the problem, but it doesn't.
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8663196 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 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•9 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•9 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•9 years ago
|
||
Attachment #8663195 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8663197 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 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
Comment 18•9 years ago
|
||
(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•9 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•9 years ago
|
||
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 21•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Oops, I forgot MozReview clobbers the first review request when I post a second one...
Assignee | ||
Comment 25•9 years ago
|
||
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•9 years ago
|
||
Comment on attachment 8666090 [details] MozReview Request: Bug 1205630 - Reftest. r=mstange Bug 1205630 - Reftest. r=mstange
Assignee | ||
Comment 27•9 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•9 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 29•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d541a83772cf
Assignee | ||
Comment 30•9 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•9 years ago
|
||
Comment on attachment 8666090 [details] MozReview Request: Bug 1205630 - Reftest. r=mstange Bug 1205630 - Reftest. r=mstange
Assignee | ||
Comment 32•9 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•9 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•9 years ago
|
||
New Try push that includes this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e65f2343a1
Comment 35•9 years ago
|
||
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 36•9 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 https://reviewboard.mozilla.org/r/20475/#review18407
Attachment #8666242 -
Flags: review+
Assignee | ||
Comment 37•9 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.
Comment 38•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db06671fc0d https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca5a29b914f
Comment 39•9 years ago
|
||
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•9 years ago
|
||
[Tracking Requested - why for this release]: Visual regression w/ lwthemes applied
Assignee | ||
Comment 41•9 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)
Comment 42•9 years ago
|
||
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•9 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•9 years ago
|
||
Comment on attachment 8666090 [details] MozReview Request: Bug 1205630 - Reftest. r=mstange Bug 1205630 - Reftest. r=mstange
Assignee | ||
Comment 45•9 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•9 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.
Comment 47•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/50bb473484fb https://hg.mozilla.org/integration/mozilla-inbound/rev/bae7847cafbd
Comment 49•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/50bb473484fb https://hg.mozilla.org/mozilla-central/rev/bae7847cafbd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 51•9 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•9 years ago
|
Attachment #8666090 -
Flags: approval-mozilla-aurora?
Comment 52•9 years ago
|
||
Tracking since this is a recent regression.
Comment 53•9 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 Minor fix for recent regression, has reftests. Approved for uplift to aurora.
Attachment #8666242 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 54•9 years ago
|
||
(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•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea5e14f1c519 https://hg.mozilla.org/releases/mozilla-aurora/rev/a2d2548a5697
Comment 57•9 years ago
|
||
Thanks Tomcat! Yes I missed it, glad you caught that and uplifted the tests too.
Comment 58•9 years ago
|
||
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.
Description
•