603605 - BMB_bookmarkThisPage is indented in the bookmarks button menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre

For some reason appmenu_bookmarkThisPage, i.e., "Bookmark This Page" is tab indented in the context menu, where other items such as appmenu_subscribeToPage, i.e., "Subscribe to This Page" are not. If this is a matter of emphasis on the current page, than both menu items should either be indented or not. As it stands, having a menu item indented simply looks like a bug.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#633

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Context menu indentation

Comment 2

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

Also a problem with the bookmarks menu toolbar button

Comment 3

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

(In reply to comment #0)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#633

Ah, you're linking to appmenu_bookmarkThisPage, which is part of the app menu. That's Windows only & might need to have menuitem-iconic.

The actual problem here is BMB_bookmarkThisPage:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1027

Comment 4

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

Attachment: Patch v0.1

remove menuitem-iconic from BMB_bookmarkThisPage

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 483244 [details] [diff] [review]
Patch v0.1

Doesn't this break <http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#289>?

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> <http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#289>?

Not sure why the icon is there in the first place, I'd be okay with removing it.

Comment 7

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

Looking at it on Windows, It actually looks pretty good and fits in with the rest of the menu (assuming subscribe gets an icon too). In fact I sort of wish it reflected the current bookmark state (filled star, labeled "edit this bookmark") but that's mostly a different issue.

It works on windows because the menus naturally have the space for icons and line up regardless of the existence of the icon

(In reply to comment #6)
> (In reply to comment #5)
> > <http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#289>?
> 
> Not sure why the icon is there in the first place, I'd be okay with removing
> it.

Looks to be have been part of bug 592900. It looks like the change wasn't meant to affect other platforms, so just putting an #ifdef around the class seems like the most appropriate thing.

Comment 8

[Dão Gottwald [:dao]]

Alex, can you explain why "Bookmark This Page" has an icon on Windows? As I understand it, menus on Windows aren't expected to have icons. Exceptions are selected top-level app menu items and bookmark items, neither of which applies here.

Also, I'm not sure why "Bookmark This Page" exists in the bookmarks button menu at all. The primary purpose of this button is accessing bookmarks, I think, so other items which aren't totally needed seem counter-productive. We already the star in the Location bar.

Comment 9

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

>Exceptions are selected top-level app menu items

Since this menu also appears on the top level when accessed from the navigation toolbar, it sort of half meets that requirement.

>Also, I'm not sure why "Bookmark This Page" exists in the bookmarks button menu
>at all. The primary purpose of this button is accessing bookmarks, I think, so
>other items which aren't totally needed seem counter-productive. We already the
>star in the Location bar.

Usage of the star icon in the location bar was really low when we ran the test pilot study.  It looks like a lot of people are avoiding it, either out of force of habit or because they don't realize that it can be used to create bookmarks.  By using the same icon in the menu, the hope was that they would mentally associate the two, and realize that there is a shorter path in primary UI.

An additional issue is that we are moving subscribe into the bookmarks menu to get it out of primary UI, and it seems odd to give the user a way to subscribe to the page through the menu without also providing a way for them to create a bookmark.  I agree that the top of the bookmarks menu seems a bit heavy right now with 4 items.  But overall I think keeping the redundancy of Bookmark this page will help users who are already a little frustrated with the transition from the traditional menu bar to the Firefox button.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> >Exceptions are selected top-level app menu items
> 
> Since this menu also appears on the top level when accessed from the navigation
> toolbar, it sort of half meets that requirement.

I thought this was an exception for the app menu rather than a new general rule.
So "Bookmark this page" is really supposed to be a prominent menu item? (But only on Windows?)

> Usage of the star icon in the location bar was really low when we ran the test
> pilot study.  It looks like a lot of people are avoiding it, either out of
> force of habit or because they don't realize that it can be used to create
> bookmarks.  By using the same icon in the menu, the hope was that they would
> mentally associate the two, and realize that there is a shorter path in primary
> UI.

Has the usage been put in perspective? It seems understandable that bookmarks aren't created nearly as often as new tabs are opened or pages are reloaded. How popular is the star compared to other methods of creating bookmarks?

Could it be that people don't like that the star bookmarks pages immediately without letting them select a folder? I suppose we could change that.

Comment 11

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

>So "Bookmark this page" is really supposed to be a prominent menu item? (But
>only on Windows?)

It's a prominent menu item in the bookmarks menu on XP, OS X and Linux (which is why some users are avoiding the star).  We generally don't want redundancy between the menus (Firefox, bookmark) and the primary UI.  For instance, we removed items like Back, Forward, Home, Web search, etc.  However we made two exceptions based on usage, creating a new tab and creating a new bookmark.

>Has the usage been put in perspective?

Unfortunately the data comes from two different studies, looking at the usage of the menu bar item versus the star in the location bar.  As new data comes in we'll be able to figure out if some users are still refusing to use the star in the location bar in favor of the menu item. 

>Could it be that people don't like that the star bookmarks pages immediately
>without letting them select a folder? I suppose we could change that.

I think it is more likely that they just have a built up path of clicking on the bookmarks menu and then on bookmark this page.  For instance, people used to use the menu to create a new tab because that was the only option (with the mouse), and introducing a new tab button in primary UI didn't seem to reduce the usage of the new tab menu item.  So the theory is simply that habits are hard for users to break.  The fact that we are removing the traditional menu bar entirely though means that we might start to see some uptake of the primary UI elements for new tab and bookmarking as people take a moment to get accustomed to the new UI.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> >So "Bookmark this page" is really supposed to be a prominent menu item? (But
> >only on Windows?)
> 
> It's a prominent menu item in the bookmarks menu on XP, OS X and Linux (which
> is why some users are avoiding the star).

Is it an oversight then that the menu item doesn't have an icon on OS X and Linux?

Comment 13

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

Oversight on Linux, OS X is considerably more strict about banning icons for menu items (usually only used for locations that the user can go to, like favicons for bookmarks, or various destinations in the Finder's go menu).  Windows allows icons for the most used items.  Some Linux apps use icons for everything, but I think it is a better strategy to only use them for the most used items, similar to Windows.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> Oversight on Linux, OS X is considerably more strict about banning icons for
> menu items (usually only used for locations that the user can go to, like
> favicons for bookmarks, or various destinations in the Finder's go menu). 
> Windows allows icons for the most used items.

Are you sure about Windows? My Windows XP and 7 experience seems to match your description for OS X, except for the start menu and the large app button menus (which is why the Firefox button menu as an exception made sense to me). We don't have a single icon in the menu bar except for History and Bookmarks either.

Comment 15

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

yeah, their HIG points out that using icons only for high usage items makes them easier to spot.  We see this in explorer for some items under the organize menu (cut/copy/paste, layout and delete), as well as only some items on the toolbar itself (open).  In context menus on 7 we see a similar usage, for instance empty recycle bin gets an icon on the recycle bin's context menu.  When we have usage data for our context menus I would eventually like for us to give a few of the very high usage items icons.

Comment 16

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

So to summarize: We should be showing the icon on Windows / Linux, but not on OS X?

Comment 17

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

yep

Comment 18

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

Attachment: Patch v0.2

This definitely fixes the OSX problem. I think it will add the icon on Linux, but I don't have a build handy to test.

Comment 19

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

Pushed http://hg.mozilla.org/mozilla-central/rev/e7a1b719e090