603141 - Dropdown is not rendered

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Pascal Finette [:pfinette]]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101009 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101009 Firefox/4.0b8pre

On this particular page the drop down for the shoe size is not rendered at all.  Works in other browsers.

Reproducible: Always

Steps to Reproduce:
1. Go to URL
2. Dropdown should be next to image (where green arrow is)
3. No dropdown
Actual Results:  
Nothing - there is no dropdown

Expected Results:  
Render the dropdown

Comment 1

[Mats Palmgren (inactive)]

The dropdown at the site is plain HTML generated by MSDropDown,
a jQuery plugin. (the site uses: MSDropDown 2.1, jQuery v1.4.2)

An example dropdown looks something like this in Fx 3.6.x:
<div style="width: 253px;" id="itemColorDDL_msdd" class="dd">
<div id="itemColorDDL_title" class="ddTitle">
  <span id="itemColorDDL_arrow" class="arrow"></span>
  <span class="textTitle" id="itemColorDDL_titletext">
  <img src="iconColorPicker.gif" align="left"> Choose Color</span>
</div>
<div style="width: 251px;" id="itemColorDDL_child" class="ddChild">
  <a href="javascript:void(0);">
    <img src="iconColorPicker.gif" align="left"> Choose Color</a>
  <a href="javascript:void(0);">
    <img src="2009_Icon_Tarmac_Ventilated_Boots_Black.jpg"> Black</a>
  <a href="javascript:void(0);">
    <img src="2009_Icon_Tarmac_Ventilated_Boots_White.jpg"> White</a>
</div></div>

In a trunk build the first child DIV "itemColorDDL_title" is missing.
MSDropDown uses the following expression to decide if it should be
generated or not (generates it when false):
a(this).attr("size") > 0 || a(this).attr("multiple") == true

In jQuery, a(this).attr("size") boils down to DOMElement["size"] on
a real <select> (hidden with CSS).
In Fx3.6.x we get zero if the "size" attribute isn't specified on
the <select>, on trunk we get 1.

Here's a testcase:
data:text/html,<select></select><script>alert(document.getElementsByTagName('select')[0]["size"])</script>

So, it appears MSDropDown/jQuery depends on a bug that we fixed.

Comment 2

[Mats Palmgren (inactive)]

This bug should probably go to Evangelism?
The fix for the site is simple: specify size="1" on the <select>s.

MSDropDown is from the following site?
http://www.marghoobsuleman.com/jquery-image-dropdown

Comment 3

[Mats Palmgren (inactive)]

The sample at the MSDropDown site doesn't work in Fx4:
http://www.marghoobsuleman.com/mywork/jcomponents/image-dropdown/samples/index.html
and it uses the latest version - 2.3

Comment 4

[Mats Palmgren (inactive)]

(I left a note in the contact form at the MSDropDown site)

Comment 5

[Mounir Lamouri (:mounir)]

So, the script should change the current condition from:
a(this).attr("size") > 0 || a(this).attr("multiple") == true
to:
a(this).attr("size") > 1 || a(this).attr("multiple") == true

Right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Do other browsers return 1 for size in this situation?  If so, why does it work there?  Sniffing on the part of this site?

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> Do other browsers return 1 for size in this situation?  If so, why does it work
> there?  Sniffing on the part of this site?

They return 0 but as far as I understand the specs, it should be 1.

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> (In reply to comment #6)
> > Do other browsers return 1 for size in this situation?  If so, why does it work
> > there?  Sniffing on the part of this site?
> 
> They return 0 but as far as I understand the specs, it should be 1.

Tested with Opera 10.7, Chrome 7 and Safari 5 (not tested with IE).

Comment 9

[Boris Zbarsky [:bzbarsky]]

IE8 returns 0 for the testcase in comment 1 at the end.

If all browsers consistently return 0 and returning 1 leads to site breakage, we should just change the spec to match reality instead of making gratuitous changes, no?  And go back to our old behavior?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Either way, deciding that needs to happen ASAP; ideally by b8.

Mounir, do you want to raise this issue on the spec, or should I?

Comment 11

[Mounir Lamouri (:mounir)]

I'm wondering if this change is breaking so many things. <select></select> and <select size='1'></select> should have the exact same behavior (and rendering). 

A clever check for "there is only one option shown and selectable" for Firefox 3.6, Opera, Chrome, Safari, IE should be:
select.size <= 1 && !select.multiple

With the new specifications (Firefox 4), it should be:
select.size == 1 && !select.multiple

So, there is no reason to check against 0 thus the retro-compatibility should be respected.

Should we revert this change because a library was badly checking something and is now broken? This is a real question, I've no experience in retro-compatibility issues.

Comment 12

[Mounir Lamouri (:mounir)]

FWIW, this change is in the tree since 5 months and it's the first reported issue.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Should we revert this change because a library was badly checking something and
> is now broken?

It really depends on the context.  Should we revert a change that made us different from every other browser _and_ broke at least one site?  Yes, imo, unless there's an overwhelming case for making the change.

Comment 14

[Marghoob Suleman]

(In reply to comment #4)
> (I left a note in the contact form at the MSDropDown site)

Thank you Mats and all of you, 
I've joined this forum. I'll fix this. Give me some time. I am busy these days. Sorry for the inconvenience. 

Regards,
MS

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

See spec bug:

http://www.w3.org/Bugs/Public/show_bug.cgi?id=11162

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I think we should move back to default to 0 and we will see what will happen with the specs.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Make select.size returns 0 by default

Better with a refreshed patch...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Do we want to stop returning 4 in the multiple case too?  Why, if so?

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> Do we want to stop returning 4 in the multiple case too?  Why, if so?

I think it would be confusing to default to 0 if multiple isn't set and 4 otherwise.
Like Aryeh said in w3c bug (comment 15), I think we should default to 0 or 1/4 but certainly not 0/4.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488902 [details] [diff] [review]
Part 1 - Make select.size returns 0 by default

Ok.  Always zero was our old behavior, right?

r=me

Comment 21

[Mats Palmgren (inactive)]

Fwiw, that's also what Chrome, Opera and IE8 does.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> Comment on attachment 488902 [details] [diff] [review]
> Patch v1
> 
> Ok.  Always zero was our old behavior, right?

Yes.

Comment 23

[Mounir Lamouri (:mounir)]

Comment on attachment 488902 [details] [diff] [review]
Part 1 - Make select.size returns 0 by default

Actually, I realized there would be a regression with this patch: <select multiple> should show 4 values by default instead of all values with this patch applied. I will attach another patch to fix that.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - <select multiple> should show 4 rows by default

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488976 [details] [diff] [review]
Part 2 - <select multiple> should show 4 rows by default

Wait, doesn't this make this testcase:

data:text/html,<select multiple><option>a<option>b<option>c<option>d<option>e</select>

render differently on trunk from webkit and Fx3.6?  I guess it was already different, but that was a bug, not a feature, I'd think.

It looks like the new behavior matches Opera, though; what does IE do?

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to comment #25)
> Comment on attachment 488976 [details] [diff] [review]
> Part 2 - <select multiple> should show 4 rows by default
> 
> Wait, doesn't this make this testcase:
> 
> data:text/html,<select
> multiple><option>a<option>b<option>c<option>d<option>e</select>
> 
> render differently on trunk from webkit and Fx3.6?  I guess it was already
> different, but that was a bug, not a feature, I'd think.

It wasn't a bug. It's what the specs require:
"The size attribute gives the number of options to show to the user. The size attribute, if specified, must have a value that is a valid non-negative integer greater than zero. If the multiple attribute is present, then the size attribute's default value is 4. If the multiple attribute is absent, then the size attribute's default value is 1.

The display size of a select element is the result of applying the rules for parsing non-negative integers to the value of element's size attribute, if it has one and parsing it is successful. If applying those rules to the attribute's value is not successful, or if the size attribute is absent, the element's display size is the default value of the attribute."

(this has nothing to do with what the IDL attribute returns)

> It looks like the new behavior matches Opera, though; what does IE do?

I should install IE on wine but didn't so I can't test on IE for the moment.

Comment 27

[Mounir Lamouri (:mounir)]

(In reply to comment #26)
> > It looks like the new behavior matches Opera, though; what does IE do?
> 
> I should install IE on wine but didn't so I can't test on IE for the moment.

IE 6 shows 4 rows.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yes, I know what the spec _says_.  I was just questioning _why_ it says it.

Sounds like it went with the IE/Opera behavior over ours, ok.

So IE shows 4 rows no matter what's going on with optgroups?

Comment 29

[Mounir Lamouri (:mounir)]

(In reply to comment #28)
> Yes, I know what the spec _says_.  I was just questioning _why_ it says it.
> 
> Sounds like it went with the IE/Opera behavior over ours, ok.
> 
> So IE shows 4 rows no matter what's going on with optgroups?

It shows 4 lines no matter what's going on with optgroups.
<select multiple>
<optgroup label='foo'>
  <option>opt 1</option>
  <option>opt 2</option>
</optgroup>
<option>opt 3</option>
<option>opt 4</option>
</select>
Will show:
foo
  opt 1
  opt 2
opt 3

Same behavior with IE, Opera and Minefield.

Comment 30

[Boris Zbarsky [:bzbarsky]]

What will it show if optgroup and option have different sizes?

Comment 31

[Mounir Lamouri (:mounir)]

(In reply to comment #30)
> What will it show if optgroup and option have different sizes?

You mean if we style the font-size of optgroup and option for example? It seems like other browsers (IE and Opera at least) don't accept that and our behavior is already buggy with size and optgroup/option with different size. It looks like we are using the biggest row size to compute the global size.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Yes, styling the font-size would do what I was asking about.

We purposefully use the bigger size; the idea is to show at least N rows in their entirety.

I guess if the other browsers don't support that, we can just ignore that issue for now...

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488976 [details] [diff] [review]
Part 2 - <select multiple> should show 4 rows by default

r=me, though this lowest-common-denominator ugliness makes me sad.  :(

Comment 34

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/73fce7283157
http://hg.mozilla.org/mozilla-central/rev/5c43321021b1

We need the dev doc to be updated (and specify that 0 is the default value).

(In reply to comment #33)
> r=me, though this lowest-common-denominator ugliness makes me sad.  :(

Me too but I guess we don't really have the choice, do we?

Comment 35

[Eric Shepherd [:sheppy]]

I have updated:

https://developer.mozilla.org/en/HTML/Element/select

It now says we return 0 by default, and has a note explaining why.

Comment 36

[Mounir Lamouri (:mounir)]

"The default value is 0 unless the multiple attribute is defined, in which case it is 4."

Should be: "The default value is 0."

Comment 37

[Eric Shepherd [:sheppy]]

Fixed.

Comment 38

[Marghoob Suleman]

Guys,
Please download latest fixed from the link below.
http://www.marghoobsuleman.com/jquery-image-dropdown

Thank you all and sorry for inconvenience.
Regards,
Marghoob Suleman