118038 - editable menulist does not display selected item

Status: RESOLVED WORKSFORME

(Core :: XUL, defect)

Description

[David Casti]

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

Comment 1

[[e-mail]]

Attachment: Proposed patch

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

Comment 2

[[e-mail]]

Attachment: Test of .open property

Comment 3

[David Hyatt]

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

Comment 4

[[e-mail]]

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?

Comment 5

[[e-mail]]

Attachment: New patch

Added missing select() method to editable menulists.

Comment 6

[[e-mail]]

Attachment: Silly me forgot my own fix :-)

Comment 7

[Peter Trudelle]

Adding keyword nsbeta1 to several bugs.

Comment 8

[[e-mail]]

Attachment: Added F4 to open menulist

Comment 9

[Aaron Leventhal]

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

Comment 10

[[e-mail]]

Attachment: F4 wasn't necessary - sorry aaron

OK, but something's still wrong because in build 2002021803 I can't open an
editable menulist with the keyboard.

Comment 11

[Peter Trudelle]

nsbeta1- per nav triage team

Comment 12

[Joe Hewitt (gone)]

Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2

Comment 13

[[e-mail]]

Attachment: Added context menu to editable menulist

I've just noticed that editable menulists don't have context menus.

Comment 14

[Mike Potter]

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.

Comment 15

[[e-mail]]

Attachment: Added support for empty menulists by request of cmanske

This now allows add/insert to work when there is no predefined menupopup.

Comment 16

[Charles Manske]

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.)

Comment 17

[[e-mail]]

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.

Comment 18

[[e-mail]]

Attachment: Fixed problem with menupopup assumption

I've just discovered a case in which the menupopup might not be the first
child.

Comment 19

[Charles Manske]

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

[Charles Manske]

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

Comment 21

[Charles Manske]

reassigning to neil since he did the work.

Comment 22

[[e-mail]]

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

[Charles Manske]

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

Comment 24

[Charles Manske]

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

Comment 25

[Charles Manske]

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

Comment 26

[Charles Manske]

*** Bug 134245 has been marked as a duplicate of this bug. ***

Comment 27

[[e-mail]]

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

Comment 28

[Charles Manske]

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!

Comment 29

[Asa Dotzler [:asa]]

If this is gonna make 1.3a then it needs to land today. Please get super-review
quickly.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 107284 [details] [diff] [review]
Fixed problem with menupopup assumption

sr=dveditz

Comment 31

[Jean-Francois Ducarroz]

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.

Comment 32

[Charles Manske]

Yeah! We finally got an SR.

Comment 33

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Does comment 22 imply that a new patch is needed?

Comment 34

[Charles Manske]

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!

Comment 35

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

[Charles Manske]

Attachment: menulist fix v2

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.

Comment 37

[Charles Manske]

Comment on attachment 108841 [details] [diff] [review]
menulist fix v2

since no code change, transferring r= and sr= from previous patch.

Comment 38

[Charles Manske]

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

Comment 39

[Charles Manske]

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.

Comment 40

[Charles Manske]

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.

Comment 41

[Asa Dotzler [:asa]]

Comment on attachment 107284 [details] [diff] [review]
Fixed problem with menupopup assumption

thanks charlie.

Comment 42

[Jean-Francois Ducarroz]

Since cmanske backed up change in EdDialogCommon.js, this bug does not block
anymore bug 132257

Comment 43

[Charles Manske]

checked into 1.3b trunk

Comment 44

[Charles Manske]

It turns out that the "removeAllItems" doesn't work when called from JS for the
editable menulist. Addition XBL needed -- patch comming.

Comment 45

[Charles Manske]

Comment on attachment 108841 [details] [diff] [review]
menulist fix v2

This was checked in, but is insufficient

Comment 46

[Charles Manske]

Attachment: additional fix v1

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.

Comment 47

[Alec Flett]

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

sr=alecf

Comment 48

[Jan Varga [:janv]]

Attachment: a testcase

it seems removeAllItems works correctly

Comment 49

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

Comment 50

[Charles Manske]

Very weird. I backed out patch and now I can't reproduce the problem either :-/

Comment 51

[Charles Manske]

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

this patch is not needed

Comment 52

[Kathleen :Brade]

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

Comment 53

[Kathleen :Brade]

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

Comment 54

[Kathleen :Brade]

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