Closed
Bug 1093790
Opened 10 years ago
Closed 10 years ago
Use light full screen button for devedition theme
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 36
People
(Reporter: bgrins, Assigned: bgrins)
References
Details
(Whiteboard: [devedition-polish])
Attachments
(3 files, 3 obsolete files)
8.43 KB,
image/png
|
Details | |
8.90 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
9.70 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
Thanks to bug 1018845, we just need to add a brighttitlebarforeground attribute to the window when the devedition theme is applied.
Updated•10 years ago
|
Group: mozilla-employee-confidential
Assignee | ||
Comment 1•10 years ago
|
||
I'm actually waiting for 1018845 to get merged into fx-team before I can confirm it is working and looking alright, but here are the code changes I think we will need to add the brighttitlebarforeground attribute (and thus get the light fullscreen button on osx). Pushed to try on based on fx-team: https://tbpl.mozilla.org/?tree=Try&rev=278917f158dc Pushed to try on based on gum: https://tbpl.mozilla.org/?tree=Try&rev=ca76e7bed506
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8517016 -
Flags: review?(gijskruitbosch+bugs)
Comment 2•10 years ago
|
||
Comment on attachment 8517016 [details] [diff] [review] brighttitlebarforeground.patch Review of attachment 8517016 [details] [diff] [review]: ----------------------------------------------------------------- Some small issues, but probably makes sense to look at this again also to see if it actually works (and passes in a try push, maybe? Maybe that's overkill...) ::: browser/base/content/browser-devedition.js @@ +85,5 @@ > if (e.type === "load") { > this.styleSheet.removeEventListener("load", this); > gBrowser.tabContainer._positionPinnedTabs(); > ToolbarIconColor.inferFromText(); > + document.documentElement.setAttribute("brighttitlebarforeground", ""); Nit: set to "true" by convention? (we do this for e.g. drawtitle as well) Also... don't we only need to do this on OS X? ::: browser/base/content/test/general/browser_devedition.js @@ +33,5 @@ > > info ("Removing a lightweight theme."); > Services.prefs.setBoolPref(PREF_LWTHEME, false); > ok (DevEdition.styleSheet, "The devedition stylesheet has been added when a lightweight theme is removed."); > + let onLoaded = Promise.defer(); Please use new DOM promises instead of old-style Promise.jsm: yield new Promise((resolve, reject) => { // do load handler, call resolve from load handler. }); @@ +44,5 @@ > + info ("Waiting for DevEdition.styleSheet load event"); > + yield onLoaded.promise; > + > + ok (document.documentElement.hasAttribute("brighttitlebarforeground"), > + "The brighttitlebarforeground attribute is not set on the window."); This message doesn't tally with the actual test...
Attachment #8517016 -
Flags: review?(gijskruitbosch+bugs)
Comment 3•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > ::: browser/base/content/browser-devedition.js > @@ +85,5 @@ > > if (e.type === "load") { > > this.styleSheet.removeEventListener("load", this); > > gBrowser.tabContainer._positionPinnedTabs(); > > ToolbarIconColor.inferFromText(); > > + document.documentElement.setAttribute("brighttitlebarforeground", ""); > > Nit: set to "true" by convention? (we do this for e.g. drawtitle as well) It really needs to be set to true, otherwise it won't do anything.
Assignee | ||
Comment 4•10 years ago
|
||
Addresses feedback - pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dcf9fda47bdf
Attachment #8517016 -
Attachment is obsolete: true
Attachment #8517548 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 5•10 years ago
|
||
Markus, with the most recent patch from the bug applied, here is what I see upon initial startup if devedition.theme.enabled is set to true. I see the same thing when switching between a lw theme and the devedition theme. If I hover the button, refocus the window, or set the attribute in a timeout instead of immediately in the load event, things go back to normal. I'm not sure if it's something weird just with my computer or what is going on - any ideas?
Flags: needinfo?(mstange)
Comment 7•10 years ago
|
||
Comment on attachment 8517548 [details] [diff] [review] brighttitlebarforeground.patch Review of attachment 8517548 [details] [diff] [review]: ----------------------------------------------------------------- Code wise, this looks fine. We should sort out the issue from comment 5 before landing though... ::: browser/base/content/test/general/browser_devedition.js @@ +44,5 @@ > + }); > + }); > + > + is (document.documentElement.getAttribute("brighttitlebarforeground"), "true", > + "The brighttitlebarforeground attribute is set on the window."); Nit: line this up with the opening brace on the previous line, please. @@ +50,5 @@ > info ("Setting browser.devedition.theme.enabled to false."); > Services.prefs.setBoolPref(PREF_DEVEDITION_THEME, false); > ok (!DevEdition.styleSheet, "The devedition stylesheet has been removed."); > + ok (!document.documentElement.hasAttribute("brighttitlebarforeground"), > + "The brighttitlebarforeground attribute is not set on the window."); Ditto.
Attachment #8517548 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 8•10 years ago
|
||
I filed bug 1095210 with a patch for the issue you found. It was a pre-existing bug but unluckily changing the fullscreen button's color made it much easier to trigger.
Assignee | ||
Updated•10 years ago
|
Group: mozilla-employee-confidential
Assignee | ||
Updated•10 years ago
|
Whiteboard: [devedition-polish]
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #8) > I filed bug 1095210 with a patch for the issue you found. It was a > pre-existing bug but unluckily changing the fullscreen button's color made > it much easier to trigger. Thanks Markus! This is working as expected now.
Whiteboard: [devedition-polish] → [fixed-in-fx-team][devedition-polish]
Assignee | ||
Comment 10•10 years ago
|
||
Re-requesting review because I had to change this a bit to accommodate the light version of the devedition theme. It's mostly the same logic though. Here's a try push: https://tbpl.mozilla.org/?tree=Try&rev=98ea47e10a4a
Attachment #8517548 -
Attachment is obsolete: true
Attachment #8521071 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-in-fx-team][devedition-polish] → [devedition-polish]
Comment 11•10 years ago
|
||
Comment on attachment 8521071 [details] [diff] [review] fullscreen-attribute.patch Review of attachment 8521071 [details] [diff] [review]: ----------------------------------------------------------------- The test is breaking on try... ::: browser/base/content/browser-devedition.js @@ +52,5 @@ > > + _inferBrightness: function() { > + ToolbarIconColor.inferFromText(); > + if (document.querySelector("#TabsToolbar").hasAttribute("brighttext")) { > + document.documentElement.setAttribute("brighttitlebarforeground", "true"); This should only be the case if tabsintitlebar is enabled.
Attachment #8521071 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 12•10 years ago
|
||
This simplifies the decision for when to set the attribute. Basically, it only adds it if the theme is applied and if it's dark. The other case when it should be inverted is a dark lw theme, and in this case the attribute doesn't need to be set. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fb9c1e81d653
Attachment #8521071 -
Attachment is obsolete: true
Attachment #8521836 -
Flags: review?(gijskruitbosch+bugs)
Comment 13•10 years ago
|
||
Comment on attachment 8521836 [details] [diff] [review] fullscreen-attribute.patch Review of attachment 8521836 [details] [diff] [review]: ----------------------------------------------------------------- r=me assuming green try There's a bunch of 1am-oh-dear-why-am-I-still-here observations below. I hope they're not 100% wrong. You may or may not want to take them into account. ::: browser/base/content/test/general/browser_devedition.js @@ +35,5 @@ > Services.prefs.setBoolPref(PREF_LWTHEME, false); > ok (DevEdition.styleSheet, "The devedition stylesheet has been added when a lightweight theme is removed."); > > + info ("Waiting for DevEdition.styleSheet load event"); > + yield new Promise((resolve, reject) => { Hrmpf. Assuming that load events will never fire sync (which is an interesting assumption), this works. That is, AIUI: new Promise(fn) calls fn sync new Promise is called sync here after you set the pref, which will have invoked the pref observer and created DevEdition.styleSheet. Therefore, the load is guaranteed to also hit this event listener. If the load is somehow not async, you lose. Slightly less tricky would be to use a mutation observer to wait for the right attribute to be set here. Your choice. :-) @@ +51,5 @@ > Services.prefs.setBoolPref(PREF_DEVEDITION_THEME, false); > ok (!DevEdition.styleSheet, "The devedition stylesheet has been removed."); > > + ok (!document.documentElement.hasAttribute("brighttitlebarforeground"), > + "The brighttitlebarforeground attribute is not set on the window."); It'd be kind of nice if these things didn't have the same message repeated so failures were immediately identifiable. I'm actually not sure where the previous try push failed (I could maybe figure it out by looking at the long log and going through the test, but it's much easier if assertion messages are unique). @@ +65,5 @@ > Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "light"); > is (document.documentElement.getAttribute("devtoolstheme"), "light", > "The documentElement has an attribute based on devtools theme."); > ok (DevEdition.styleSheet, "The devedition stylesheet is still there with the light devtools theme."); > + ok (!document.documentElement.hasAttribute("brighttitlebarforeground"), This test is not very reliable this way. The current sequence of events is: 1) set dev edition theme pref (triggers load of stylesheet, we're using the dark theme) 2) set color pref to light (sync-updates the attribute on the window) 3) check for attribute on window x) ... the stylesheet loads and also triggers an update of the attribute on the window. There's no telling where x happens, and no assertions as to behaviour either way. :-) We never check the fact that enabling the devtools theme should be setting the attribute on the window in here, and doing so would require the same load checks the previous function uses, which then makes me wonder if that check shouldn't be in here instead.
Attachment #8521836 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #13) > Comment on attachment 8521836 [details] [diff] [review] > fullscreen-attribute.patch > > Review of attachment 8521836 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me assuming green try > > There's a bunch of 1am-oh-dear-why-am-I-still-here observations below. I > hope they're not 100% wrong. You may or may not want to take them into > account. > > ::: browser/base/content/test/general/browser_devedition.js > @@ +35,5 @@ > > Services.prefs.setBoolPref(PREF_LWTHEME, false); > > ok (DevEdition.styleSheet, "The devedition stylesheet has been added when a lightweight theme is removed."); > > > > + info ("Waiting for DevEdition.styleSheet load event"); > > + yield new Promise((resolve, reject) => { > > Hrmpf. Assuming that load events will never fire sync (which is an > interesting assumption), this works. That is, AIUI: > > new Promise(fn) calls fn sync > > new Promise is called sync here after you set the pref, which will have > invoked the pref observer and created DevEdition.styleSheet. > > Therefore, the load is guaranteed to also hit this event listener. > > If the load is somehow not async, you lose. > > Slightly less tricky would be to use a mutation observer to wait for the > right attribute to be set here. Your choice. :-) I would think if the load could sometimes fire sync that it would always and this would be a perma orange. But I don't know whether that is something that could intermittently happen. I'll switch to a mutation observer and use it in the assertions below also. > > @@ +51,5 @@ > > Services.prefs.setBoolPref(PREF_DEVEDITION_THEME, false); > > ok (!DevEdition.styleSheet, "The devedition stylesheet has been removed."); > > > > + ok (!document.documentElement.hasAttribute("brighttitlebarforeground"), > > + "The brighttitlebarforeground attribute is not set on the window."); > > It'd be kind of nice if these things didn't have the same message repeated > so failures were immediately identifiable. I'm actually not sure where the > previous try push failed (I could maybe figure it out by looking at the long > log and going through the test, but it's much easier if assertion messages > are unique). > Yeah, updated the messages > @@ +65,5 @@ > > Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "light"); > > is (document.documentElement.getAttribute("devtoolstheme"), "light", > > "The documentElement has an attribute based on devtools theme."); > > ok (DevEdition.styleSheet, "The devedition stylesheet is still there with the light devtools theme."); > > + ok (!document.documentElement.hasAttribute("brighttitlebarforeground"), > > This test is not very reliable this way. The current sequence of events is: > > 1) set dev edition theme pref (triggers load of stylesheet, we're using the > dark theme) > 2) set color pref to light (sync-updates the attribute on the window) > 3) check for attribute on window > > x) ... the stylesheet loads and also triggers an update of the attribute on > the window. > > There's no telling where x happens, and no assertions as to behaviour either > way. :-) > > We never check the fact that enabling the devtools theme should be setting > the attribute on the window in here, and doing so would require the same > load checks the previous function uses, which then makes me wonder if that > check shouldn't be in here instead. I think I'm following here - I used the mutation observer to wait for the attribute to show up and check that before proceeding into the other devtools theme checks. https://tbpl.mozilla.org/?tree=Try&rev=1ebf56109013
Attachment #8523211 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f4d12173f912
Keywords: checkin-needed
Whiteboard: [devedition-polish] → [devedition-polish][fixed-in-fx-team]
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f4d12173f912
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [devedition-polish][fixed-in-fx-team] → [devedition-polish]
Target Milestone: --- → Firefox 36
You need to log in
before you can comment on or make changes to this bug.
Description
•