219832 - Multiple checks in Window Menu (OS X)

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Joel Nelson (don't send email)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030827
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030827

There are multiple checkmarks at the bottom of the Window menu (showing active
windows) in Mozilla 1.5b (20030827) and Mozilla 1.5 RC1 at times if I have
multiple windows open.   However, this is not always reproducible, which makes
it hard to troubleshoot.

Reproducible: Sometimes

Steps to Reproduce:
1. Open multiple windows
2. Check Window menu to toggle between windows
3. Observe multiple checkmarks to the left of some of the numbers (1,2,3, etc.)

Actual Results:  
There were multiple checkmarks.

Expected Results:  
Only the active window should have been checked.

Comment 1

[FZiegler]

I'm seeing this all the time with 1.5b on OS X, too. Not only in Mail but also
Browser, Composer, anything that makes windows.

--> trying to change to Browser: XP Toolkit/Widgets: Menus.

Comment 2

[Stefan [:stefanh]]

Looks related to bug 161383 and bug 144622.

Comment 3

[Don Crandall]

Attachment: Fixes the multiple checkmarks for the window menu

The patch simply adds a name attribute to the dynamic menu, this will group the
accumilated items together to give the menu items their expected type="radio"
behavior.

The patch offered is for Mozilla and Firebird and Thunderbird.

Comment 4

[Mike Calmus]

Attachment: Alternate patch to use with patch from 161383 to not break 90824

While the patch presented
(http://bugzilla.mozilla.org/attachment.cgi?id=140355&action=view) fixes the
issue, making the menu behave in the same manner as a radio button group would
make fixing bug #90824 much more difficult (impossible?) without stripping the
code that is the essence of that patch. This patch, when used along with the
one presented in bug <a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=161383">161383</a> will fix
both issues and still make it possible to later add in code to address issue
90824. Instead of only setting the "checked" attribute for the front-most
window, it also unsets the attribute for all others.

Comment 5

[[e-mail]]

Attachment: Patch version 1

This has the same effect as attachment 145396 [details] [diff] [review].

Comment 6

[[e-mail]]

Attachment: Patch version 2

This would combine with attachment 140355 [details] [diff] [review], by ignoring type="radio" if there is
no named group, which is how the mac code currently works. Unfortunately I
diverged from the mac code here, by fixing the group name change logic...

Comment 7

[[e-mail]]

Comment on attachment 145396 [details] [diff] [review]
Alternate patch to use with patch from 161383 to not break 90824

But this way is definitely wrong.

Comment 8

[Greg K.]

How do I reproduce this problem? The steps in comment 0 don’t cause the
described behavior using 1.7RC1.

Comment 9

[Greg K.]

Ah, never mind, I see it after switching around between windows using the Window
menu. Flagging blocking1.7? since this is a companion patch to bug 161383.

Comment 10

[[e-mail]]

So the question is, which way should <menuitem type="radio" checked="true">
behave: the non-mac way, which is to uncheck all the other radio menuitems with
no group, or the mac way, which is to ignore all other menuitems?

Comment 11

[Boris Zbarsky [:bzbarsky]]

How do <radio> elements with no group behave?  I would say this should behave
the same way, no?

Comment 12

[Alec Flett]

I like the mac way. all radio menuitems with no group should not all be
considered within the same group.. that's asinine! :)

Comment 13

[[e-mail]]

<radio> elements group by containment, so no joy there, but I looked at <input
type="radio"> elements, and our current code does not group unnamed HTML radio
buttons (which are probably invalid HTML anyway; IE5.0 "disables" them)

Comment 14

[[e-mail]]

Comment on attachment 140355 [details] [diff] [review]
Fixes the multiple checkmarks for the window menu

OK, so we need this on the trunk and branch to resolve the issue...

Comment 15

[[e-mail]]

Comment on attachment 145420 [details] [diff] [review]
Patch version 2

...and this on the trunk to stop people making the same mistake again.

Comment 16

[Alec Flett]

Comment on attachment 140355 [details] [diff] [review]
Fixes the multiple checkmarks for the window menu

yikes! :) Hope there aren't any other toplevel windows with this issue.

sr=alecf

Comment 17

[[e-mail]]

Comment on attachment 140355 [details] [diff] [review]
Fixes the multiple checkmarks for the window menu

Low-risk portion of the fix suitable for the branch.

Comment 18

[[e-mail]]

alecf: I didn't spot anyone else trying to set menubar checks in popupshown.
There proxy context menu does but that's pure xul so it should be unaffected.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this review for at least a week, and probably
longer...

Comment 20

[[e-mail]]

Comment on attachment 145420 [details] [diff] [review]
Patch version 2

roc likes these easy reviews ;-)

Comment 21

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

Comment on attachment 145420 [details] [diff] [review]
Patch version 2

That wasn't so easy :-)

-    mContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::name, value);
-    if (value == mGroupName)

This code never did anything, because mGroupName was already always the new
group name --- right?

Comment 22

[[e-mail]]

Attachment: Indeed it was

Comment 23

[[e-mail]]

Comment on attachment 146945 [details] [diff] [review]
Indeed it was

Clarified as per comment.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 140355 [details] [diff] [review]
Fixes the multiple checkmarks for the window menu

a=asa (on behalf of drivers) for checkin to 1.7

Comment 25

[Asa Dotzler [:asa]]

I don't see this in the list of branch checkins. Can someone please land this
patch on the 1.7 branch, or if it's already there, add the fixed1.7 keyword? Thanks.

Comment 26

[[e-mail]]

Fixes checked in.

Comment 27

[Tracy Walker [:tracy]]

verified