Closed
Bug 888572
Opened 11 years ago
Closed 11 years ago
Recently Closed Tabs / Recently Closed Windows menus inaccessible from the panel menu
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: dao, Assigned: jaws)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [Australis:M9][Australis:P2])
Attachments
(1 file, 1 obsolete file)
17.25 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
Looks like there's no way to access these menus with the mouse anymore.
Comment 1•11 years ago
|
||
On a whim, marking P2 as I really don't think we want to hide this option for mouse users. There's a cumbersome workaround in re-enabling the menubar. Asking for UX feedback because I'm not sure what we want to do here: - Adding it to the history subview would cause sub-subviews, and AFAIK we're explicitly avoiding those, so that doesn't sound like a great idea. - We could make separate buttons and subviews for them, but that also doesn't sound particularly great (what would the icon look like? Should we have the icon in the menupanel by default?) - We could also put them under the all-tabs icon? But that'd require showing that icon even on windows with non-overflowing tabs (if there are closed tabs/windows).
Flags: needinfo?(ux-review)
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
Assignee | ||
Comment 2•11 years ago
|
||
Redirecting the needinfo request to get a quicker response...
Flags: needinfo?(ux-review) → needinfo?(shorlander)
Assignee | ||
Comment 3•11 years ago
|
||
We could also put them in the History subview, but not as a sub-subview. Instead just place them in another section of the History subview. This might make the subview somewhat taller, but I don't see a huge issue coming from that as long as it will still fit on the screen.
Assignee | ||
Comment 4•11 years ago
|
||
I'll look in to adding these in to the History subview.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(shorlander)
Assignee | ||
Comment 5•11 years ago
|
||
This is mostly a copy from browser-places.js for the recently closed tabs + windows.
Attachment #815045 -
Flags: review?(mconley)
Comment 7•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #5) > This is mostly a copy from browser-places.js for the recently closed tabs + > windows. That's a lot of code to duplicate :\ Can't we at least spin that out to a JSM or something?
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 815045 [details] [diff] [review] Patch Yeah, I'll look in to that.
Attachment #815045 -
Flags: review?(mconley)
Assignee | ||
Comment 9•11 years ago
|
||
Moved the code to a new module, could use some suggestions on the names of the module and its methods.
Attachment #815045 -
Attachment is obsolete: true
Attachment #815921 -
Flags: review?(mconley)
Attachment #815921 -
Flags: review?(bmcbride)
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 815921 [details] [diff] [review] Patch v2 >+XPCOMUtils.defineLazyModuleGetter(this, "RecentlyClosedUIUtils", RecentlyClosedTabsAndWindowsMenuUtils? Just "RecentlyClosed" seems too ambiguous.
Comment 11•11 years ago
|
||
Comment on attachment 815921 [details] [diff] [review] Patch v2 Review of attachment 815921 [details] [diff] [review]: ----------------------------------------------------------------- r+, but there's a bunch of renaming nits. Up to you whether you want to have me have another look when done. ::: browser/components/customizableui/content/panelUI.inc.xul @@ +47,5 @@ > + <toolbarseparator id="PanelUI-recentlyClosedTabs-separator"/> > + <vbox id="PanelUI-recentlyClosedTabs"/> > + <toolbarseparator id="PanelUI-recentlyClosedWindows-separator"/> > + <vbox id="PanelUI-recentlyClosedWindows"/> > + <toolbarseparator/> this guy feels a little unimportant without an id. Should we give it one? (I'm not sure why it doesn't have one, while the others do) ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +100,5 @@ > + > + let recentlyClosedTabs = doc.getElementById("PanelUI-recentlyClosedTabs"); > + while (recentlyClosedTabs.firstChild) { > + recentlyClosedTabs.removeChild(recentlyClosedTabs.firstChild); > + } I wish we had a better way of doing this. recentlyClosedTabs.innerHTML = '' might work, but makes me feel terrible. @@ +110,5 @@ > + > + let recentlyClosedWindows = doc.getElementById("PanelUI-recentlyClosedWindows"); > + while (recentlyClosedWindows.firstChild) { > + recentlyClosedWindows.removeChild(recentlyClosedWindows.firstChild); > + } Move this up together with the other removal code, perhaps? ::: browser/components/sessionstore/src/RecentlyClosedUIUtils.jsm @@ +35,5 @@ > + let list = doc.createDocumentFragment(); > + if (ss.getClosedTabCount(aWindow) != 0) { > + let items = JSON.parse(ss.getClosedTabData(aWindow)); > + for (let i = 0; i < items.length; i++) { > + let item = doc.createElementNS(kNSXUL, aTagName); Please don't iterate over an array of 'items', and then have a local variable called 'item' that *isn't* items[i]. Call it element, or node, or menuItem, but not item. And call the items array "closedTabs" or something. @@ +80,5 @@ > + * @param aTagName > + * The tag name that will be used when creating the UI items. > + * @returns A document fragment with UI items for each recently closed window. > + */ > + getWindowsFragment: function(aWindow, aTagName) { This is almost exactly the above, except s/Tab/Window/. If you don't want to improve that here, can you file a followup to do it? ::: browser/components/sessionstore/src/moz.build @@ +16,5 @@ > 'DocShellCapabilities.jsm', > 'DocumentUtils.jsm', > 'Messenger.jsm', > 'PrivacyLevel.jsm', > + 'RecentlyClosedUIUtils.jsm', I agree with Dão that the naming could be better.
Attachment #815921 -
Flags: review?(mconley)
Attachment #815921 -
Flags: review?(bmcbride)
Attachment #815921 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/9737cc4b6837 (In reply to :Gijs Kruitbosch from comment #11) > This is almost exactly the above, except s/Tab/Window/. If you don't want to > improve that here, can you file a followup to do it? Filed bug 926579 as a follow-up to reduce the duplication.
Comment 13•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #12) > https://hg.mozilla.org/projects/ux/rev/9737cc4b6837 > > (In reply to :Gijs Kruitbosch from comment #11) > > This is almost exactly the above, except s/Tab/Window/. If you don't want to > > improve that here, can you file a followup to do it? > > Filed bug 926579 as a follow-up to reduce the duplication. Ah, and I forgot to say... could you land the refactoring part of this (ie patch minus customizableui parts) on m-c as well?
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #13) > (In reply to Jared Wein [:jaws] from comment #12) > > https://hg.mozilla.org/projects/ux/rev/9737cc4b6837 > > > > (In reply to :Gijs Kruitbosch from comment #11) > > > This is almost exactly the above, except s/Tab/Window/. If you don't want to > > > improve that here, can you file a followup to do it? > > > > Filed bug 926579 as a follow-up to reduce the duplication. > > Ah, and I forgot to say... could you land the refactoring part of this (ie > patch minus customizableui parts) on m-c as well? Yes, this was tracked in bug 926928.
Comment 15•11 years ago
|
||
Would it be possible to add some (sub)section headers to the view? It now looks funny with some menu items, page titles, more menu items, page titles - only structured with separators. It's hard to make sense of it all.
Comment 16•11 years ago
|
||
(In reply to Thomas Stache from comment #15) > Would it be possible to add some (sub)section headers to the view? It now > looks funny with some menu items, page titles, more menu items, page titles > - only structured with separators. It's hard to make sense of it all. Filed as bug 928843.
Updated•11 years ago
|
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9737cc4b6837
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•