Closed
Bug 1389939
Opened 7 years ago
Closed 7 years ago
Stop syncing devtools and browser dark/light themes (unbreak cases where syncing is broken, like the firebug theme)
Categories
(DevTools :: General, defect, P2)
Tracking
(firefox-esr52 unaffected, firefox55 unaffected, firefox56+ verified, firefox57 verified)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | + | verified |
firefox57 | --- | verified |
People
(Reporter: mhetflejs, Assigned: bgrins)
References
Details
(Keywords: regression, verifyme)
Attachments
(2 files)
1.07 MB,
image/gif
|
Details | |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170813100233 Steps to reproduce: 1. Close all Nightly windows 2. Start a new one 3. Apply dark theme 4. Click on 'Web developer' Actual results: Dark theme was changed to light one. Expected results: It should not affect the theme at all.
Updated•7 years ago
|
Component: Untriaged → Theme
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c6bbe349d654f15bf11baaa1f58638be640fa138&tochange=0f63a6b51c1be3f196283db97264728b2c0b6e73 Works as expected if you try to reproduce it more than once per session.
Comment 4•7 years ago
|
||
Alex, this is a regression from lazy devtools. I think we should either stop syncing the dark/light themes with devtools entirely, or devtools needs to update its own theme to the browser theme once it starts.
Blocks: 1359855
Component: Theme → Developer Tools
Flags: needinfo?(poirot.alex)
Summary: Web developer menu changes theme → Web developer menu changes theme back after changing theme with devtools not having been started
Updated•7 years ago
|
status-firefox56:
--- → affected
status-firefox57:
--- → affected
tracking-firefox56:
--- → ?
Keywords: regression
Comment 5•7 years ago
|
||
IMO we should stop the syncing now that it's exposed to non-developers.
Comment 6•7 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #5) > IMO we should stop the syncing now that it's exposed to non-developers. I'm not sure. Light devtools against a dark browser theme or vice versa is jarring, so avoiding that might still be a worthwhile goal, but maybe we need to re-evaluate how to achieve that.
Comment 7•7 years ago
|
||
(In reply to :Gijs from comment #6) > (In reply to Johann Hofmann [:johannh] from comment #5) > > IMO we should stop the syncing now that it's exposed to non-developers. > > I'm not sure. Light devtools against a dark browser theme or vice versa is > jarring, so avoiding that might still be a worthwhile goal, but maybe we > need to re-evaluate how to achieve that. That's a fair point (though I can totally see folks making bugs asking for specifically that). In any case, switching the entire chrome UI from the devtools settings always seemed a bit strange to me (personally), so I'd vote in favor of making the devtools sync their themes more passively.
It seems that the same bug occures while you 'drag&drop' some tab to open it in new window. If the theme in devtool is set to 'dark' or 'light', everything works fine. Problem is with the 'firebug' theme.
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to :Gijs from comment #6) > (In reply to Johann Hofmann [:johannh] from comment #5) > > IMO we should stop the syncing now that it's exposed to non-developers. > > I'm not sure. Light devtools against a dark browser theme or vice versa is > jarring, so avoiding that might still be a worthwhile goal, but maybe we > need to re-evaluate how to achieve that. When we switched to "Compact Themes" instead of the Dev Edition theme we originally discussed getting rid of the syncing and instead linking to about:addons from the devtools toolbox next to the theme selection: https://bugzilla.mozilla.org/show_bug.cgi?id=1314091#c9. We ultimately decided to keep the automatic syncing behavior at that time, but maybe we should revisit that decision at this point after the photon updates and lazy devtools loading. Some points about the current situation (not including this particular bug): * Dev Edition has Dark browser theme and dark devtools theme applied * Other channels have Default browser theme and light devtools theme applied * Syncing (browser -> devtools): if you apply the Dark browser theme the dark devtools theme gets applied (regardless of what was applied before). Same for Light browser theme and light devtools theme. If you then go back and apply the Default browser theme, the devtools theme is left alone. * Syncing (devtools -> browser): If you apply the dark devtools theme *and* the Dark browser theme is currently applied, the Light browser theme will be applied. Same for light devtools theme and Light browser theme. Firebug theme behavior is undefined / has bugs like bug 1370635 I'm not sure who should make this decision. Jeff, do you have an opinion about this? Here are two possible ways to go: 1) Fix this particular issue having to do with theme resetting on devtools lazy load (and other syncing issues like bug 1370635). Probably should move any syncing into a non-devtools file so we can avoid this bug. 2) Get rid of automatic syncing and instead link to about:addons or customize mode from the devtools toolbox theme section
Flags: needinfo?(jgriffiths)
Comment 10•7 years ago
|
||
To be honest, after comment 8 I wonder if this is just a dupe of bug 1370635. It's not clear to me if there's a separate issue or not, maybe the regression range just expanded the number of situations where this issue appears?
Reporter | ||
Comment 11•7 years ago
|
||
Hmm... the difference between this bug and bug 1370635 is that this one only occurs during the first devtools start. The other one happens every time with active devtools. But they are surely related. Fixing one issue would probably fix the other as well... So maybe it would be better to resolve one as a dupe. Maybe wait for needinfos and decide after that?
Comment 13•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #9) ... > I'm not sure who should make this decision. Jeff, do you have an opinion > about this? Here are two possible ways to go: > > 1) Fix this particular issue having to do with theme resetting on devtools > lazy load (and other syncing issues like bug 1370635). Probably should move > any syncing into a non-devtools file so we can avoid this bug. > 2) Get rid of automatic syncing and instead link to about:addons or > customize mode from the devtools toolbox theme section I've never been a fan of the syncing behaviour, it adds complexity and creates strange side effects. I think web developers will understand that browser themes and devtools themes are distinct. Likewise, post-photon I don't think it makes sense for the Firebug theme to impact the browser theme in any way.
Flags: needinfo?(jgriffiths)
Comment 14•7 years ago
|
||
Morphing the bug per comment #13.
Summary: Web developer menu changes theme back after changing theme with devtools not having been started → Stop syncing devtools and browser dark/light themes (unbreak cases where syncing is broken, like the firebug theme)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(poirot.alex)
Comment 15•7 years ago
|
||
Marking fix-optional and I don't think we need to track this for 56 as we're already in mid-beta. We could still take a patch for 57 though.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8900353 [details] Bug 1389939 - Stop syncing devtools theme and compact themes; https://reviewboard.mozilla.org/r/171714/#review177078
Attachment #8900353 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa4db3335c72761a13ba26abd53d68093928dc7&selectedJob=125252791
Comment 19•7 years ago
|
||
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eca0163d0964 Stop syncing devtools theme and compact themes;r=Gijs
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eca0163d0964
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 22•7 years ago
|
||
Please nominate this for Beta approval when you get a chance.
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 23•7 years ago
|
||
Comment on attachment 8900353 [details] Bug 1389939 - Stop syncing devtools theme and compact themes; Approval Request Comment [Feature/Bug causing the regression]: Devtools theme and Compact theme syncing [User impact if declined]: Bugs like 1370635 where the browser theme unexpectedly changes [Is this code covered by automated tests?]: No, this patch was removing code and associated tests [Has the fix been verified in Nightly?]: Awaiting verification [Needs manual test from QE? If yes, steps to reproduce]: Yes, see Comment 0 in this bug. Those steps need be done in a clean profile. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: It's removing a small feature (that only runs once devtools is opened) [String changes made/needed]:
Flags: needinfo?(bgrinstead)
Attachment #8900353 -
Flags: approval-mozilla-beta?
Comment 24•7 years ago
|
||
Andrei, can your team take a look and verify the fix? Thanks.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Comment 25•7 years ago
|
||
I've reproduced this issue on an affected Nightly build from 2017-08-13. Verified fixed on latest Nightly 57.0a1 (2017-09-03) across platforms: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64. I'll verify this issue in Beta as well, so letting the qe+ flag in place for now.
Flags: needinfo?(andrei.vaida)
Comment 26•7 years ago
|
||
Comment on attachment 8900353 [details] Bug 1389939 - Stop syncing devtools theme and compact themes; Fix a regressoin and was verified. Beta56+.
Attachment #8900353 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 27•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a2bbac5af299
Flags: in-testsuite+
Comment 28•7 years ago
|
||
Verified fixed on 56 Beta 11 with the following OSes: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•