Closed Bug 1802338 Opened 1 year ago Closed 1 year ago

RDM z-index tweaks

Categories

(DevTools :: Responsive Design Mode, defect)

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox109 fixed)

Status:
RESOLVED FIXED
Milestone:
109 Branch
Tracking Flags:
Tracking Status
firefox109 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

Bug 1802338 - Tweak z-index of the RDM toolbar. r=nchevobbe
48 bytes, text/x-phabricator-request
Details | Review
Bug 1802338 - Centralize z-indices that fight in the browser area. r=Gijs,dao
48 bytes, text/x-phabricator-request
Details | Review

Two issues:

  • The dependent bug is an issue for the RDM toolbar. That's pre-existing but seems it'd be worth being consistent.

  • The splitter for regular devtools draws on top of the RDM toolbar. That's because of this line of code.

Assignee: nobody → emilio

So that it paints on top of the dialog stack, and the devtools splitter.

Not sure if this is worth it, your call. But it's a bit clearer to
figure out what fights with what.

Depends on D163002

The Bugbug bot thinks this bug should belong to the 'DevTools::Responsive Design Mode' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: General → Responsive Design Mode

Backed out for causing failures on z-index

task 2022-11-30T02:13:27.931Z] 02:13:27     INFO - Buffered messages finished
[task 2022-11-30T02:13:27.932Z] 02:13:27     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | custom property `--browser-stack-z-index-devtools-splitter` is not referenced - 
[task 2022-11-30T02:13:27.932Z] 02:13:27     INFO - Stack trace:
[task 2022-11-30T02:13:27.932Z] 02:13:27     INFO - chrome://mochikit/content/browser-test.js:test_ok:1449
[task 2022-11-30T02:13:27.932Z] 02:13:27     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:533
[task 2022-11-30T02:13:27.932Z] 02:13:27     INFO - Not taking screenshot here: see the one that was previously logged
[task 2022-11-30T02:13:27.933Z] 02:13:27     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | custom property `--browser-stack-z-index-rdm-toolbar` is not referenced - 
[task 2022-11-30T02:13:27.933Z] 02:13:27     INFO - Stack trace:
[task 2022-11-30T02:13:27.933Z] 02:13:27     INFO - chrome://mochikit/content/browser-test.js:test_ok:1449
[task 2022-11-30T02:13:27.933Z] 02:13:27     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:533
[task 2022-11-30T02:13:27.934Z] 02:13:27     INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-has-dir-attr’.  Ruleset ignored due to bad selector." on resource://gre-resources/html.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true}
Flags: needinfo?(emilio)

So the test fails because if I understand how this test works, we run it once with only non-devtools stylesheets, and once with only devtools stylesheets. Do you know why this is? Obviously since I defined the variables in a non-devtools css and used them in devtools, stuff breaks and thinks they're unused / unreferenced.

Do you have any suggestion for how to best get around it / something else? I could whitelist them but that seems unfortunate.

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

So the test fails because if I understand how this test works, we run it once with only non-devtools stylesheets, and once with only devtools stylesheets. Do you know why this is?

bug 1013518 comment 0 has rationale.

Obviously since I defined the variables in a non-devtools css and used them in devtools, stuff breaks and thinks they're unused / unreferenced.
Do you have any suggestion for how to best get around it / something else? I could whitelist them but that seems unfortunate.

Can we add chrome://browser/content/browser.css into the list of CSS files loaded into the devtools version? I guess that leaves the RDM one unused in the non-devtools run - you could allowlisting just that variable, though it's still not pretty. You could add a dummy browser "use" of the variable but that seems worse than allowlisting in just the test.

Does that help?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ded71887ed3
Tweak z-index of the RDM toolbar. r=devtools-reviewers,jdescottes
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a19f569bced3
Tweak z-index of the RDM toolbar. r=devtools-reviewers,jdescottes

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

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox109: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 109 Branch

(In reply to :Gijs (he/him) from comment #6)

Can we add chrome://browser/content/browser.css into the list of CSS files loaded into the devtools version? I guess that leaves the RDM one unused in the non-devtools run - you could allowlisting just that variable, though it's still not pretty. You could add a dummy browser "use" of the variable but that seems worse than allowlisting in just the test.

Does that help?

That helps but would also leave a bunch of stuff like:

FAIL custom property `--panel-color` is not referenced

On the devtools run... I guess I can just whitelist these properties in both runs and call it a day, wdyt?

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to :Gijs (he/him) from comment #6)

Can we add chrome://browser/content/browser.css into the list of CSS files loaded into the devtools version? I guess that leaves the RDM one unused in the non-devtools run - you could allowlisting just that variable, though it's still not pretty. You could add a dummy browser "use" of the variable but that seems worse than allowlisting in just the test.

Does that help?

That helps but would also leave a bunch of stuff like:

FAIL custom property `--panel-color` is not referenced

I'm confused, this isn't defined in browser/base/content/browser.css, so how is it even showing up?

On the devtools run... I guess I can just whitelist these properties in both runs and call it a day, wdyt?

Assuming I'm missing something with my first question - are the custom property bits not remembering which file they were defined in? I was assuming that because the devtools job filters out errors/warnings from non-devtools things and vice versa that this wouldn't happen... Fixing that part would probably be more maintainable than having an allowlist that duplicates all the browser.css variables that trip this file (and would require that people adding/removing more variables also add/remove more allowlist entries, which is tedious)

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #11)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to :Gijs (he/him) from comment #6)

Can we add chrome://browser/content/browser.css into the list of CSS files loaded into the devtools version? I guess that leaves the RDM one unused in the non-devtools run - you could allowlisting just that variable, though it's still not pretty. You could add a dummy browser "use" of the variable but that seems worse than allowlisting in just the test.

Does that help?

That helps but would also leave a bunch of stuff like:

FAIL custom property `--panel-color` is not referenced

I'm confused, this isn't defined in browser/base/content/browser.css, so how is it even showing up?

Platform-specific browser.css loads browser-shared.css which loads ctrlTab.css?

Assuming I'm missing something with my first question - are the custom property bits not remembering which file they were defined in? I was assuming that because the devtools job filters out errors/warnings from non-devtools things and vice versa that this wouldn't happen... Fixing that part would probably be more maintainable than having an allowlist that duplicates all the browser.css variables that trip this file (and would require that people adding/removing more variables also add/remove more allowlist entries, which is tedious)

Well, once we decide "this run is for devtools", we assume all the defined variables etc are "from devtools". We could fix that somehow I guess, but it's still awkward...

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

(In reply to :Gijs (he/him) from comment #11)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to :Gijs (he/him) from comment #6)

Can we add chrome://browser/content/browser.css into the list of CSS files loaded into the devtools version? I guess that leaves the RDM one unused in the non-devtools run - you could allowlisting just that variable, though it's still not pretty. You could add a dummy browser "use" of the variable but that seems worse than allowlisting in just the test.

Does that help?

That helps but would also leave a bunch of stuff like:

FAIL custom property `--panel-color` is not referenced

I'm confused, this isn't defined in browser/base/content/browser.css, so how is it even showing up?

Platform-specific browser.css loads browser-shared.css which loads ctrlTab.css?

Sorry, I was trying to be explicit earlier about not loading platform-specific browser.css - just the platform-agnostic content packaged one (chrome://browser/content/browser.css, not chrome://browser/skin/browser.css). Yes, it's confusing we have 2 browser.css (though this kind of thing used to be the norm...) - but the changes in the patch here are in the platform-agnostic browser.css, so I don't think you need the platform-specific skin browser.css ?

Flags: needinfo?(emilio)

Hmm, I tried this again and just whitelisting two of the variables seemed to work, so I'll just reland without that hack, but I'll re-try if it somehow bounces.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ccaec9f3c11a
Centralize z-indices that fight in the browser area. r=Gijs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: