643184 - Checkbox menu items in a toolbar do not display the check mark when checked in Firefox 4 on Mac OS X

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Chris Pederick]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0

Checkbox menu items in a toolbar do not display the check mark when checked in Firefox 4 on Mac OS X. They do seem to work in a menu rather than a toolbar - using the DOM Inspector it appears to be because the iconic part of the menu has a width of 0, but I'm not sure where this is coming from.

Note that the exact same code works in Firefox 3 and earlier so this appears to be a regression (or change) in Firefox 4.

Reproducible: Always

Steps to Reproduce:
1. Install the Web Developer extension from https://addons.mozilla.org/en-US/firefox/addon/web-developer/ and restart
2. Go to Google.com and click 'Outline' -> 'Outline Block Level Elements' in the Web Developer toolbar
3. Re-open the 'Outline' menu in the Web Developer toolbar
Actual Results:  
The 'Outline Block Level Elements' menu is not checked

Expected Results:  
The 'Outline Block Level Elements' menu should be checked

The 'Outline Block Level Elements' menu under the main browser 'Tools' menu then 'Web Developer' -> 'Outline' is checked after the steps above, but I'm not sure why that is behaving differently. Again, note that this all works in Firefox 3.

Comment 1

[Chris Pederick]

Sorry, forgot to also mention that these same steps work just fine on Windows so this appears to be specific to Mac OS X.

Comment 2

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

Regression between 10090103 and 10090203.

PASS: http://hg.mozilla.org/mozilla-central/rev/f47972d05473
FAIL: http://hg.mozilla.org/mozilla-central/rev/dc2939f2640d

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f47972d05473&tochange=dc2939f2640d

Not sure which of those changesets caused this regression.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Almost all my stuff was backed out in that interval. I don't know which of the rest could have caused this.

Comment 4

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

Sadly I cannot build any version of Minefield from this given range due to a bustage in Spidermonkey.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alice, are you able to do builds to get a precise changeset for a regression?

Comment 6

[Alice0775 White]

(In reply to comment #5)
> Alice, are you able to do builds to get a precise changeset for a regression?

I do not have Mac.

Comment 7

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

Markus, are you able to compile the builds from my given regression range in comment 2? Sadly I still have no success.

Comment 8

[Mark Smith [:mcs]]

We just encountered this bug in our Page Saver add-on so it may be affecting a lot of different toolbar-based menus on Mac OS.  Also, bug 642112 looks like it might be a duplicate.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Josh, this regression sounds kinda bad, maybe you can look into it?

Comment 10

[Jonathan Kew [:jfkthame]]

I'm not sure the regression range in comment #2 is accurate. I did a build of f47972d05473 and I'm seeing the problem there following the STR given.

Actually, it's not 100% reliable; at one point, after playing around with a number of the Web Developer toolbar menus, suddenly the checkmarks in the Outline menu began to appear as expected. After restarting the browser, they're missing again, and I have not found a way to restore them. But there's something intermittent/flaky about this....

Comment 11

[Jonathan Kew [:jfkthame]]

Testing with nightlies indicates a regression in the 2010-06-23-03-mozilla-central build; 06-22 works fine.

This points to a regression range of:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ce18b4287791&tochange=8f05ab3aa198

Note that in builds after the regression, the checkmarks still _sometimes_ show up correctly, so repeated testing is needed in order to be reasonably confident whether a given build has the problem or not.

Comment 12

[Josh Aas]

Bug 559033 jumps out at me in that regression range:

Markus Stange: [Mac] New toolbar button style

Comment 13

[Josh Aas]

Reassigning to Markus for now since it looks like his regression. If Markus can't look at it we can find a new owner, possibly me.

Comment 15

[(no longer active)]

Dao probably knows this stuff well, too.

Comment 17

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

Looks like we should track this regression for Firefox 5 inclusion. It's a highly visible regression on OS X.

Comment 18

[LegNeato]

We wouldn't block going to beta or a release on this issue so we are not tracking for 5

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Christian, we need a way to track Firefox 4 regressions that we should fix in Firefox 5. What do you suggest?

Comment 20

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

Here the comment from Jorge on bug 651008, which will be a dupe of this bug but explains the problem in detail:

Forgot to mention the cause: the menuitem element has a child box that has its
background-image property set to the check icon. For some reason the width of
this box is set to 0 by default, except for the main menuitems as mentioned in
comment #2. Setting the width to some other value with CSS fixes the problem.

Comment 22

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

(In reply to comment #19)
> Christian, we need a way to track Firefox 4 regressions that we should fix in
> Firefox 5. What do you suggest?

Christian, any comment from you on that topic?

Beside that question we could simply come up with a fix and ask for approval-aurora once it has been landed on m-c.

Comment 23

[Markus Stange [:mstange]]

I've done another regression search using the testcase in bug 651008 and have come to this regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3e7755e00fa1&tochange=32a13ebe9ba0 which points to bug 542192

Comment 24

[u381999]

I tried to install a different theme (instead of the default one) ad the problem is "solved". Menus work well using "MacOSX Theme (Firefox 4+) 1.7.0".
hoping this helps...

Comment 25

[Mook (work)]

Attachment: Testcase

Since this hasn't been mentioned yet: here's a testcase.  This appears to only happen if the menu is initially displayed with the menuitem not having a checked= attribute.  If it's checked (either originally set in the xul, or set by code) when the menu is displayed, it's fine.

Comment 26

[Leonard Camacho [:lcamacho]]

Attachment: patchV1

I have a patch that works with testcase.

Comment 27

[Leonard Camacho [:lcamacho]]

Attachment: patchV2

Updated to work with testcase from bug 651008.

Comment 28

[Neil Deakin]

Comment on attachment 655397 [details] [diff] [review]
patchV2

This causes a large gap to appear on the left side of items in menulists between the checkbox and label. You likely need to have the width or something else apply only to items not in menulists.

Comment 29

[Leonard Camacho [:lcamacho]]

Neil, can you attach an example or screenshot of that gap?

Comment 30

[Neil Deakin]

You should be able to see it in the homepage menulist in the General tab of the Preferences window.

Comment 31

[Leonard Camacho [:lcamacho]]

Attachment: patchV3

Comment 32

[Neil Deakin]

Attachment: iconic and checkbox menu testcase

The following testcase shows various combinations of iconic and checkbox menuitems a toolbar.

I'm not sure if this patch handles the case where both a set properly. Note the margins on the left for example.

The needed patch should ideally preserve the existing behaviour. (Or it can implement the actual Mac native look which isn't exactly what we do currently)

Comment 33

[Neil Deakin]

Comment on attachment 665093 [details] [diff] [review]
patchV3

Clearing review due to issues noted above.

Comment 34

[Eric Promislow]

Komodo bug http://bugs.activestate.com/show_bug.cgi?id=96556 depends on this.

Comment 35

[Ralf Strobel]

The problem still exists in FF 23 on Mac and prevents the correct rendering of toolbarbutton > menupopup > menuitem[type="checkbox"]. If the checkmark was not set during the first popup event, it will never be displayed.

I investigated the problem a little and it seems to have to do with the way the outset indentation for the checkmark is defined. The space reserved by .menu-iconic-left seems to be too small (15px) for the typical icon (16px). The icon itself also misses a width declaration.

The following alternative definition fixes the problem for me:
.menu-iconic-left { margin-left: -16px; margin-right: -16px; padding-left: 16px; }
.menu-iconic-icon { width:16px; margin-left: 0px; margin-right: 0px; }

Comment 36

[john]

(In reply to Ralf Strobel from comment #35)
> The problem still exists in FF 23 on Mac and prevents the correct rendering
> of toolbarbutton > menupopup > menuitem[type="checkbox"]. If the checkmark
> was not set during the first popup event, it will never be displayed.
> 
> I investigated the problem a little and it seems to have to do with the way
> the outset indentation for the checkmark is defined. The space reserved by
> .menu-iconic-left seems to be too small (15px) for the typical icon (16px).
> The icon itself also misses a width declaration.
> 
> The following alternative definition fixes the problem for me:
> .menu-iconic-left { margin-left: -16px; margin-right: -16px; padding-left:
> 16px; }
> .menu-iconic-icon { width:16px; margin-left: 0px; margin-right: 0px; }

Thanks! I can confirm Ralf's fix works for me (FF 24 on OSX 10.8.5).

Comment 37

[Kadir Topal [:atopal]]

Ralf, could you provide a patch and ask for review? Thanks!

Comment 38

[Ralf Strobel]

Sorry, I don't work on FF itself, just add-ons. I don't even know where the original theme is defined.

Comment 39

[Stefan [:stefanh]]

Bug 1012445 will fix this since we'll then draw a native checkmark over the menuitem instead of setting an icon with css.

Comment 40

[Stefan [:stefanh]]

This was fixed in bug 1012445.