Closed
Bug 1450360
Opened 7 years ago
Closed 7 years ago
Dark theme floating scrollbars no longer respond to mouse events
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61+ verified)
VERIFIED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | verified |
People
(Reporter: jdescottes, Assigned: mattwoodrow)
References
Details
(Keywords: regression)
Attachments
(3 files, 2 obsolete files)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Updated•7 years ago
|
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
Comment 3•7 years ago
|
||
(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•7 years ago
|
||
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
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Updated•7 years ago
|
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
Comment 9•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Reporter | ||
Comment 10•7 years ago
|
||
Thanks a lot for jumping on this!
Assignee | ||
Comment 12•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review |
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+
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
(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?
Assignee | ||
Comment 20•7 years ago
|
||
(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)
Comment 21•7 years ago
|
||
Thank you!
![]() |
||
Comment 22•7 years ago
|
||
(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)
Comment 23•7 years ago
|
||
bugherder |
Assignee | ||
Comment 24•7 years ago
|
||
(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!
Updated•7 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
tracking-firefox61:
--- → +
Updated•7 years ago
|
Flags: qe-verify+
Comment 25•7 years ago
|
||
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.
Updated•7 years ago
|
Keywords: regression
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•