editable menulist does not display selected item

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
18 years ago
5 years ago

People

(Reporter: david, Assigned: neil)

Tracking

({access})

Trunk
mozilla1.3beta
Other
Windows 2000
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.3a -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xul1.0-widgets-menulist])

Attachments

(2 attachments, 10 obsolete attachments)

Reporter

Description

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

18 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 1

18 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
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

18 years ago
Assignee

Updated

18 years ago
Keywords: patch, review

Comment 3

18 years ago
Didn't aaronl just fix the .open property and add it to menulists?  Had he not
checked in yet?

Updated

18 years ago
Blocks: 70753
Status: NEW → ASSIGNED
Whiteboard: [xul1.0-widgets-menulist]
Target Milestone: --- → mozilla1.0
Assignee

Comment 4

18 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

18 years ago
Posted patch New patch (obsolete) — Splinter Review
Added missing select() method to editable menulists.
Attachment #64727 - Attachment is obsolete: true
Assignee

Comment 6

18 years ago
Attachment #66868 - Attachment is obsolete: true

Comment 7

18 years ago
Adding keyword nsbeta1 to several bugs.
Keywords: nsbeta1
Assignee

Comment 8

18 years ago
Posted patch Added F4 to open menulist (obsolete) — Splinter Review
Attachment #67069 - Attachment is obsolete: true

Comment 9

18 years ago
Neil, F4 should already drop down menulist! Are you sure that you tried that on
the trunk?
Assignee

Comment 10

18 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 11

18 years ago
nsbeta1- per nav triage team
Keywords: nsbeta1nsbeta1-

Comment 12

18 years ago
Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2
Target Milestone: mozilla1.0 → mozilla1.2

Updated

18 years ago
No longer blocks: 70753
Assignee

Comment 13

17 years ago
I've just noticed that editable menulists don't have context menus.
Attachment #70273 - Attachment is obsolete: true

Comment 14

17 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

Updated

17 years ago
No longer blocks: 164535
Assignee

Comment 15

17 years ago
This now allows add/insert to work when there is no predefined menupopup.
Assignee

Updated

17 years ago
Attachment #90749 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #106665 - Flags: review?(cmanske)

Comment 16

17 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

17 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

17 years ago
Attachment #106665 - Flags: review?(cmanske) → review+
Assignee

Comment 18

17 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

17 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

17 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

17 years ago
reassigning to neil since he did the work.
Assignee: hyatt → neil
Status: ASSIGNED → NEW

Updated

17 years ago
Attachment #107284 - Flags: superreview?(hewitt)
Assignee

Comment 22

17 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.
Assignee

Updated

17 years ago
Blocks: 183055

Comment 23

17 years ago
*** Bug 62185 has been marked as a duplicate of this bug. ***

Comment 24

17 years ago
*** Bug 164033 has been marked as a duplicate of this bug. ***

Comment 25

17 years ago
*** Bug 102675 has been marked as a duplicate of this bug. ***

Comment 26

17 years ago
*** Bug 134245 has been marked as a duplicate of this bug. ***
Assignee

Comment 27

17 years ago
*** Bug 136912 has been marked as a duplicate of this bug. ***

Comment 28

17 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?
If this is gonna make 1.3a then it needs to land today. Please get super-review
quickly.

Updated

17 years ago
Blocks: 184216
Comment on attachment 107284 [details] [diff] [review]
Fixed problem with menupopup assumption

sr=dveditz
Attachment #107284 - Flags: superreview?(hewitt) → superreview+
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

Comment 32

17 years ago
Yeah! We finally got an SR.
Status: NEW → ASSIGNED
Flags: blocking1.3a?
Does comment 22 imply that a new patch is needed?

Comment 34

17 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

17 years ago
Posted patch menulist fix v2 (obsolete) — Splinter Review
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

17 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

17 years ago
dbaron: new bugzilla code makes it difficult to transfer reviews from previous
versions! :(

Comment 39

17 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

17 years ago
Attachment #108841 - Flags: approval1.3a?

Comment 40

17 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

17 years ago
Attachment #108841 - Flags: approval1.3a?
Comment on attachment 107284 [details] [diff] [review]
Fixed problem with menupopup assumption

thanks charlie.
Attachment #107284 - Flags: approval1.3a? → approval1.3a-

Updated

17 years ago
Flags: blocking1.3a? → blocking1.3a-
Since cmanske backed up change in EdDialogCommon.js, this bug does not block
anymore bug 132257
No longer blocks: 132257

Updated

17 years ago
No longer blocks: 184216
No longer blocks: 132257

Comment 43

17 years ago
checked into 1.3b trunk
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

Comment 44

17 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

17 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

17 years ago
Posted patch additional fix v1 (obsolete) — Splinter Review
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

17 years ago
Attachment #110524 - Flags: superreview?(alecf)
Attachment #110524 - Flags: review?(glazman)

Comment 47

17 years ago
Comment on attachment 110524 [details] [diff] [review]
additional fix v1

sr=alecf
Attachment #110524 - Flags: superreview?(alecf) → superreview+

Comment 48

17 years ago
Posted file a testcase
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

17 years ago
Very weird. I backed out patch and now I can't reproduce the problem either :-/
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → WORKSFORME

Comment 51

17 years ago
Comment on attachment 110524 [details] [diff] [review]
additional fix v1

this patch is not needed
Attachment #110524 - Attachment is obsolete: true

Updated

17 years ago
Summary: menulist does not display selected item → editable menulist does not display selected item

Comment 52

17 years ago
*** Bug 181802 has been marked as a duplicate of this bug. ***

Comment 53

17 years ago
*** Bug 165286 has been marked as a duplicate of this bug. ***

Comment 54

17 years ago
*** Bug 184010 has been marked as a duplicate of this bug. ***

Updated

11 years ago
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.