Open Bug 1017412 Opened 10 years ago Updated 2 years ago

Duplicated ID assignment to different menuitems & Duplicated command definitions in multiple commandset is seen

Categories

(Thunderbird :: Mail Window Front End, defect)

24 Branch
x86
Windows 7
defect

Tracking

(Not tracked)

People

(Reporter: World, Unassigned)

Details

(Whiteboard: [leave open])

Attachments

(1 file)

Duplicated ID assignment to different menuitems & Duplicated command definitions.in multiple keyset is seen in Tb 24.5.0 on Win.
Intentional?

Following is duplicated ID list obtained by;
1.  document.getElementsByTagName() for "key" , "command", "menuitem", "toolbarbutton".
2. Sort element by ID and element.getAttribute("id").
I don't know which is returned by document.getElementById(duplicated_ID) .

> Duplicated command definitions.in multiple commandset.
> DupID : cmd_delete : command(parent=commandset/id=mailEditMenuItems), command(parent=commandset/id=mailToolbarItems)
> DupID : cmd_chat : command(parent=commandset/id=mailGoMenuItems), command(parent=commandset/id=mailToolbarItems)

> Duplicated ID assignment to different menuitems.
> DupID : addNewTag : menuitem(parent=menupopup/id=mailContext-tagpopup), menuitem(parent=menupopup/id=tagMenu-tagpopup), menuitem(parent=menupopup/id=button-tagpopup)
> DupID : manageTags : menuitem(parent=menupopup/id=mailContext-tagpopup), menuitem(parent=menupopup/id=tagMenu-tagpopup), menuitem(parent=menupopup/id=button-tagpopup)
> DupID : newNewMsgCmd : menuitem(parent=menupopup/id=appmenu_newMenupopup), menuitem(parent=menupopup/id=menu_NewPopup)
> DupID : applyFilters : menuitem(parent=menupopup/id=appmenu_FilterMenu), menuitem(parent=menupopup/id=taskPopup)
> DupID : applyFiltersToSelection : menuitem(parent=menupopup/id=appmenu_FilterMenu), menuitem(parent=menupopup/id=taskPopup)
messenger window only is checked.
Summary: Duplicated ID assignment to different menuitems & Duplicated command definitions.in multiple commandset is seen → Duplicated ID assignment to different menuitems & Duplicated command definitions in multiple commandset is seen
Interesting, good job. E.g. with the "addNewTag" item, the parent items have different IDs and the sibling cmd_removeTags menuitems have differing IDs. But "addNewTag" and "manageTags" are duplicated. Actually triplicated.

mkmelin, the IDs should be unique per document. Is the main window + message context menu a single document?
Assignee: nobody → acelists
Component: General → Mail Window Front End
Flags: needinfo?(mkmelin+mozilla)
I think it's seen as one single document, yes. But of course if it's an overlay the overlay is used, so I'm not sure there's a problem here.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #3)
> so I'm not sure there's a problem here.

Yes of course, no problem will occur unless document.getElementById(duplicated_ID) is issued for the duplicated_ID.
Problems are:
- Addon developers can't use document.getElementById(duplicated_ID) freely and safely.
- This kind of problem is usually evidence of "Tb developers don't know W3C rule of 'ID of XUL/HTML element has to be
  unique'" for many software engineers who don't know rules in Mozilla world well.
I didn't look closely enough, but yes you're correct, e.g. this id should not be duplicated
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1018
(In reply to WADA from comment #4)
> - This kind of problem is usually evidence of "Tb developers don't know W3C
> rule of 'ID of XUL/HTML element has to be
>   unique'" for many software engineers who don't know rules in Mozilla world
> well.

They surely know at least this rule, but it is often just bad copy&paste and sometimes it is hard to see which elements get into the same document as they are spread among several files.
So I updated the element IDs, but I can't help with the command IDs. I tried to remove cmd_delete from 'commandset id="mailToolbarItems"' as it looked like button_delete there is enough. But it caused failures in mozmill/folder-display/test-deletion-with-multiple-displays.js test.

I changed the css to match multiple IDs now with the [command=] clause as other rules do. I mark Josiah for review of that. I wonder why other OSes didn't reference the ID I have renamed.
Attachment #8431800 - Flags: ui-review?(josiah)
Attachment #8431800 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 8431800 [details] [diff] [review]
fix duplicate element IDs

Review of attachment 8431800 [details] [diff] [review]:
-----------------------------------------------------------------

'applyFilters' at here is ID of element.
http://mxr.mozilla.org/comm-central/source/mail/base/content/hiddenWindow.js#31
It should be changed to 'appmenu_applyFilters' accoring to id change.
Attachment #8431800 - Flags: review-
Foolowing is ID change by you.
> - <menuitem id="addNewTag"                  + <menuitem id="mailContext-addNewTag"
> - <menuitem id="addNewTag"                   + <menuitem id="tagMenu-addNewTag"
> - <menuitem id="addNewTag"                   + <menuitem id="button-addNewTag"
> - <menuitem id="applyFilters"                   + <menuitem id="appmenu_applyFilters"
> - <menuitem id="applyFiltersToSelection"  + <menuitem id="appmenu_applyFiltersToSelection"
> - <menuitem id="manageTags"                 + <menuitem id="mailContext-manageTags"
> - <menuitem id="manageTags"                 + <menuitem id="tagMenu-manageTags"
> - <menuitem id="manageTags"                 + <menuitem id="button-manageTags"
> - <menuitem id="newNewMsgCmd"          + <menuitem id="appmenu_newNewMsgCmd"
> - <menuitem id="newNewMsgCmd"          + <menuitem id="menu_newNewMsgCmd"

Only 'applyFilters' is seen in hiddenWindow.js.
'addNewTag', 'manageTags', 'applyFiltersToSelection', 'newNewMsgCmd' are not seen in hiddenWindow.js.
No need to add new IDs to hiddenWindow.js?
(In reply to :aceman from comment #7)
> I tried to remove cmd_delete from 'commandset id="mailToolbarItems"'
> as it looked like button_delete there is enough.
> But it caused failures in mozmill/folder-display/test-deletion-with-multiple-displays.js test.

<command id=cmd_delete> is defined under <commandset id=mailEditMenuItems> and <commandset id=mailToolbarItems> in this order in mailWindowOverlay.xul.
Overlay is usually done with specifying higher level element. But in utilityOverlay.xul, <command id=cmd_delete> is overlayed without specifying higher level element. <command id=cmd_delete> is directly specified.
And, as seen in following comment, some quirks is made on utilityOverlay.xul. it's loaded twice.
 http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#14
As I wrote in bug 359748, loading sequence of utilityOverlay.xul is funny and is executed twice by quirks.

These may be relevant to phenomenon you saw after deletion of <command id=cmd_delete> under <commandset id=mailToolbarItems>, because result of XUL overlay may be unpredictable if duplicated ID is defined.

Does test-deletion-with-multiple-displays.js emulate quirks in messenger, "overlay utilityOverlay.xul twice" etc., correctly?
"duplicated cmd_delete" may be prerequisite of such quirks  :-)
(In reply to :aceman from comment #7)
> I tried to remove cmd_delete from 'commandset id="mailToolbarItems"'
> as it looked like button_delete there is enough.
> But it caused failures in mozmill/folder-display/test-deletion-with-multiple-displays.js test.

It may be disable/enable relevant problem.
- When duplicated. call "two dups" "cmd#1 and cmd#2"
  Task #1-1: disables cmd#1, Task #1-2: enables cmd#1, Task #1-3: use cmd#1, Task #1-4: disables cmd#1
  Task #2-1: disables cmd#2, Task #2-2: enables cmd#2, Task #2-3: use cmd#2, Task #2-4: disables cmd#2
- When duplication is removed
   Task #1-2: enables cmd,                                Task #1-3: tries to use cmd, but fails because disabled
                                  Task #2-1: disables cmd,                                                                            Task #2-2: enables cmd,
Does document.getElementById consistently return specific element anytime anywhere when duplicated IDs are defined?
Difference between two dups was visible by element.outerHTML in addition to element.parentNode.id.
> element.outerHTML :
> Under <commandset id=mailEditMenuItems>
> <command xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="cmd_delete"
>    valueFolder="Delete Folder" valueFolderAccessKey="D" valueNewsgroup="Unsubscribe"
>    valueNewsgroupAccessKey="b" valueMessage="Delete Message" valueMessageAccessKey="D"
>    valueIMAPDeletedMessage="Undelete Message" valueIMAPDeletedMessageAccessKey="d"
>    valueMessages="Delete Selected Messages" valueMessagesAccessKey="D"
>    valueIMAPDeletedMessages="Undelete Selected Messages"
>    valueIMAPDeletedMessagesAccessKey="d" oncommand="goDoCommand('cmd_delete')"
>    valueDefault="Delete" valueDefaultAccessKey="D"
>    disabled="true" label="Delete Message" accesskey="D"/>
> Under <commandset id=mailToolbarItems>
> <command xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="cmd_delete"/>
All overlay is done on first dup under <commandset id=mailEditMenuItems> as expected.

I can't imagine reason why "delete dup under <commandset id=mailToolbarItems>" produces problem.

Is "dup under <commandset id=mailToolbarItems>" returned to document.getElementById("cmd_delete") in some scopes?
If so, "no disabled=true in second dup" may explain, because disabled=true/false is always controled as designed/implemented after deletion of dup.

Is your test case relevant to disabled=true/false status of <command id="cmd_delete">?
(In reply to :aceman from comment #7)
> But it caused failures in mozmill/folder-display/test-deletion-with-multiple-displays.js test.

"Invoking delete" is done by "press_delete();".
Can you add status/attribute value check steps just before it?
   Delete key, Edit menu/Delete, <command id="cmd_delete">.
What am I ui-reviewing here? If you wanted me to review the CSS code, flag me for review. Thanks.
(In reply to WADA from comment #8)
> Comment on attachment 8431800 [details] [diff] [review]
> patch
> 
> Review of attachment 8431800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 'applyFilters' at here is ID of element.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/hiddenWindow.
> js#31
> It should be changed to 'appmenu_applyFilters' accoring to id change.

One occurrence of 'applyFilters' stayed so it can't be removed from this file. I don't know which id must go into this hiddenWindow list. See that appmenu_filtersCmd is not there (while filtersCmd is), that is in the same menupopup as the new appmenu_applyFilters. So I do not think we need to change anything in this file.
Comment on attachment 8431800 [details] [diff] [review]
fix duplicate element IDs

Yes, I said in comment 7 that I'd like you to see the css changes.
Attachment #8431800 - Flags: ui-review?(josiah) → review?(josiah)
Comment on attachment 8431800 [details] [diff] [review]
fix duplicate element IDs

Review of attachment 8431800 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. 
Re the css change, that style is slightly less efficient, but probably not enough to care.
Attachment #8431800 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8431800 [details] [diff] [review]
fix duplicate element IDs

Review of attachment 8431800 [details] [diff] [review]:
-----------------------------------------------------------------

Well, the CSS seems fine to me.
Attachment #8431800 - Flags: review?(josiah) → review+
Attachment #8431800 - Attachment description: patch → fix duplicate element IDs
Keywords: checkin-needed
Whiteboard: [leave open]

Is this "fix" ever going to be uplifted, or is the bug dead?

[leave open]

What's left to do?

Flags: needinfo?(acelists)

The cmd_delete and cmd_chat IDs are unfixed by this patch. But you may want to split them into a new bug for easier tracking (and close this one).

Status: ASSIGNED → NEW
Flags: needinfo?(acelists)
Assignee: acelists → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.