345739 - 'Block Images from %s' context menu item is misaligned

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Joey Minta]

The context menu option for 'Block images from %s' is misaligned on Mac.  (Right click on an image to show this.)  This is a regression, but I don't have a window yet.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

mconnor says that this is likely related to something Mento did :)

Comment 2

[Mark Mentovai]

Either me or Stefan.

Comment 3

[Mark Mentovai]

I made it bad, so that the "B" in Block was under the "e" in Set above it.  Stefan made it worse, now the B is under the space after the word Set.  Marking dependencies.

Comment 4

[Mark Mentovai]

Attachment: Theme fix

Comment 5

[Asaf Romano (gone)]

Comment on attachment 230473 [details] [diff] [review]
Theme fix

so this regresses the searchbar popup

Comment 6

[Mark Mentovai]

Attachment: Make it a checkbox menuitem

"So," actually, the theme was doing the right thing.  This shouldn't be represented as an iconic menu item, it should be represented as <menuitem type=checkbox>.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 230477 [details] [diff] [review]
Make it a checkbox menuitem

Thanks for cleaning this up, please let this bake on trunk first to enusre there was no real reason to make this so bogus...

r=mano.

Comment 8

[Mark Mentovai]

Checked in on the trunk.

Comment 9

[Mark Mentovai]

Comment on attachment 230477 [details] [diff] [review]
Make it a checkbox menuitem

Marking a? now or I'll forget about it, but I intend to give this a day or two of bake time.

Comment 10

[Stefan [:stefanh]]

(In reply to comment #6)
> Created an attachment (id=230477) [edit]
> Make it a checkbox menuitem
> 
> "So," actually, the theme was doing the right thing.  This shouldn't be
> represented as an iconic menu item, it should be represented as <menuitem
> type=checkbox>.
> 
(In reply to comment #6)
> Created an attachment (id=230477) [edit]
> Make it a checkbox menuitem
> 
> "So," actually, the theme was doing the right thing.  This shouldn't be
> represented as an iconic menu item, it should be represented as <menuitem
> type=checkbox>.
> 

Right, since checkbox menuitems are menuitem-iconic ones (see menu.css#303). Any menu-iconic menu item that is not in a menulist or not a checkbox/radio menuitem will have 21 px space at the left. The menulist/checkbox ones has only 3 px padding to the left. Since the checkmark image takes 16px in width and has a margin of 2px the text will end up 21 px to the left (and align with text in "normal" menuitems).

Comment 11

[Mark Mentovai]

FYI: Native Mac iconic menu items put the left edge of the icon at the same horizontal position as the left edge of the text of non-iconic menu items.  Take a look at Safari's bookmarks menu and note how it matches the new (correct) behavior of the search engine selection menu in Firefox, and the behavior of menu items in bookmark folders and RSS feeds in the bookmarks toolbar.

(Too bad our native menu items in the menu bar don't have icons...hmm...)

Comment 12

[Stefan [:stefanh]]

(In reply to comment #6)
> Created an attachment (id=230477) [edit]
> Make it a checkbox menuitem
> 
> "So," actually, the theme was doing the right thing.  This shouldn't be
> represented as an iconic menu item, it should be represented as <menuitem
> type=checkbox>.
> 

Righ(In reply to comment #11)
> FYI: Native Mac iconic menu items put the left edge of the icon at the same
> horizontal position as the left edge of the text of non-iconic menu items. 
> Take a look at Safari's bookmarks menu and note how it matches the new
> (correct) behavior of the search engine selection menu in Firefox, and the
> behavior of menu items in bookmark folders and RSS feeds in the bookmarks
> toolbar.
> 
> (Too bad our native menu items in the menu bar don't have icons...hmm...)
> 

There's a 5 years old "need-more-work" patch in bug 46177 :-;

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230477 [details] [diff] [review]
Make it a checkbox menuitem

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.

Comment 14

[Mark Mentovai]

Checkered in on MOZILLA_1_8_BRANCH before 1.8.1b2.