Last Comment Bug 643184 - Checkbox menu items in a toolbar do not display the check mark when checked in Firefox 4 on Mac OS X
: Checkbox menu items in a toolbar do not display the check mark when checked i...
Status: RESOLVED FIXED
: regression
Product: Toolkit
Classification: Components
Component: Themes (show other bugs)
: Trunk
: All Mac OS X
: -- normal with 4 votes (vote)
: mozilla33
Assigned To: Stefan [:stefanh]
:
: Dão Gottwald [:dao]
Mentors:
: 642112 650179 651008 (view as bug list)
Depends on: 1012445
Blocks: 542192
  Show dependency treegraph
 
Reported: 2011-03-19 15:26 PDT by Chris Pederick
Modified: 2014-07-10 05:37 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Testcase (1.58 KB, application/vnd.mozilla.xul+xml)
2011-11-16 17:01 PST, Mook (as)
no flags Details
patchV1 (788 bytes, patch)
2012-08-25 17:52 PDT, Leonard Camacho [:lcamacho]
no flags Details | Diff | Splinter Review
patchV2 (1.95 KB, patch)
2012-08-25 20:47 PDT, Leonard Camacho [:lcamacho]
enndeakin: review-
Details | Diff | Splinter Review
patchV3 (2.18 KB, patch)
2012-09-26 12:44 PDT, Leonard Camacho [:lcamacho]
enndeakin: review-
Details | Diff | Splinter Review
iconic and checkbox menu testcase (2.68 KB, application/vnd.mozilla.xul+xml)
2012-09-29 14:16 PDT, Neil Deakin
no flags Details

Description User image Chris Pederick 2011-03-19 15:26:57 PDT
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 User image Chris Pederick 2011-03-19 15:35:18 PDT
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 User image Henrik Skupin (:whimboo) 2011-03-20 16:49:27 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-20 18:44:55 PDT
Almost all my stuff was backed out in that interval. I don't know which of the rest could have caused this.
Comment 4 User image Henrik Skupin (:whimboo) 2011-03-21 08:29:21 PDT
Sadly I cannot build any version of Minefield from this given range due to a bustage in Spidermonkey.
Comment 5 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-21 13:10:38 PDT
Alice, are you able to do builds to get a precise changeset for a regression?
Comment 6 User image Alice0775 White 2011-03-21 15:41:32 PDT
(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 User image Henrik Skupin (:whimboo) 2011-03-30 08:35:37 PDT
Markus, are you able to compile the builds from my given regression range in comment 2? Sadly I still have no success.
Comment 8 User image Mark Smith (:mcs) 2011-04-07 09:55:16 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-07 19:54:06 PDT
Josh, this regression sounds kinda bad, maybe you can look into it?
Comment 10 User image Jonathan Kew (:jfkthame) 2011-04-07 22:13:45 PDT
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 User image Jonathan Kew (:jfkthame) 2011-04-08 00:29:43 PDT
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 User image Josh Aas 2011-04-08 19:48:23 PDT
Bug 559033 jumps out at me in that regression range:

Markus Stange: [Mac] New toolbar button style
Comment 13 User image Josh Aas 2011-04-08 19:52:12 PDT
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 14 User image Kevin Dangoor 2011-04-11 12:14:05 PDT
*** Bug 642112 has been marked as a duplicate of this bug. ***
Comment 15 User image :Ehsan Akhgari 2011-04-11 14:14:06 PDT
Dao probably knows this stuff well, too.
Comment 16 User image Markus Stange [:mstange] 2011-04-15 10:34:35 PDT
*** Bug 650179 has been marked as a duplicate of this bug. ***
Comment 17 User image Henrik Skupin (:whimboo) 2011-04-15 10:39:23 PDT
Looks like we should track this regression for Firefox 5 inclusion. It's a highly visible regression on OS X.
Comment 18 User image christian 2011-04-15 12:27:21 PDT
We wouldn't block going to beta or a release on this issue so we are not tracking for 5
Comment 19 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-17 16:52:04 PDT
Christian, we need a way to track Firefox 4 regressions that we should fix in Firefox 5. What do you suggest?
Comment 20 User image Henrik Skupin (:whimboo) 2011-04-20 15:21:43 PDT
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 21 User image Henrik Skupin (:whimboo) 2011-04-20 15:22:30 PDT
*** Bug 651008 has been marked as a duplicate of this bug. ***
Comment 22 User image Henrik Skupin (:whimboo) 2011-04-20 15:24:31 PDT
(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 User image Markus Stange [:mstange] 2011-04-29 08:05:29 PDT
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 User image fcana 2011-07-01 05:49:51 PDT
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 User image Mook (as) 2011-11-16 17:01:05 PST
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.
Comment 26 User image Leonard Camacho [:lcamacho] 2012-08-25 17:52:57 PDT
Created attachment 655382 [details] [diff] [review]
patchV1

I have a patch that works with testcase.
Comment 27 User image Leonard Camacho [:lcamacho] 2012-08-25 20:47:41 PDT
Created attachment 655397 [details] [diff] [review]
patchV2

Updated to work with testcase from bug 651008.
Comment 28 User image Neil Deakin 2012-09-25 10:39:43 PDT
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 User image Leonard Camacho [:lcamacho] 2012-09-26 11:56:18 PDT
Neil, can you attach an example or screenshot of that gap?
Comment 30 User image Neil Deakin 2012-09-26 12:01:31 PDT
You should be able to see it in the homepage menulist in the General tab of the Preferences window.
Comment 31 User image Leonard Camacho [:lcamacho] 2012-09-26 12:44:13 PDT
Created attachment 665093 [details] [diff] [review]
patchV3
Comment 32 User image Neil Deakin 2012-09-29 14:16:21 PDT
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 33 User image Neil Deakin 2012-10-25 05:46:14 PDT
Comment on attachment 665093 [details] [diff] [review]
patchV3

Clearing review due to issues noted above.
Comment 34 User image Eric Promislow 2013-01-02 16:16:03 PST
Komodo bug http://bugs.activestate.com/show_bug.cgi?id=96556 depends on this.
Comment 35 User image Ralf Strobel 2013-09-04 09:39:35 PDT
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 User image john 2013-10-07 19:16:31 PDT
(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 User image Kadir Topal [:atopal] 2013-12-16 05:04:43 PST
Ralf, could you provide a patch and ask for review? Thanks!
Comment 38 User image Ralf Strobel 2013-12-16 05:12:18 PST
Sorry, I don't work on FF itself, just add-ons. I don't even know where the original theme is defined.
Comment 39 User image Stefan [:stefanh] 2014-05-21 15:24:51 PDT
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 User image Stefan [:stefanh] 2014-07-09 21:18:26 PDT
This was fixed in bug 1012445.

Note You need to log in before you can comment on or make changes to this bug.