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)

defect
Not set
normal

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)

Looks like there's no way to access these menus with the mouse anymore.
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]
Redirecting the needinfo request to get a quicker response...
Flags: needinfo?(ux-review) → needinfo?(shorlander)
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.
I'll look in to adding these in to the History subview.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
Attached patch Patch (obsolete) — Splinter Review
This is mostly a copy from browser-places.js for the recently closed tabs + windows.
Attachment #815045 - Flags: review?(mconley)
(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?
Comment on attachment 815045 [details] [diff] [review]
Patch

Yeah, I'll look in to that.
Attachment #815045 - Flags: review?(mconley)
Attached patch Patch v2Splinter Review
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)
Comment on attachment 815921 [details] [diff] [review]
Patch v2

>+XPCOMUtils.defineLazyModuleGetter(this, "RecentlyClosedUIUtils",

RecentlyClosedTabsAndWindowsMenuUtils? Just "RecentlyClosed" seems too ambiguous.
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+
Blocks: 926579
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.
No longer blocks: 926579
Depends on: 926579
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]
(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?
Depends on: 926928
(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.
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.
Blocks: 928843
(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.
No longer blocks: 928843
Depends on: 928843
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.

Attachment

General

Created:
Updated:
Size: