Dark theme floating scrollbars no longer respond to mouse events

VERIFIED FIXED in Firefox 61

Status

defect
VERIFIED FIXED
Last year
8 months ago

People

(Reporter: jdescottes, Assigned: mattwoodrow)

Tracking

({regression})

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61+ verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Posted patch respect-zindex-scrollbar (obsolete) — Splinter Review
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.
Assignee: nobody → matt.woodrow
Attachment #8964223 - Flags: review?(mstange)
Attachment #8964223 - Flags: review?(mstange) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/924e90305946
Respect the z-index property set on scrollbars. r=mstange
(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.
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).
Flags: needinfo?(matt.woodrow)
Posted patch respect-zindex-scrollbar (obsolete) — Splinter Review
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.
Attachment #8964244 - Flags: review?(mstange)
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
Attachment #8964223 - Attachment is obsolete: true
Attachment #8964244 - Attachment is obsolete: true
Attachment #8964244 - Flags: review?(mstange)
Attachment #8964437 - Flags: review?(mstange)
Attachment #8964437 - Flags: review?(mstange) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c379b660d5a8
Respect the z-index property set on scrollbars. r=mstange
https://hg.mozilla.org/mozilla-central/rev/c379b660d5a8
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Thanks a lot for jumping on this!
Duplicate of this bug: 1450320
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?
Flags: needinfo?(aiakab)
Comment on attachment 8964726 [details]
Bug 1450360 - Reland chunk that got accidentally removed during a branch merge.

https://reviewboard.mozilla.org/r/233450/#review239030
Attachment #8964726 - Flags: review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08c9beb155d5
Reland chunk that got accidentally removed during a branch merge. r=mattwoodrow
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
Flags: needinfo?(matt.woodrow)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5135503f825
Reland chunk that got accidentally removed during a branch merge. r=mattwoodrow
(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?
(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.
Flags: needinfo?(matt.woodrow)
Thank you!
(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.
Flags: needinfo?(aiakab)
(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!
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.