Closed Bug 1389939 Opened 3 years ago Closed 3 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)

57 Branch
defect

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)

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.
Will provide regression as soon as posible.
Attached image Theme_change.gif
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.
Has Regression Range: --- → yes
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
IMO we should stop the syncing now that it's exposed to non-developers.
(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.
(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.
Priority: -- → P2
(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)
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?
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?
Track 56+ as regression.
(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)
Depends on: 1370635
Depends on: 1380703
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)
Flags: needinfo?(poirot.alex)
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: nobody → bgrinstead
Status: NEW → ASSIGNED
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+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eca0163d0964
Stop syncing devtools theme and compact themes;r=Gijs
Duplicate of this bug: 1370635
https://hg.mozilla.org/mozilla-central/rev/eca0163d0964
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(bgrinstead)
Depends on: 1394315
Keywords: verifyme
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?
Andrei, can your team take a look and verify the fix? Thanks.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
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 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+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
No longer depends on: 1403543
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.