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)
Tracking
(Not tracked)
NEW
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)
Reporter | ||
Comment 1•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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)
Reporter | ||
Comment 4•10 years ago
|
||
(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.
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 8•10 years ago
|
||
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-
Reporter | ||
Comment 9•10 years ago
|
||
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?
Reporter | ||
Comment 10•10 years ago
|
||
(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 :-)
Reporter | ||
Comment 11•10 years ago
|
||
(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?
Reporter | ||
Comment 12•10 years ago
|
||
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">?
Reporter | ||
Comment 13•10 years ago
|
||
(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">.
Comment 14•10 years ago
|
||
What am I ui-reviewing here? If you wanted me to review the CSS code, flag me for review. Thanks.
Comment 15•10 years ago
|
||
(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 16•10 years ago
|
||
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 17•10 years ago
|
||
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 18•10 years ago
|
||
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]
Comment 19•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/9e8809412458
Keywords: checkin-needed
Comment 20•4 years ago
|
||
Is this "fix" ever going to be uplifted, or is the bug dead?
Comment 22•3 years ago
|
||
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)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•