592887 - Reorganize History Menu

Status: RESOLVED DUPLICATE of bug 625322

(Firefox :: Menus, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

After a chat with Faaborg, we're thinking it best to shuffle some things around in the History menu.

We would like the menu to contain (as closely as possible) the same entries in the same order as the history menu off the Firefox button. We're also thinking it best to remove the "Tabs from other computers entry" from the history menu.

Bug 588482 is going to add a "Restore Last Session" entry to both menus, so I'll hold off the changes until then.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

A little ascii menu based on what we talked about

History
-------
Back
Forward
Home
-------
Show All History
-------
Clear Recent History (move from tools)
-------
Restore Previous Session
Recently Closed Tabs
Recently Closed Windows
------
the
last
bunch
of
history
entries

Josh - I know you've done a bunch of menu stuff. Not sure if you'd be interested in taking this on. It's mostly just shuffling things around with some removing from browser-places.js as well.

Comment 2

[Joshua M [:soapy]]

Attachment: Wip patch v1

Wip patch, will update when 588482 lands.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> Created attachment 471815 [details] [diff] [review]
> Wip patch v1
> 
> Wip patch, will update when 588482 lands.

Thanks for doing this Josh. I just landed bug 588482.

Might be good to have mconnor & faaborg weigh in here on the need (or lack thereof) for "tabs from other computers".

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

I think it makes the most sense in the tab overflow menu, since that is where tabs live.  History just isn't a good mapping to active tabs.

Comment 5

[Joshua M [:soapy]]

Just wanted to say my wip patch does work on the latest trunk. Can someone give me some feedback on it?

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #5)
> Just wanted to say my wip patch does work on the latest trunk. Can someone give
> me some feedback on it?

Looks like a good start. This doesn't remove the tabs from other computers item & doesn't do the majority of the reordering in the history menu though.

Comment 7

[Joshua M [:soapy]]

Attachment: Patch v1

Removed "sync-tabs-menuitem".

Comment 8

[Joshua M [:soapy]]

Attachment: Screenshot of Patch v1

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #8)
> Created attachment 475261 [details]
> Screenshot of Patch v1

Looks right to me, I'll just poke Alex for ui-r? Also, I think it best to remove the code that's going to enable/disable the "tabs from other computers" from browser-places.js while we're dumping the menuitem (and the code expects getElementById to succeed).

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#739 and the call to it a few lines down.

Comment 10

[Joshua M [:soapy]]

Attachment: Patch v2

Removed some unnecessary code for "tabs from other computers" from browser-places.js.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 475299 [details] [diff] [review]
Patch v2

>+                <menuitem id="sanitizeItem"
>+                          accesskey="&clearRecentHistory.accesskey;"
>+                          label="&clearRecentHistory.label;"
>+                          key="key_sanitize"
>+                          command="Tools:Sanitize"/>
>+                <menuseparator/>

Need to remove the accesskey here.

Can you add ids to the new menuseparators?

Comment 12

[Dão Gottwald [:dao]]

Is "Tabs from other computers" still keyboard-accessible with this patch?

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
> Is "Tabs from other computers" still keyboard-accessible with this patch?

I assume so - it's still in the alltabs-popup. But I don't know much about whether or not that's keyboard-accessible

Comment 14

[Mike Connor [:mconnor]]

It isn't, so this patch makes that feature inaccessible, which sucks a fair bit...

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Filed follow up bug 597308

Comment 16

[Joshua M [:soapy]]

Attachment: Patch v3

Comment 17

[Dão Gottwald [:dao]]

This needs to actually depend on bug 597308, and that needs to be a beta7 blocker if you want this for Firefox 4.

Comment 18

[Dão Gottwald [:dao]]

(Alternatively don't remove "Tabs from other computers" here and take care of it in bug 597308.)

Comment 19

[Joshua M [:soapy]]

Attachment: Patch v4

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 478503 [details] [diff] [review]
Patch v4

>+                <menuseparator id="historyMenuNavSeperator"/>
>+                <menuseparator id="historyMenuAllHistorySeperator"/>
>+                <menuseparator id="historyMenuSanitizeSeperator"/>

The ids seem rather strange. Maybe insert an underscore after historyMenu?

>-                <menuseparator id="startHistorySeparator"/>
>-                <menuseparator id="endHistorySeparator"
>-                               class="hide-if-empty-places-result"
>-                               builder="end"/>

Is builder="end" safe to remove?

>+                <menuseparator id="historyMenuListSeparator"
>+                               class="hide-if-empty-places-result"/>

This should probably be called startHistorySeparator for backward compatibility.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> >+                <menuseparator id="historyMenuNavSeperator"/>
> >+                <menuseparator id="historyMenuAllHistorySeperator"/>
> >+                <menuseparator id="historyMenuSanitizeSeperator"/>
> 
> The ids seem rather strange. Maybe insert an underscore after historyMenu?

That's the way how mostly all other menu items are designed. Would be nice when we could stay consistent at least inside one menu and don't introduce one more different style.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#339

Comment 22

[Steffen Wilberg]

The reordering was done in bug 625322, so duping to that.
Bug 631805 has been filed to move "Clear Recent History" from the Tools menu to the History menu.
Bug 631805 is about Removing "Tabs from Other Computers" from the History menu.