178212 - menulist does not drop down when it is in a listbox cell

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Kevin Doyle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016

Using the following code I can see the arrow on the drop down lit box fine. 
Nothing happens when I click on it.  I have to click on the box part of the menu
list for anything to happen.  

Also when I set the contents as non-editable, the user can edit them anyway.  I
have used editable="none" and editable="false" and taken out this property so
that the default would kick in, all with no success.

As you can also see I have had to include JS code to mimic some of the normal
funcionality of a menulist.
 
I do not get this issue when the menulist is in grid widget or just alone in a
window.

<script type="application/x-javascript">

function myListBox(listboxid) {
  //alert(listboxid);

  var listbox = document.getElementById(listboxid);
  if(listbox.open) {
   listbox.open = false;
  }
  else {
   listbox.open = true;
  } 
  return;
}

function myPopUpMenu(listboxid) {
  //alert(listboxid);

  var listbox = document.getElementById(listboxid);
  if(listbox.open) {
   listbox.open = false;
  }
  else {
   listbox.open = true;
  } 
  return;
}

</script>
 

 <groupbox flex="2">
   <caption label="Phyiscal Characteristics"/>

   <listbox rows="2" class="list" id="physicalcharacteristics-list" flex="1">

    <listcols>
     <listcol flex="2"/>
     <splitter state="open" class="tree-splitter"/>
     <listcol flex="1"/>
     <splitter class="tree-splitter"/>
     <listcol flex="1"/>
     <splitter class="tree-splitter"/>
    </listcols>

    <listhead>
     <listheader label="Description" />
     <listheader label="Value" />
     <listheader label="UOM" />
    </listhead>



    <listitem id="height-item">
     <listcell label="Height" />
     <listcell>
      <textbox id="heightvaluevalue-text-cell" class="textcell" value="68.00"
flex="1"/>
     </listcell>
     <listcell type="menupopup ">
       <menulist label="" id="heightuom-text-cell" editable="none"
onclick="myListBox('heightuom-text-cell')">
         <menupopup  flex="1" onclick="myPopUpMenu('heightuom-text-cell')" >
           <menuitem label="Inches"/>
           <menuitem label="Feet"/>
           <menuitem label="Centimeters"/>
           <menuitem label="Milimeters"/>
         </menupopup>
       </menulist>
     </listcell>
    </listitem>

   </listbox>
  </groupbox>

Reproducible: Always

Steps to Reproduce:
1. Use code above
2. Click on arrow to left on menu list, nothing happens
3. Select an item from teh list by clicking in the text area of the list and
choosing your value.
4. You can then edit the value in the text area, you should not be able to this.

Actual Results:  
Result where that the arrow does not work in the menu list and that the listitem
value in the textarea of the menulist iseditable.

Expected Results:  
It should allow the user to click on the arrow of the menulist so that the user
can select a value from th list.  After the user has selected a value from the
list by click the list should disapear and the textare of the menulist should
have the value the user choose ffrom the list.  This value in teh text are
should not be editable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

XUL

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jan, any ideas?

Kevin, could you possibly post a complete XUL testcase using
http://bugzilla.mozilla.org/attachment.cgi?bugid=178212&action=enter ?

Comment 3

[Kevin Doyle]

Attachment: XUL file with menulist working and not working

You will notice the menulists on the right work as they should.  When the
menulist on the left in the list box it does not work the same.  It is also set
to be none editable and it allows edits.  There is JavaScript to make the menu
behave somewhat as expected.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> It is also set to be none editable 

The editable attribute does not take values.  If it's set, the list is editable.
 If it's not set, the list is not editable.

If I remove the "editable" part, I see the problem with the menulist not
dropping down... confirming and over to trees.

Comment 5

[Ariel Fatecha]

well, if you put in: <lisitem allowevents="true">
all is fine.
maybe you want to modify this in the XBL listbox.xml.
just put it in the content's tag:
<content allowevents="true">
I will post a patch file for listbox.xml
----
about editable="none", in xul.css:
menulist[editable]{
so...1) do nothing; or 2)
menulist[editable="true"]{ 

I think it's better "do nothing".

Comment 6

[Ariel Fatecha]

Attachment: proposed patch for listbox.xml

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113835 [details] [diff] [review]
proposed patch for listbox.xml

Jag?  Neil? What do you think?

Comment 8

[Ariel Fatecha]

I have an not desired secondary effect, with my patch:
you can't, select the complete row, because you make an allowevent.

Comment 9

[Ariel Fatecha]

ok, I will post a new proposed patch.
the anonymous tag listrows is who make exclution rule (if seltype="single") or
add listitem (if seltype="multiple"). it does that in handler tags, testing if
event.target.localName == "listitem"

allowevents="true" it's necesary to allow click inside the listitem, but that
makes change it its event.target. In the XUL file attached, in first listcell
event.target is label, in second is a textbox, a so on...

to solve it, in listitem binding add one handler event="click", there I create
an event type MouseEvent which takes parameter from foreign click event and use
it. So listrows tag feel no diference...

Comment 10

[Ariel Fatecha]

Attachment: proposed patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ariel, please move the review flags over to your updated patch....

Comment 12

[Ariel Fatecha]

well, during the weekend I have tested the patch, and it has a problem with
multiple selections. (seltype="multiple"), because it doesn't have a listener
for mouse event "mousedown" in the listitem tag. so I will post a new patch (I
would like to think that it's my last patch in this bug :-) ).
The new one has tested better than the previous one. 

Comment 13

[Ariel Fatecha]

Attachment: a much better patch

Comment 14

[[e-mail]]

Comment on attachment 114014 [details] [diff] [review]
a much better patch

> allowevents="true" it's necesary to allow click inside the listitem,
> but that makes change it its event.target. In the XUL file attached,
> in first listcell event.target is label, in second is a textbox, and
> so on...

Then wouldn't it be better to locate the containing listitem?

Comment 15

[Ariel Fatecha]

you can't localice the listitem. You don't know how deep can be the target in
the listitem. so, you would need a diferent code for:
<listitem label="blabla"/> in the most common case,
and:
<listitem>
  <listcell label="blabla"/>
  <listcell label="otherbla"/>
</listitem>
and 
<listitem>
  <listcell>
     <menulist>
        <menupopup>
           <menuitem label="one"/>
        </menupopup>
     </menulist>
  </listcell>
  <listcell>
     <label value="bla"/>
  </listcell>
</listitem>

all of this options have diferente references to the listitem tag. So you can't
take the listitem from the event.target.

exist other way:
1. handle "click" and "mousedown" in listitem.
2. from handler call to method in the listrow's implementation.
3. methods in 2 are the actual handlers of "click" and "mousedown", and I pass
the listitem's reference.
if you prefer this option, I would write other patch, for you can have more
options...

Comment 16

[Ariel Fatecha]

I will post a new patch based in methods, not in events... so works too, just
it's a diferente proposed patch...
I delete handlers from listrows tag, and I added them as methods in listbox tag.
I put two handler in listitem to call this methods.

Comment 17

[Ariel Fatecha]

Attachment: other kind of patch

perseverancia....

Comment 18

[[e-mail]]

Comment on attachment 114027 [details] [diff] [review]
other kind of patch

This is usable but I would prefer you to put all the code in the handler,
rather than using the two extra methods.

Also your patch did not apply; one problem was the white-space only diffs, the
other problem is that you appear to be using a three month old version of
listbox.xml

Comment 19

[Ariel Fatecha]

well, I understood... it's time!!! :-) [but later it's better than never]
now, I has last listbox.xml.
I put the methods in the handlers, and the handlers in listitem tag...
it has tested and works...
just it's waiting I post it...

Comment 20

[Ariel Fatecha]

Attachment: I would like to think that it's my last patch in this bug

fixing warnings! :-)

Comment 21

[[e-mail]]

Comment on attachment 114113 [details] [diff] [review]
I would like to think that it's my last patch in this bug

I still had to apply the patch manually :-(

Comment 22

[Boris Zbarsky [:bzbarsky]]

So... I am given to understand (by hyatt) that <handler>s are recompiled every
single time they are called, unlike <method>s which are compiled once per class
(not even once per object).  THe upshot is that moving code _out_ of handlers
may be a good idea.

Check with hyatt to verify that, please....

Comment 23

[Ariel Fatecha]

apparently that's right.
In nsXBLPrototypeHandler::ExecuteHandler(...) exist a call to
CompileEventHandler(...) who make it.
But I need <handler>s even if I put methods, for to call them.
So I think is the same.
Boris, if you want than I post other patch with methods, make a comment then
I'll do it. I have one is done.

Comment 24

[Boris Zbarsky [:bzbarsky]]

_if_ I understand this correctly, it's not the same because compiling the JS
every time can be a noticeable perf hit....  but again, I don't really know this
neck of the woods that well yet; hyatt, neil, and jag are much more familiar
with xbl than I am.

Comment 25

[Ariel Fatecha]

I agree with you. 
but I think that I can't remove the <handler>s, just reduce its content (that's
putting in a method).

Comment 26

[Ariel Fatecha]

Attachment: patch with methods

handler's content in methods

Comment 27

[Boris Zbarsky [:bzbarsky]]

Of course.  All I was saying is it may be worthwhile to make the <handler>s as
short as possible.

Comment 28

[[e-mail]]

Or you could just ask hyatt why it's necessary to recompile <handlers> :-P
Anyway, what's the perf hit on a mouse click?

Comment 29

[Ariel Fatecha]

this bug is important to me, because it can change part of my work.
(and I want to have a bug patched by me :-))
So I like to see this fixed soon.
For that reason I want to turn obsolete one patch. I prefer keep the reviewed+
(for obvious reasons).
boris? neil? comments?

Comment 30

[Jan Varga [:janv]]

sorry, I'm not a superreviewer

Comment 31

[Ariel Fatecha]

Comment on attachment 114113 [details] [diff] [review]
I would like to think that it's my last patch in this bug

varga: ok, thanks, I'm looking for in superreviewers list now...

Comment 32

[Boris Zbarsky [:bzbarsky]]

ben doesn't really read bugmail, so if you want his review you'll have to mail
him directly....

My comments on the business with obsoleting the patch which should be faster are
"eh".

Comment 33

[Ariel Fatecha]

ok, so I turn it to not obsolete, and obsoleting other... :-)
I thought that keeping the patch which has reviewed+ would make It close
faster... but I was wrong ... :-(

Comment 34

[Boris Zbarsky [:bzbarsky]]

Ariel, you asked for my opinion.  That was my opinion.  I'm not the module owner
for this code nor a major contributor to it.  Neil, Jan, hyatt, Ben are.  So you
should go with whatever they say.

Comment 35

[Ariel Fatecha]

Ariel, you asked for my opinion.  That was my opinion.  I'm not the module owner
for this code nor a major contributor to it.  Neil, Jan, hyatt, Ben are.  So you
should go with whatever they say.

Comment 36

[Ariel Fatecha]

:-) my mistake...
ok, I did open previous patch, I'll mail Ben for a comment and superreview... thanks
Boris, you are a layout superreview, rigth?

Comment 37

[Jan Varga [:janv]]

Comment on attachment 114136 [details] [diff] [review]
patch with methods

hmm, this patch seems to be reverse
btw, have you tried to use allowevents="true" in your testcase ?
I recall that we needed to use this attribute explicitly, why make it default ?

Comment 38

[Ariel Fatecha]

Jan, if I use allowevents="true" in testcase works fine just if I move listbody
handler's body to listitem.

Comment 39

[Jan Varga [:janv]]

>Jan, if I use allowevents="true" in testcase works fine
that's my point, why don't you use it in your testcase.

if there is another problem and you need to move handlers, fine
but I don't think allowevents should be default.

Comment 40

[Ariel Fatecha]

Jan, one more question:
what is better:
I have to move listbody handler's body in listitem, but where:
all code in listitem's handler, or, in listitem's methods whick called from
listitem's handlers?. Other way to say it: like patch 114113 or like patch 114136?
After that, I'll put new patch. thanks :-)

Comment 41

[Jan Varga [:janv]]

Ariel, I think it's not a big deal. As Neil mentioned it's not perf critical
since it's just a click, so I'm ok with <handlers>
It should be fixed in XBL core code to not recompile it each time if that's the
case.

Comment 42

[Ariel Fatecha]

Attachment: patch with jan's changes

Jan, that patch is exactly like neil review+, except for allowevents="true" by
default. So if you test it don't forget put allowevents="true" in listitem in
testcase.

Comment 43

[Jan Varga [:janv]]

Comment on attachment 116313 [details] [diff] [review]
patch with jan's changes

looks good
r=varga

Comment 44

[jag (Peter Annema)]

Comment on attachment 116313 [details] [diff] [review]
patch with jan's changes

sr=jag

Comment 45

[Jan Varga [:janv]]

checked in, thanks for the patch