Multiple checks in Window Menu (OS X)

VERIFIED FIXED

Status

()

Core
XUL
VERIFIED FIXED
14 years ago
9 years ago

People

(Reporter: Joel Nelson (don't send email), Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug, {verified1.7})

Trunk
PowerPC
Mac OS X
verified1.7
Points:
---
Bug Flags:
blocking1.7 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

14 years ago
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.
Component: Mail Window Front End → XP Toolkit/Widgets: Menus
Product: MailNews → Browser

Comment 2

14 years ago
Looks related to bug 161383 and bug 144622.

Comment 3

14 years ago
Created attachment 140355 [details] [diff] [review]
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

14 years ago
Created attachment 145396 [details] [diff] [review]
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.

Updated

14 years ago
Attachment #145396 - Flags: review?(sspitzer)

Updated

14 years ago
Blocks: 90824

Updated

14 years ago
Attachment #145396 - Flags: review?(sspitzer) → review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 5

14 years ago
Created attachment 145417 [details] [diff] [review]
Patch version 1

This has the same effect as attachment 145396 [details] [diff] [review].
Assignee: sspitzer → neil.parkwaycc.co.uk
Attachment #145396 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 6

14 years ago
Created attachment 145420 [details] [diff] [review]
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...
(Assignee)

Comment 7

14 years ago
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.
Attachment #145396 - Flags: review?(neil.parkwaycc.co.uk) → review-

Comment 8

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

Comment 9

14 years ago
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.
Flags: blocking1.7?
(Assignee)

Comment 10

14 years ago
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?
How do <radio> elements with no group behave?  I would say this should behave
the same way, no?

Comment 12

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

Comment 13

14 years ago
<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)

Updated

14 years ago
Flags: blocking1.7? → blocking1.7+
(Assignee)

Comment 14

14 years ago
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...
Attachment #140355 - Flags: superreview?(alecf)
Attachment #140355 - Flags: review+
(Assignee)

Comment 15

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

...and this on the trunk to stop people making the same mistake again.
Attachment #145420 - Flags: superreview?(bzbarsky)
Attachment #145420 - Flags: review?(bzbarsky)

Comment 16

14 years ago
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
Attachment #140355 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 17

14 years ago
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.
Attachment #140355 - Flags: approval1.7?
(Assignee)

Comment 18

14 years ago
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.
I'm not going to be able to get to this review for at least a week, and probably
longer...
(Assignee)

Comment 20

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

roc likes these easy reviews ;-)
Attachment #145420 - Flags: superreview?(roc)
Attachment #145420 - Flags: superreview?(bzbarsky)
Attachment #145420 - Flags: review?(roc)
Attachment #145420 - Flags: review?(bzbarsky)
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?
Attachment #145420 - Flags: superreview?(roc)
Attachment #145420 - Flags: superreview+
Attachment #145420 - Flags: review?(roc)
Attachment #145420 - Flags: review+
(Assignee)

Comment 22

14 years ago
Created attachment 146945 [details] [diff] [review]
Indeed it was
(Assignee)

Comment 23

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

Clarified as per comment.
Attachment #146945 - Flags: superreview?(roc)
Attachment #146945 - Flags: review?(roc)
Attachment #146945 - Flags: superreview?(roc)
Attachment #146945 - Flags: superreview+
Attachment #146945 - Flags: review?(roc)
Attachment #146945 - Flags: review+

Comment 24

14 years ago
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
Attachment #140355 - Flags: approval1.7? → approval1.7+

Comment 25

14 years ago
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.
(Assignee)

Comment 26

14 years ago
Fixes checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.7
Resolution: --- → FIXED

Updated

14 years ago
QA Contact: esther → benc
verified
Status: RESOLVED → VERIFIED
Keywords: fixed1.7 → verified1.7

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: benc → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.