1450360 - Dark theme floating scrollbars no longer respond to mouse events

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Julian Descottes [:jdescottes]]

Attachment: scrollbars-displayed-below-ui.png

Found while looking at Bug 1450320

STRs:
- open devtools
- enable dark theme
- go to inspector and resize the toolbox to get a scrollbar on the markup view
- try to drag the scrollbar

ER: should drag
AR: nothing happens.

Some other scrollbars work a bit better, eg in the rule view, but not all the time. I guess sometimes they are below invisible elements which catch the event instead of the scrollbar.

Note also that the highlighted line of the markup view goes over the floating scrollbar. Similarly in RDM, the floating scrollbars are now displayed below some UI elements (attaching screenshot).

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dcd10220d55aea46db212314c46d25a96a7be243&tochange=3f37287132bff08037b75b07e5485fcaa29ee886

Might be Bug 1448841.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: respect-zindex-scrollbar

The devtools dark theme is faking overlay scrollbars, by applying styling to hide the track, push it out over content using margins and putting it on top using z-index.

We never respected the z-index style, but we always put scrollbars on top anyway, so it worked.

This patches makes sure we actually go into the positioned descendants list if z-index was specified.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/924e90305946
Respect the z-index property set on scrollbars. r=mstange

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Created attachment 8964223 [details] [diff] [review]
> respect-zindex-scrollbar
> 
> The devtools dark theme is faking overlay scrollbars, by applying styling to
> hide the track, push it out over content using margins and putting it on top
> using z-index.
> 
> We never respected the z-index style, but we always put scrollbars on top
> anyway, so it worked.
> 
> This patches makes sure we actually go into the positioned descendants list
> if z-index was specified.

As an aside: we don't really want to be faking overlay scrollbars for the dark theme. But AFAIK it's the only way to currently get the dark theme to not look bad with scrollbars on Windows. What we'd rather have is some way to use native scrollbars but tell them to be dark - probably using the CSS Scrollbars spec, but if you have ideas on other ways to support this we'd be all ears. There's more discussion here about it here: https://github.com/devtools-html/rfcs/issues/27#issuecomment-344675487.

Comment 4

[Cosmin Sabou [:CosminS]]

Backed out for mochitest e-10s failures on layout/forms/test/test_bug1301290.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0cb041700c38f2f2a436252de179ac8ed6f299f3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=e10s&selectedJob=171409741

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171409822&repo=mozilla-inbound&lineNumber=8906

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/1758267802e956446af295b0e6ef0b3bd4104a26

Comment 5

[Matt Woodrow (:mattwoodrow)]

Ugh, scrollbars :(

We explicitly style the resizer with a z-index here: https://searchfox.org/mozilla-central/source/toolkit/content/minimal-xul.css#79

The failing test (test_bug1301290.html) however, relies on us ignoring that, and having 'overlay' scrollbar behaviour (where the resizer goes into the Content list, if the scrollframe has no other positioned descendants).

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: respect-zindex-scrollbar

This preserves the existing behaviour, where the resizer is treated as an overlay, and includes a new method of handling bug 1450520, which takes the presence of resizers into account.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: respect-zindex-scrollbar

Looks like Windows always has the resizer frame present, so the previous patch broke retained-dl entirely.

HasResizer is false (confirmed locally in a debugger), so checking for that makes the tests pass again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47fd712ead6b2e959d300eea73069a5ecad113cc

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c379b660d5a8
Respect the z-index property set on scrollbars. r=mstange

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c379b660d5a8

Comment 10

[Julian Descottes [:jdescottes]]

Thanks a lot for jumping on this!

Comment 12

[Matt Woodrow (:mattwoodrow)]

It looks like a branch merge accidentally removed the RetainedDisplayListBuilder piece of this code getting removed, which then resulted in bug 1443027 getting backed out.

The merge changeset that removed the code is here: https://hg.mozilla.org/integration/mozilla-inbound/rev/99a953f1823f#l19.773

Arthur, it looks like you landed this merge. Do you remember if there were merge conflicts that needed manual resolution, or did mercurial handle this itself?

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1450360 - Reland chunk that got accidentally removed during a branch merge.

Review commit: https://reviewboard.mozilla.org/r/233450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233450/

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8964726 [details]
Bug 1450360 - Reland chunk that got accidentally removed during a branch merge.

https://reviewboard.mozilla.org/r/233450/#review239030

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08c9beb155d5
Reland chunk that got accidentally removed during a branch merge. r=mattwoodrow

Comment 16

[Andreea Pavel [:apavel]]

Backed out for build bustages at builds/worker/workspace/build/src/layout/painting/RetainedDisplayListBuilder.cpp:1118

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=08c9beb155d5233f1e57073b44e9175ef6231de3

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171745312&repo=autoland&lineNumber=15728

Backout: https://hg.mozilla.org/integration/autoland/rev/d112cf7b2b60e6244099dc3b599a2444ba0d1da3

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8964726 [details]
Bug 1450360 - Reland chunk that got accidentally removed during a branch merge.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233450/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5135503f825
Reland chunk that got accidentally removed during a branch merge. r=mattwoodrow

Comment 19

[Andreea Pavel [:apavel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Comment on attachment 8964726 [details]
> Bug 1450360 - Reland chunk that got accidentally removed during a branch
> merge.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/233450/diff/1-2/

There were also other bustages on that push: 

https://treeherder.mozilla.org/logviewer.html#?job_id=171745296&repo=autoland&lineNumber=27284

and

https://treeherder.mozilla.org/logviewer.html#?job_id=171745302&repo=autoland&lineNumber=23482

They appeared after the backout. These too are fixed by the push in comment 18?

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to Andreea Pavel [:apavel] from comment #19)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> > Comment on attachment 8964726 [details]
> > Bug 1450360 - Reland chunk that got accidentally removed during a branch
> > merge.
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/233450/diff/1-2/
> 
> There were also other bustages on that push: 
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=171745296&repo=autoland&lineNumber=27284
> 
> and
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=171745302&repo=autoland&lineNumber=23482
> 
> They appeared after the backout. These too are fixed by the push in comment
> 18?

Yeah, they're all the same issue, just different messages from different compilers.

Comment 21

[Andreea Pavel [:apavel]]

Thank you!

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> It looks like a branch merge accidentally removed the
> RetainedDisplayListBuilder piece of this code getting removed, which then
> resulted in bug 1443027 getting backed out.
> 
> The merge changeset that removed the code is here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/99a953f1823f#l19.773
> 
> Arthur, it looks like you landed this merge. Do you remember if there were
> merge conflicts that needed manual resolution, or did mercurial handle this
> itself?
(Arthur doesn't work today, replying for him.)

There was a incorrectly resolved merge conflict, but not on the linked merge and not by Arthur, but by the previous shift when central with https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=d75d996016dcf325c2db2ed8a47af512d07ffacd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable as tip got merged to autoland which had https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=87840a3dd71583805be28dd07f9aedaa90d32142&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable as tip.

That later got merged to central and to inbound.

Sorry for the trouble, will explain to the sheriffs what should have been done.

Comment 23

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d5135503f825

Comment 24

[Matt Woodrow (:mattwoodrow)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #22)
> 
> There was a incorrectly resolved merge conflict, but not on the linked merge
> and not by Arthur, but by the previous shift when central with
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&revision=d75d996016dcf325c2db2ed8a47af512d07ffacd&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable as tip got merged to
> autoland which had
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=87840a3dd71583805be28dd07f9aedaa90d32142&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable as tip.
> 
> That later got merged to central and to inbound.
> 
> Sorry for the trouble, will explain to the sheriffs what should have been
> done.

Ah great, thanks Sebastian!

Comment 25

[Timea Cernea [:tbabos][inactive]]

I have reproduced the issue using Firefox 61.0a1 (2018-03-30) on Windows 10 x64. 

On the latest Firefox Nightly (Build ID: 20180426100055) and latest Firefox Beta 60.0b15, the issue cannot be reproduced on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64.