Closed Bug 1179780 Opened 9 years ago Closed 9 years ago

TEST-UNEXPECTED-FAIL | layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window.

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: acomminos, Assigned: acomminos)

References

Details

Attachments

(2 files, 6 obsolete files)

Update deferred-anchor2.xhtml reftest to be independent of transparent scrollbars.
(deleted), patch
mstange
: review+
Details | Diff | Splinter Review
Render opaque scrollbar troughs in the root viewport on GTK3. Carry r=karlt
(deleted), patch
acomminos
: review+
Details | Diff | Splinter Review 
Leaf layer partitioning tests fail on GTK3 try.

TEST-UNEXPECTED-FAIL | layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.] - expected PASS
TEST-UNEXPECTED-FAIL | layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.] - expected PASS
This appears to be caused by having transparent scrollbar troughs on GTK3, making the content layer not render as opaque- thus not clipping other leaf layers. Perhaps we should consider dropping support for transparent scrollbars, instead rendering an opaque background behind them and reverting the changes in bug 1169666?
This is the same approach as was initially suggested in the first patch. Drawing an opaque trough allows much better masking of leaf layers.
Attachment #8629036 - Flags: review?(karlt)
This reverts the changes made in bug 1169666, which are no longer needed.
Attachment #8629037 - Flags: review?(karlt)
Attachment #8629037 - Attachment description: Remove scrollbar transparency support in reftests. → Part 2 - Remove scrollbar transparency support in reftests.
(In reply to Andrew Comminos [:acomminos] from comment #1)
> This appears to be caused by having transparent scrollbar troughs on GTK3,
> making the content layer not render as opaque- thus not clipping other leaf
> layers.

I'm not clear what the consequences of that are.

What are the other leaf layers that are not clipped?

Can you add the output with MOZ_DUMP_PAINT_LIST=1, please?

What provides the background for the square in the bottom right of
data:text/html,<html style='overflow:scroll'> ?

It would also be interesting to know if this test passes on Mac.
It was disabled in 0d3c93bc571 of Bug 539356 for bug 748219, which was worked
around but now marked as fixed.
Flags: needinfo?(acomminos)
Here's the dump of the layer tree when snapshotted for the test:

> ClientLayerManager (0x7fffc5c2fa00)
>   ClientContainerLayer (0x7fffd90c9000) [visible=< (x=0, y=0, w=1000, h=1142); >] [opaqueContent] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1000.000000, h=1142.000000)] [sr=(x=0.000000, y=0.000000, w=500.000000, h=571.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [color=rgba(0, 0, 0, 0)] [scrollId=5] [z=2] }]
>     ClientPaintedLayer (0x7fffd90ce400) [bounds=(x=0, y=0, w=1000, h=1142)] [visible=< (x=0, y=0, w=1000, h=1142); >] [opaqueContent] [valid=< (x=0, y=0, w=1000, h=1142); >]
>       ContentClient (0x7fffc00dd460)
>     ClientContainerLayer (0x7fffd9ae3000) [clip=(x=0, y=142, w=1000, h=1000)] [visible=< (x=0, y=142, w=1000, h=1000); >]
>       ClientContainerLayer (0x7fffd9ae4400) [clip=(x=0, y=142, w=1000, h=1000)] [visible=< (x=0, y=1116, w=974, h=26); >]
>         ClientPaintedLayer (0x7fffd9ae4c00) [bounds=(x=0, y=1116, w=974, h=26)] [visible=< (x=0, y=1116, w=974, h=26); >] [opaqueContent] [valid=< (x=0, y=1116, w=974, h=26); >]
>           ContentClient (0x7fffc00ddac0)
>       ClientContainerLayer (0x7fffd9ae6800) [clip=(x=0, y=142, w=1000, h=1000)] [visible=< (x=974, y=142, w=26, h=974); >]
>         ClientPaintedLayer (0x7fffd9ae7400) [bounds=(x=974, y=142, w=26, h=974)] [visible=< (x=974, y=142, w=26, h=974); >] [opaqueContent] [valid=< (x=974, y=142, w=26, h=974); >]
>           ContentClient (0x7fffc00ddbd0)
>       ClientContainerLayer (0x7fffd9ae7c00) [clip=(x=0, y=142, w=1000, h=1000)] [visible=< (x=974, y=1116, w=26, h=26); >] [opaqueContent]
>         ClientPaintedLayer (0x7fffd9ae8000) [clip=(x=0, y=0, w=0, h=0)] [not visible]
>         ClientColorLayer (0x7fffd9ae8400) [bounds=(x=974, y=1116, w=26, h=26)] [visible=< (x=974, y=1116, w=26, h=26); >] [opaqueContent] [color=rgba(237, 237, 237, 1)] [bounds=(x=974, y=1116, w=26, h=26)]
>       ClientPaintedLayer (0x7fffd9ae9800) [clip=(x=0, y=0, w=0, h=0)] [transform=[ 1 0; 0 1; 0 142; ]] [not visible]
>       ClientColorLayer (0x7fffd9aecc00) [transform=[ 1 0; 0 1; 0 142; ]] [bounds=(x=0, y=0, w=974, h=974)] [visible=< (x=0, y=0, w=974, h=974); >] [opaqueContent] [color=rgba(255, 255, 255, 1)] [bounds=(x=0, y=0, w=974, h=974)]

Observe that the ClientContainerLayers hosting the scrollbar troughs do not propagate the opacity of their children (due to the widget transparency setting). This forces the ClientPaintedLayer at address 0x7fffd90ce400 to fill the entire content area, instead of clipping to above the layer at 0x7fffd9ae3000.

I'm not sure what paints the bottom right scrollbar joiner, it does have its own (opaque) layer however at 0x7fffd9ae8400.

Whether or not this is acceptable is up for discussion. I haven't found any other browsers using GTK3 that seem to 'do the right thing', Epiphany doesn't even fill behind the trough, leaving garbage that occasionally changes.
Flags: needinfo?(acomminos)
(In reply to Andrew Comminos [:acomminos] from comment #5)
> This forces the ClientPaintedLayer at address 0x7fffd90ce400 to
> fill the entire content area, instead of clipping to above the layer at
> 0x7fffd9ae3000.

Thanks.  That's what this test was designed to detect, I assume.

In layout/reftests/z-index/overlayscrollbar-sorting-ref-visible.html
(bug 1169666) we saw the cyan background of the overflow:auto div show behind
its scrollbars, but here we don't have the background (in 0x7fffd9aecc00 I assume) of the overflow:scroll html document behind its scrollbars.

I guess that's because overflow is special on the html element in that the
overflow applies to an anonymous parent viewport: "UAs must apply the overflow
property set on the root element to the viewport."

So I guess here we want the window background behind root viewports'
scrollbars, but we want the element background behind non-root-element
scrollbars.  That's what we are getting I assume, but the layer structure is
not ideal for the root viewport.

roc, would adding a window-background-color display item behind the root
viewport's scrollbars be a practical way to get the desired layer structure?
Flags: needinfo?(roc)
Yes, anything that makes the scrollbars opaque using an opaque color should do.
Flags: needinfo?(roc)
Comment on attachment 8629036 [details] [diff] [review]
Part 1 - Render opaque scrollbar troughs on GTK3.

I don't think we want window background on all scrollbars and I don't think
the consequences of the test failure here are bad enough that we need to rush
this in as a workaround.

I suspect the square providing 0x7fffd9ae8400 between the scrollbars will be
the clue on how to extend that color behind scrollbars in root viewports.
Attachment #8629036 - Flags: review?(karlt) → review-
Comment on attachment 8629037 [details] [diff] [review]
Part 2 - Remove scrollbar transparency support in reftests.

I think we should just silence the failure in test_leaf_layers_partition_browser_window.xul and leave this bug open if putting a display item behind the root viewport's scrollbars is not trivial.
Attachment #8629037 - Flags: review?(karlt) → review-
I agree, having opaque scrollbars only in the root viewport makes a lot of sense. I think it would be better to put this change in the widget code rather than adding an extra item to nsGfxScrollFrame as it's a pretty platform-specific change.

Karl, what do you think of this approach? It passes the leaf layers test and doesn't fill scrollbars outside of the root.
Attachment #8629036 - Attachment is obsolete: true
Attachment #8629037 - Attachment is obsolete: true
Attachment #8631678 - Flags: review?(karlt)
Comment on attachment 8631678 [details] [diff] [review]
Render opaque scrollbar troughs in the root viewport on GTK3.

This looks good to me, thanks.

root_ only implies root of the document, not necessarily the topmost content
document, but that's a minor issue that can be addressed separately if
necessary.

nsGfxScrollFrame uses additional tests when it wants the topmost content
document:
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/layout/generic/nsGfxScrollFrame.cpp#l2176
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/layout/generic/nsGfxScrollFrame.cpp#l850
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/layout/generic/nsGfxScrollFrame.cpp#l850

>     case MOZ_GTK_SCROLLBAR_BUTTON:
>         return moz_gtk_scrollbar_button_paint(cr, rect, state,
>                                               (GtkScrollbarButtonFlags) flags,
>                                               direction);
>         break;
>     case MOZ_GTK_SCROLLBAR_TRACK_HORIZONTAL:
>     case MOZ_GTK_SCROLLBAR_TRACK_VERTICAL:
>         return moz_gtk_scrollbar_trough_paint(widget, cr, rect,
>-                                              state, direction);
>+                                              state, direction,
>+                                              (GtkScrollbarTrackFlags) flags);

Please pass flags before direction, for consistency with other functions.
Attachment #8631678 - Flags: review?(karlt) → review+
Thanks, updated to fix flags argument position and call IsRootContentDocument on the track.
Attachment #8631678 - Attachment is obsolete: true
Attachment #8632100 - Flags: review+
Updated to only affect chrome widgets.
Attachment #8632100 - Attachment is obsolete: true
Attachment #8632815 - Flags: review+
Check if in namespace XUL instead of IsChrome check. Fix type mismatch.
Attachment #8632815 - Attachment is obsolete: true
Attachment #8632965 - Flags: review+
Comment on attachment 8632966 [details] [diff] [review]
Update deferred-anchor2.xhtml reftest to be independent of transparent scrollbars.

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

Have you confirmed that -moz-Dialog is guaranteed to be the same color as the one you draw behind opaque scrollbars?
Attachment #8632966 - Flags: review?(mstange) → review+
Yup, we assign it to the window background in widget/gtk/nsLookAndFeel.cpp for GTK3.

> 103     case eColorID__moz_dialog:
> 105         aColor = sMozWindowBackground;
Updated to zero out flags, (hopefully) stopping intermittent reftest failures.

GTK3 try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be8c47535ada
Attachment #8632965 - Attachment is obsolete: true
Attachment #8633482 - Flags: review+
Try push results are as expected, last few GTK3 reftests fixed in other bugs.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5483abed1dba
https://hg.mozilla.org/mozilla-central/rev/a0910fa98706
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: