Closed
Bug 118038
Opened 23 years ago
Closed 22 years ago
editable menulist does not display selected item
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
WORKSFORME
mozilla1.3beta
People
(Reporter: david, Assigned: neil)
References
Details
(Keywords: access, Whiteboard: [xul1.0-widgets-menulist])
Attachments
(2 files, 10 obsolete files)
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20011019 Netscape6/6.2 BuildID: 2001122106 the menulist should display the selected value when a page is opened... instead it displays nothing... an exploit is attached. Reproducible: Always Steps to Reproduce: <?xml version="1.0"?> <?xml-stylesheet href="chrome://global/skin/" type="text/css"?> <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" resizable="true" style="background-color: #64849f"> <groupbox orient="vertical"> <caption label="Exploit"/> <grid flex="1"> <columns> <column align="right" style="min-width:50px"/> <column flex="1"/> </columns> <rows style="color:white; font-weight:bold; background-color: transparent"> <row> <label value="Company:"/> <menulist> <menupopup> <menuitem label="Intermedia Communications"/> <menuseparator/> <menuitem label="Lucent" selected="true"/> </menupopup> </menulist> </row> </rows> </grid> </groupbox> </window> Actual Results: menu is blank on page open Expected Results: menu should display "Lucent" thread on netscape.public.mozilla.xpfe: http://groups.google.com/groups?threadm=3C34BB2E.8080909%40netscape.com&prev=/groups%3Fq%3Dnetscape.public.mozilla.xpfe%26hl%3Den%26btnG%3DGoogle%2BSearch
Updated•23 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•23 years ago
|
||
This patch contains the following fixes: * Fix initial selection via selected="true" attribute * Created menuBoxObject field (c.f. outliner) * Use selectedInternal instead of selectedItem inside loops * Allow all menulists to open via .open = true * All functions now use .firstChild to locate the menupopup (was inconsistent) * Don't set src on editable menulists because they don't have one * Simplified focus and blur events on editable menulists * Used DOM key constants instead of literals * Used .hasChildNodes() instead of .childNodes.length > 0
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Comment 3•23 years ago
|
||
Didn't aaronl just fix the .open property and add it to menulists? Had he not checked in yet?
Updated•23 years ago
|
Blocks: 70753
Status: NEW → ASSIGNED
Whiteboard: [xul1.0-widgets-menulist]
Target Milestone: --- → mozilla1.0
Assignee | ||
Comment 4•23 years ago
|
||
Comment on attachment 64727 [details] [diff] [review] Proposed patch Typo: this.selectedItem = curr.childNodes[val]; should of course be this.selectedItem = this.firstChild.childNodes[val]; Hmmm... would val in this.firstChild.childNodes be more efficient than val < this.firstChild.childNodes.length?
Assignee | ||
Comment 5•23 years ago
|
||
Added missing select() method to editable menulists.
Attachment #64727 -
Attachment is obsolete: true
Assignee | ||
Comment 6•23 years ago
|
||
Attachment #66868 -
Attachment is obsolete: true
Assignee | ||
Comment 8•23 years ago
|
||
Attachment #67069 -
Attachment is obsolete: true
Comment 9•23 years ago
|
||
Neil, F4 should already drop down menulist! Are you sure that you tried that on the trunk?
Assignee | ||
Comment 10•23 years ago
|
||
OK, but something's still wrong because in build 2002021803 I can't open an editable menulist with the keyboard.
Attachment #70258 -
Attachment is obsolete: true
Comment 12•22 years ago
|
||
Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2
Target Milestone: mozilla1.0 → mozilla1.2
Assignee | ||
Comment 13•22 years ago
|
||
I've just noticed that editable menulists don't have context menus.
Attachment #70273 -
Attachment is obsolete: true
Comment 14•22 years ago
|
||
This is still a problem. What I've noticed is this. From JS: document.getElementById( "menulist" ).value = "somevalue" will get the item selected properly (ie. do that on load, everything will be fine). However, if you want to persist the selection, then that doesn't work because you don't know what "somevalue" is. You can see this bug in all its glory in the calendar. The event filter does not show up properly at the top of the right hand side.
Blocks: 164535
Assignee | ||
Comment 15•22 years ago
|
||
This now allows add/insert to work when there is no predefined menupopup.
Assignee | ||
Updated•22 years ago
|
Attachment #90749 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #106665 -
Flags: review?(cmanske)
Comment 16•22 years ago
|
||
Comment on attachment 106665 [details] [diff] [review] Added support for empty menulists by request of cmanske Wait a minute! Do we have to append a menupopup AND menuitem? I was requesting only to auto-append a menupopup. If you do both, won't that make empty menus look funny? (They'll have an empty menuitem that will be visible.)
Assignee | ||
Comment 17•22 years ago
|
||
Comment on attachment 106665 [details] [diff] [review] Added support for empty menulists by request of cmanske cmanske: we want menulist.appendItem to create the menuitem but also a menupopup if one does not yet exist, rather than failing.
Updated•22 years ago
|
Attachment #106665 -
Flags: review?(cmanske) → review+
Assignee | ||
Comment 18•22 years ago
|
||
I've just discovered a case in which the menupopup might not be the first child.
Attachment #106665 -
Attachment is obsolete: true
Comment 19•22 years ago
|
||
Comment on attachment 107284 [details] [diff] [review] Fixed problem with menupopup assumption Question: Why did you change from the ".collapsed" back to "setAttribute("collapsed","true") notation?
Comment 20•22 years ago
|
||
Comment on attachment 107284 [details] [diff] [review] Fixed problem with menupopup assumption r=cmanske Great! This fixes the intilialization problem I was seeing in Composer toolbar
Attachment #107284 -
Flags: review+
Comment 21•22 years ago
|
||
reassigning to neil since he did the work.
Assignee: hyatt → neil
Status: ASSIGNED → NEW
Updated•22 years ago
|
Attachment #107284 -
Flags: superreview?(hewitt)
Assignee | ||
Comment 22•22 years ago
|
||
Comment on attachment 107284 [details] [diff] [review] Fixed problem with menupopup assumption >@@ -448,14 +424,14 @@ > { > if (SeeMore) > { >- gDialog.MoreSection.collapsed = true; >+ gDialog.MoreSection.setAttribute("collapsed","true"); > gDialog.MoreFewerButton.setAttribute("more","0"); > gDialog.MoreFewerButton.setAttribute("label",GetString("MoreProperties")); > SeeMore = false; > } > else > { >- gDialog.MoreSection.collapsed = false; >+ gDialog.MoreSection.removeAttribute("collapsed"); > gDialog.MoreFewerButton.setAttribute("more","1"); > gDialog.MoreFewerButton.setAttribute("label",GetString("FewerProperties")); > SeeMore = true; Doh! I was diffing old code against new code. This bit doesn't belong.
Comment 23•22 years ago
|
||
*** Bug 62185 has been marked as a duplicate of this bug. ***
Comment 24•22 years ago
|
||
*** Bug 164033 has been marked as a duplicate of this bug. ***
Comment 25•22 years ago
|
||
*** Bug 102675 has been marked as a duplicate of this bug. ***
Comment 26•22 years ago
|
||
*** Bug 134245 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 27•22 years ago
|
||
*** Bug 136912 has been marked as a duplicate of this bug. ***
Comment 28•22 years ago
|
||
Comment on attachment 107284 [details] [diff] [review] Fixed problem with menupopup assumption Please note all the dups we recently found. This fixes a lot of long-standing problems!
Attachment #107284 -
Flags: approval1.3a?
Comment 29•22 years ago
|
||
If this is gonna make 1.3a then it needs to land today. Please get super-review quickly.
Comment 30•22 years ago
|
||
Comment on attachment 107284 [details] [diff] [review] Fixed problem with menupopup assumption sr=dveditz
Attachment #107284 -
Flags: superreview?(hewitt) → superreview+
Comment 31•22 years ago
|
||
Patch for bug 132257 is currently blocked by ths bug because EdDialogCommon.js already call menulist.removeAllItems which currently generate a JS error and therefore block any further JS to be executed.
Blocks: 132257
Does comment 22 imply that a new patch is needed?
Comment 34•22 years ago
|
||
dbaron: do I really have to attach a new patch? My build doesn't include those 2 lines changed by Neil's patch. Trust me!
If we're going to ship 1.3alpha tomorrow, someone who isn't familiar with the patch might need to do the checkin if we decide to take it, and in that case it's nice to have a current patch attached to the bug.
Comment 36•22 years ago
|
||
This is the exact patch to be checked in. The EdDialogCommon.js changes were already checked in as the fix to bug 183055. At the time, there was full expectation that this would be checked in at the same time, but for some reson (still not explained more than a week later), the SR was not supplied.
Attachment #107284 -
Attachment is obsolete: true
Comment 37•22 years ago
|
||
Comment on attachment 108841 [details] [diff] [review] menulist fix v2 since no code change, transferring r= and sr= from previous patch.
Attachment #108841 -
Flags: superreview+
Attachment #108841 -
Flags: review+
Comment 38•22 years ago
|
||
dbaron: new bugzilla code makes it difficult to transfer reviews from previous versions! :(
Comment 39•22 years ago
|
||
umm. sr=deveditz on the menulist.xml changes for "menulist fix v2" As described, the EdDialogCommon.js changes were already checked in, but dbaron requested an exact new patch. New system prevents transferring review notation with original reviewer credit.
Assignee | ||
Updated•22 years ago
|
Attachment #108841 -
Flags: approval1.3a?
Comment 40•22 years ago
|
||
Since this wasn't approved for 1.3a checkin, I backed out minimal changes from bug 183055 so menulists in Publish and Advanced Edit dialogs still work.
Target Milestone: mozilla1.2alpha → mozilla1.3beta
Assignee | ||
Updated•22 years ago
|
Attachment #108841 -
Flags: approval1.3a?
Comment 41•22 years ago
|
||
Comment on attachment 107284 [details] [diff] [review] Fixed problem with menupopup assumption thanks charlie.
Attachment #107284 -
Flags: approval1.3a? → approval1.3a-
Updated•22 years ago
|
Flags: blocking1.3a? → blocking1.3a-
Comment 42•22 years ago
|
||
Since cmanske backed up change in EdDialogCommon.js, this bug does not block anymore bug 132257
No longer blocks: 132257
Comment 43•22 years ago
|
||
checked into 1.3b trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 44•22 years ago
|
||
It turns out that the "removeAllItems" doesn't work when called from JS for the editable menulist. Addition XBL needed -- patch comming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 45•22 years ago
|
||
Comment on attachment 108841 [details] [diff] [review] menulist fix v2 This was checked in, but is insufficient
Attachment #108841 -
Attachment is obsolete: true
Comment 46•22 years ago
|
||
It seems the "removeAllItems" method for id="menulist" is not public to users of editable menulist (id="menulist-editable"), so we have to add a method there too.
Updated•22 years ago
|
Attachment #110524 -
Flags: superreview?(alecf)
Attachment #110524 -
Flags: review?(glazman)
Comment 47•22 years ago
|
||
Comment on attachment 110524 [details] [diff] [review] additional fix v1 sr=alecf
Attachment #110524 -
Flags: superreview?(alecf) → superreview+
Comment 48•22 years ago
|
||
it seems removeAllItems works correctly
Comment on attachment 110524 [details] [diff] [review] additional fix v1 I am reluctant to r= this fix since it _should_ really inherit from the binding. I understand the importance of this workaround and could reconsider this decision if no other option is available.
Attachment #110524 -
Flags: review?(glazman) → review-
Comment 50•22 years ago
|
||
Very weird. I backed out patch and now I can't reproduce the problem either :-/
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → WORKSFORME
Comment 51•22 years ago
|
||
Comment on attachment 110524 [details] [diff] [review] additional fix v1 this patch is not needed
Attachment #110524 -
Attachment is obsolete: true
Updated•22 years ago
|
Summary: menulist does not display selected item → editable menulist does not display selected item
Comment 52•22 years ago
|
||
*** Bug 181802 has been marked as a duplicate of this bug. ***
Comment 53•22 years ago
|
||
*** Bug 165286 has been marked as a duplicate of this bug. ***
Comment 54•22 years ago
|
||
*** Bug 184010 has been marked as a duplicate of this bug. ***
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•