Andrew Comminos [:acomminos] Assignee
	Description • 9 years ago

In GTK3 themes it's possible for scrollbars to have transparency, as noted in bug 982640. As a result, Ubuntu's default theme 'Ambiance' fails the reftest layout/reftests/z-index/overlayscrollbar-sorting-1.html, as it uses the border-radius property- causing the CSS background of the parent to leak through in the reference.

	Andrew Comminos [:acomminos] Assignee
	Comment 1 • 9 years ago

This patch resolves the issue, but likely isn't the best way to do it. Karl, if you have any suggestions, that would be great.

Reftest push with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab4b22be0b1f

	bull500
	Comment 2 • 9 years ago

Just want to add to the bug that scroll-bar gets cut on the bottom, while scrolling

	Karl Tomlinson (:karlt)
	Comment 3 • 9 years ago

Comment on attachment 8612931 [details] [diff] [review]
Draw generic widget background behind GTK3 scrollbars.

This is a reasonable approach if we need the theme code to render opaque troughs.

However, with the rounded troughs, I think it looks better to keep the transparent corners to show a document background, than to have the corners filled in with the window background.  This reftest demonstrates the issue nicely.  So I'd like to keep the current behavior of the theme code unless there is a strong reason to change it.

However, this reftest is expecting the scrollable content background to be visible but we are seeing the ancestor background.  I don't know whether the particular element for the background is specified, but I think the current gtk3 behavior is pleasant.

I recommend annotating the test as failing and leaving this bug open to track whether it is really a bug in the app, or just a bug in the test.

	Jeff Muizelaar [:jrmuizel]
	Comment 4 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> Comment on attachment 8612931 [details] [diff] [review]
> 
> I recommend annotating the test as failing and leaving this bug open to
> track whether it is really a bug in the app, or just a bug in the test.

That's fair. In some cases it may make more sense to fix the test so that is passes regardless of the transparency of the scrollbars.

	Andrew Comminos [:acomminos] Assignee
	Comment 5 • 9 years ago

This simply skips the failing overlay scrollbar reftests on GTK.

Try (GTK3): https://treeherder.mozilla.org/#/jobs?repo=try&revision=331732c323b6

	Andrew Comminos [:acomminos] Assignee
	Comment 6 • 9 years ago

This updated patch makes the skipping specific to GTK3 and adds a new 'transparentScrollbars' reftest flag.

	Andrew Comminos [:acomminos] Assignee
	Comment 7 • 9 years ago

Added a few more testcases to skip.

	Karl Tomlinson (:karlt)
	Comment 8 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> However, this reftest is expecting the scrollable content background to be
> visible but we are seeing the ancestor background.  I don't know whether the
> particular element for the background is specified, but I think the current
> gtk3 behavior is pleasant.

overlayscrollbar-sorting-1.html are just testing that "overlay scrollbars are on top of positioned content", so I suspect the right fix to the test is to have the same background on the overflow:auto div in the test as in the reference, but better ask the author of the test to review that change.
overlayscrollbar-sorting-5.html may be similar.

	Karl Tomlinson (:karlt)
	Comment 9 • 9 years ago

hScrollSimpleHeight.html, 647192-1.html, and scrollable-columns.xul seem to
be assuming that scrollbars are opaque but don't say what they are trying to
test.

	Karl Tomlinson (:karlt)
	Comment 10 • 9 years ago

Comment on attachment 8623692 [details] [diff] [review]
Skip overlay scrollbar reftests on GTK3.

Please move "||transparentScrollbars" from skip-if() to fails-if() so we know when the tests are fixed.

	Jeff Muizelaar [:jrmuizel]
	Comment 11 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #10)
> Comment on attachment 8623692 [details] [diff] [review]
> Skip overlay scrollbar reftests on GTK3.
> 
> Please move "||transparentScrollbars" from skip-if() to fails-if() so we
> know when the tests are fixed.

If we set them to fails-if then they will fail with themes that don't have transparent scrollbars (e.g. on developer's machines)

	Karl Tomlinson (:karlt)
	Comment 12 • 9 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> If we set them to fails-if then they will fail with themes that don't have
> transparent scrollbars (e.g. on developer's machines)

That would be OK for reasonable tests.  Good tests fail on developers machines for code problems, and we wouldn't want the tests to be forgotten about due to code problems, in Gecko or platform libraries.  When code problems are fixed, it is not easy to find existing disabled or random-annotated tests.

But, thinking more carefully, I agree that fails-if is not necessary here because I'm fairly sure these are just broken tests that are getting in the way (assuming all the percent-overflow-sizing tests are similar).  If someone fixes the tests, then they can re-enable them.

random is probably a better option for bad tests, if the tests have some crashtest value, checking for assertions, etc.  If you are sure the tests are redundant, and for some reason are redundant only on this platform, then skip-if is OK.

	Jeff Muizelaar [:jrmuizel]
	Comment 13 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> random is probably a better option for bad tests, if the tests have some
> crashtest value, checking for assertions, etc.  If you are sure the tests
> are redundant, and for some reason are redundant only on this platform, then
> skip-if is OK.

I agree that random-if is best. I had forgotten aobut random-if when suggesting fails-if

	Andrew Comminos [:acomminos] Assignee
	Comment 14 • 9 years ago

Updated to use random-if.

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/395099ed230c

	Andrew Comminos [:acomminos] Assignee
	Comment 16 • 9 years ago

One of the reftest manifest changes mistakenly changed the order of flags, causing the OS X skip not to work. This patch fixes it.

	Pulsebot
	Comment 17 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e60b4d5ad7

	Andrew Comminos [:acomminos] Assignee
	Comment 18 • 9 years ago

Closing this, was resolved a long time ago.