571752 - Add History to the Firefox button menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dão Gottwald [:dao]]

History and Bookmarks aren't in the Firefox button menu yet and need to be added. I think Bookmarks was actually intended to be left out in favor of the bookmarks button. I don't know if there was an intentional change of course.

Comment 1

[Peter Henkel [:Terepin]]

Perhaps Bookmarks button if managing and Firefox menu is for displaying them?

Comment 2

[tymerkaev]

Maybe current Firefox button should be show current content of menu bar?

Comment 3

[Marco Bonardo [:mak]]

yeah, bookmarks entry does not look really useful once the bookmarks button comes to life. And I agree it should be left out.
History is 2 clicks deep in this design, while is 1 click deep currently, I was somehow liking the double pan menu with history in the right part of strata40.

Comment 4

[Marco Bonardo [:mak]]

I guess somebody has to take this, assigning to me

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This adds the History menu, for now I've not added any icon since no item in the menu has icons, they can be easily added later, for now the goal seems to be functionality.
I've not added the bookmarks menu for the reasoning in previous comments, bookmarks menu button patch is imo in a good enough shape to be reviewed and landed.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 451060 [details] [diff] [review]
patch v1.0

>-                <menuseparator id="endHistorySeparator" builder="end"/>
>+                <menuseparator id="endHistorySeparator"
>+                               class="hideifemptyplacesresult"
>+                               builder="end"/>

Underscores and hyphens are pretty common in classes, so I'd suggest something like hide-if-empty-places-result, to make this more easily readable.

>+        <menu id="appmenu_history"
>+              oncommand="this._placesView._onCommand(event);"
>+              onclick="checkForMiddleClick(this, event);"
>+              label="&historyMenu.label;">
>+          <menupopup id="appmenu_history_popup"
>+                     placespopup="true"
>+                     onpopupshowing="if (!document.getElementById('appmenu_history')._placesView)
>+                                       new HistoryMenu(event);"
>+                     tooltip="bhTooltip"
>+                     popupsinherittooltip="true">

You could use this.parentNode instead of document.getElementById('appmenu_history').

Wouldn't the oncommand and onclick attributes make more sense on the menupopup node?

Otherwise looks good to me, although I'd prefer someone who knows more about places to sign off on this.

I don't think ui-review is needed here.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.1

makes sense, addressed comments.

The only other one who could sign-off this properly in our team is Mano, but I see him rarely these days. That said these changes are mostly boilerplate and I don't think they require all those checks. Even if I try to don't introduce regressions as a primary target, there is nothing we can't do to be 100% safe in any case.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 451251 [details] [diff] [review]
patch v1.1

>+            <menuitem id="appmenu_sanitizeHistory"
>+                      accesskey="&clearRecentHistory.accesskey;"

I don't think we want the access key here, as the button is only accessible with the mouse.

Comment 9

[Marco Bonardo [:mak]]

with removed accesskey
http://hg.mozilla.org/mozilla-central/rev/2e666a1a2c7e

Comment 10

[Josh Tumath]

What about the recently closed tabs feature?

Comment 11

[Marco Bonardo [:mak]]

tabs management entries should go to tabs menus as far as I know (not sure if contextual menus or all tabs, but they are not part of History).