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

RESOLVED FIXED in mozilla33

Status

()

Toolkit
Themes
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Chris Pederick, Assigned: stefanh)

Tracking

({regression})

Trunk
mozilla33
All
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5-)

Details

Attachments

(3 attachments, 2 obsolete attachments)

1.58 KB, application/vnd.mozilla.xul+xml
Details
2.18 KB, patch
Neil Deakin (not available until Aug 9)
: review-
Details | Diff | Splinter Review
2.68 KB, application/vnd.mozilla.xul+xml
Details
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Sorry, forgot to also mention that these same steps work just fine on Windows so this appears to be specific to Mac OS X.
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.
Keywords: regression
Hardware: x86 → All
Whiteboard: [4rc2]
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Almost all my stuff was backed out in that interval. I don't know which of the rest could have caused this.
Sadly I cannot build any version of Minefield from this given range due to a bustage in Spidermonkey.
Alice, are you able to do builds to get a precise changeset for a regression?

Comment 6

6 years ago
(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.
Markus, are you able to compile the builds from my given regression range in comment 2? Sadly I still have no success.

Comment 8

6 years ago
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.
Josh, this regression sounds kinda bad, maybe you can look into it?
Assignee: nobody → joshmoz
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....
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

6 years ago
Bug 559033 jumps out at me in that regression range:

Markus Stange: [Mac] New toolbar button style

Comment 13

6 years ago
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.
Assignee: joshmoz → mstange

Updated

6 years ago
Duplicate of this bug: 642112
Dao probably knows this stuff well, too.
Duplicate of this bug: 650179
Looks like we should track this regression for Firefox 5 inclusion. It's a highly visible regression on OS X.
tracking-firefox5: --- → ?

Comment 18

6 years ago
We wouldn't block going to beta or a release on this issue so we are not tracking for 5
tracking-firefox5: ? → -
Christian, we need a way to track Firefox 4 regressions that we should fix in Firefox 5. What do you suggest?
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.
Component: Menus → Theme
QA Contact: menus → theme
Duplicate of this bug: 651008
(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.
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
Blocks: 542192

Updated

6 years ago
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes

Comment 24

6 years ago
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

6 years ago
Created attachment 575040 [details]
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.
Created attachment 655382 [details] [diff] [review]
patchV1

I have a patch that works with testcase.
Attachment #655382 - Flags: review?(dao)
Created attachment 655397 [details] [diff] [review]
patchV2

Updated to work with testcase from bug 651008.
Attachment #655382 - Attachment is obsolete: true
Attachment #655382 - Flags: review?(dao)
Attachment #655397 - Flags: review?(dao)
Attachment #655397 - Flags: review?(enndeakin)
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.
Attachment #655397 - Flags: review?(enndeakin) → review-
Attachment #655397 - Flags: review?(dao)
Assignee: mstange → nobody
Whiteboard: [4rc2]
Neil, can you attach an example or screenshot of that gap?
You should be able to see it in the homepage menulist in the General tab of the Preferences window.
Created attachment 665093 [details] [diff] [review]
patchV3
Attachment #655397 - Attachment is obsolete: true
Attachment #665093 - Flags: review?(enndeakin)
Created attachment 666249 [details]
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 on attachment 665093 [details] [diff] [review]
patchV3

Clearing review due to issues noted above.
Attachment #665093 - Flags: review?(enndeakin) → review-

Comment 34

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

Comment 35

4 years ago
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

4 years ago
(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).
Ralf, could you provide a patch and ask for review? Thanks!

Comment 38

4 years ago
Sorry, I don't work on FF itself, just add-ons. I don't even know where the original theme is defined.
(Assignee)

Comment 39

3 years ago
Bug 1012445 will fix this since we'll then draw a native checkmark over the menuitem instead of setting an icon with css.
Depends on: 1012445
(Assignee)

Comment 40

3 years ago
This was fixed in bug 1012445.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Assignee: nobody → stefanh
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.