Closed
Bug 863299
Opened 10 years ago
Closed 10 years ago
Drop support for small icons mode in the toolbars
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat, Whiteboard: [Australis:M7])
Attachments
(1 file, 4 obsolete files)
34.85 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
For Australis, we are going to simplify our user interface so we can focus more of our time implementing core features. Small icons mode is not the default view and would be fulfilled better if it was implemented as an optional theme. The attached patch removes all references to small icons mode that I could find, but I'm not sure if we need to change http://hg.mozilla.org/mozilla-central/annotate/ef0432d35332/browser/base/content/browser.js#l3451 or keep it? Also, do we need to keep smallicons mode for fullscreen, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js#519 ?
Attachment #739109 -
Flags: review?(mnoorenberghe+bmo)
Comment 1•10 years ago
|
||
You completely removed @navbarLargeIcons@, but all those selectors were not only large-icon mode specific but also nav-bar specific. Same goes for '#navigator-toolbox[iconsize="large"] > #nav-bar' which you removed rather than replacing it with '#nav-bar'. Some of your JS changes are about the toolbar mode rather than icon size. General note: All the stuff supporting small icons mode seems pretty straightforward and mechanic. I don't think removing it frees up resources significantly.
Comment 2•10 years ago
|
||
+++ b/toolkit/content/customizeToolbar.js - let modeList = document.getElementById("modelist"); - let label = document.getElementById("modelistLabel"); - label.hidden = true; - modeList.hidden = true; + for (let id of ["modelist", "modelistLabel", "smallicons"]) { + let element = document.getElementById(id); + element.hidden = true; + } It looks like this hides this IDs for all applications like Thunderbird and Seamonkey too, is this true? Please can't you add rules in content/browser.css to hide them? Or better would be in skin/browser.css then third party themes can still support the modes they want.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Richard Marti [:Paenglab] from comment #2) > +++ b/toolkit/content/customizeToolbar.js > - let modeList = document.getElementById("modelist"); > - let label = document.getElementById("modelistLabel"); > - label.hidden = true; > - modeList.hidden = true; > + for (let id of ["modelist", "modelistLabel", "smallicons"]) { > + let element = document.getElementById(id); > + element.hidden = true; > + } > > It looks like this hides this IDs for all applications like Thunderbird and > Seamonkey too, is this true? > > Please can't you add rules in content/browser.css to hide them? Or better > would be in skin/browser.css then third party themes can still support the > modes they want. Please disregard this change, it was supposed to be reverted but it slipped by.
Assignee | ||
Updated•10 years ago
|
Attachment #739109 -
Attachment is obsolete: true
Attachment #739109 -
Flags: review?(mnoorenberghe+bmo)
Updated•10 years ago
|
Blocks: australis-cust
Assignee | ||
Comment 4•10 years ago
|
||
Rebased and fixed the issues that Dao mentioned.
Attachment #754999 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #754999 -
Flags: review?(felipc)
Assignee | ||
Updated•10 years ago
|
Attachment #754999 -
Flags: review?(felipc) → review?(dao)
Comment 6•10 years ago
|
||
Comment on attachment 754999 [details] [diff] [review] Patch v1.1 Review of attachment 754999 [details] [diff] [review]: ----------------------------------------------------------------- I'm not comfortable reviewing the JS changes as I don't have experience with this code and haven't even read/followed bugs where other people are touching the modes. I didn't see obvious issues with the CSS but I was surprised that this doesn't remove much except for on Linux. ::: browser/base/content/browser-social.js @@ -814,5 @@ > iframe.setAttribute("origin", provider.origin); > iframe.setAttribute("src", shareEndpoint); > > let navBar = document.getElementById("nav-bar"); > - let anchor = navBar.getAttribute("mode") == "text" ? Is this change something that was missed in bug 573329? I'm not that familiar with how small icons and text/icons+text modes interact tbh. ::: browser/base/content/browser.js @@ +3300,5 @@ > > function BrowserToolboxCustomizeChange(aType) { > + gHomeButton.updatePersonalToolbarStyle(); > + BookmarksMenuButton.customizeChange(); > + allTabs.readPref(); Where did allTabs.readPref() come from? ::: browser/components/nsBrowserGlue.js @@ +1370,5 @@ > let modeResource = this._rdf.GetResource("mode"); > let iconsizeResource = this._rdf.GetResource("iconsize"); > for (let toolbarResource of toolbarResources) { > + this._setPersist(toolbarResource, modeResource, "icons"); > + this._setPersist(toolbarResource, iconsizeResource, "large"); Is the reason we are setting values rather than clearing them because we aren't removing the toolkit capability to set the mode and the default is something other than large? IIUC the default is large. Does some other code take care of not persisting default values ala pref service? Perhaps this doesn't matter in practice but it seems nicer to work like prefs so that changing the default changes the value for existing users with the old default. I'm not very familiar with how persistence works. @@ +1385,5 @@ > } > > + if (currentUIVersion < 15) { > + > + } I guess this isn't needed?
Attachment #754999 -
Flags: review?(mnoorenberghe+bmo) → feedback+
Updated•10 years ago
|
Whiteboard: [Australis:M6] → [Australis:M7]
Assignee | ||
Updated•10 years ago
|
Attachment #754999 -
Flags: review?(dao)
Assignee | ||
Comment 7•10 years ago
|
||
I moved the changes in browser-social.js to a separate bug, bug 881385. Thanks for the feedback. I checked my localstore.rdf and for custom toolbars we only store the collapsed, so this patch now only sets the "large" or "icons" if the toolbar had something different. I suppose we could also clear the attribute, although I don't see that being done elsewhere in migrateUI. I've made the changes you requested and will request review from Blair.
Attachment #754999 -
Attachment is obsolete: true
Attachment #760523 -
Flags: review?(bmcbride)
Comment 8•10 years ago
|
||
Comment on attachment 760523 [details] [diff] [review] Patch v1.2 Review of attachment 760523 [details] [diff] [review]: ----------------------------------------------------------------- Only looked at the JS, as MattN looked at the CSS. ::: browser/components/nsBrowserGlue.js @@ +1383,4 @@ > this._setPersist(toolbarResource, modeResource, "icons"); > + } > + let iconsize = this._getPersist(toolbarResource, iconsizeResource); > + if (iconsize && iconsize != "large") { This seems to be incorrect. With the old customizeToolbar.js, the mode was locked for TabsToolbar (via the lockmode attribute) - it couldn't be changed. So toolbarMode was guaranteed to be "icons" for that toolbar, therefore the iconsize was never updated to "large". But, with this change, that toolbar will get iconsize set to "large" - which we don't want.
Attachment #760523 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 9•10 years ago
|
||
Huh, I didn't know about lockmode or lockiconsize before. There doesn't appear to be any reference to these attributes outside of browser.xul. It should also be noted that TabsToolbar isn't modified as it's not in the above array. I did notice that lockiconsize is used for PersonalToolbar.
Attachment #760523 -
Attachment is obsolete: true
Attachment #760722 -
Flags: review?(bmcbride)
Comment 10•10 years ago
|
||
Comment on attachment 760722 [details] [diff] [review] Patch v1.3 Review of attachment 760722 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +1380,2 @@ > let iconsizeResource = this._rdf.GetResource("iconsize"); > + let lockiconsizeResource = this._rdf.GetResource("lockiconsize"); Er, I don't think the lock* attributes are persisted, are they? They're not in my localstore.rdf, and it didn't seem them get persisted in customizeToolbar.js (relevant code is in updateToolboxProperty, fwiw).
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #10) > Comment on attachment 760722 [details] [diff] [review] > Patch v1.3 > > Review of attachment 760722 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/nsBrowserGlue.js > @@ +1380,2 @@ > > let iconsizeResource = this._rdf.GetResource("iconsize"); > > + let lockiconsizeResource = this._rdf.GetResource("lockiconsize"); > > Er, I don't think the lock* attributes are persisted, are they? They're not > in my localstore.rdf, and it didn't seem them get persisted in > customizeToolbar.js (relevant code is in updateToolboxProperty, fwiw). Thanks for pointing out updateToolboxProperty, the |"lock" + aProp| made it a lot harder to find. But this makes it a bit harder, since during migrateUI we can't call getMostRecentWindow since it runs before any window has been opened. What do you suppose I do here?
Flags: needinfo?(bmcbride)
Assignee | ||
Comment 12•10 years ago
|
||
I have a patch coming that will work around this issue.
Flags: needinfo?(bmcbride)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #760722 -
Attachment is obsolete: true
Attachment #760722 -
Flags: review?(bmcbride)
Attachment #760978 -
Flags: review?(bmcbride)
Comment 14•10 years ago
|
||
Comment on attachment 760978 [details] [diff] [review] Patch v1.4 Review of attachment 760978 [details] [diff] [review]: ----------------------------------------------------------------- That works :)
Attachment #760978 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/d34b32544b06
Whiteboard: [Australis:M7] → [Australis:M7][fixed-in-ux]
Updated•10 years ago
|
Keywords: addon-compat
Updated•10 years ago
|
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d34b32544b06
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M7][fixed-in-ux] → [Australis:M7]
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•