395107 - [FIX]Not updating UI after selection attribute removed from option inside SELECT element

Status: RESOLVED FIXED

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

Description

[cmtalbert]

Attachment: Test case demonstrating the problem

The UI is not being updated after the selected attribute is removed from the second option via javascript.  You can verify by DOM Inspector that the attribute is no longer on the option element. However, the browser still renders the second option as selected in addition to the slection on the first option. This is a regression from Firefox 2.0.0.6.

I have attached a sample test case that demonstrates the bug.

Testing in IE 7, I verify that without any javascript activity, the second element is selected.  If I only enable the javascript that resets the selection to the first element, then the selection UI appears on the first element, but not the second element.  So, in IE's case, the removeAttribute("selected") on the second element is unneeded.  They are probably behaving this way because the select element does not have the "multiple" attribute set.

I wonder if the real issue here is that Firefox is ignoring the fact that the "mupltiple" attribute is not set on the "select" element.

Comment 1

[cmtalbert]

Attachment: Proper rendering of this test for reference.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think we should try to do what IE does here.... Jonas, what do you think?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, I don't think we should do exactly what IE does. Just like IE maps textField.value to textField.getAttribute("value") it maps option.selected to option.getAttribute("selected"). In other words, when the user, or javascript, selects an option, the selected attribute is removed from all other options.

We've never done that, but rather use the DOM attribute only to initiate javascript properties.


It does look like we get our knickers in a knot here though. Both options appear to be selected which clearly is wrong. I don't agree with the reference though as the disabled attribute has an effect. I think we should behave as if the DOM looked like the selected attribute was set on both options, and the first option was disabled. Attaching a testcase.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: reference rendering 2

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So basically it looks like we're just rendering the first option wrong by rendering it as selected.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The problem seems to be on the DOM side, both options have their .selected set to true, which is probably why layout gets all confused.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I think we should behave as if the DOM looked like the selected attribute was
> set on both options

That would be fine, yes.  I wonder why that's not happening...

Jonas, do you want to dig, or should I?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure that this bug is super important, though it is always scary with regressions...

But if you feel you have the time to work on it feel free to.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well.  I don't have the time, in fact, but I did cause the regression, so I feel obligated to fix it...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Bug 401554 might be the same issue as this, and appears on a real live site.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

OK, let's try this.

The basic issue was that toggling .defaultSelected could change the value of .selected but that the nsHTMLSelectElement was never notified of the change.  This patch notifies it.  With this patch, adding an option to a <select> and then setting defaultSelected is equivalent to setting defaultSelected and then adding the option, as far as I can see.  Neither quite matches what the <select> would have looked like when parsed from scratch, but I think that's sorta the behavior we want, given the IE compat needs here.

In brief, the patch does these:
* Do the notifying described above before setting or unsetting the "selected"
  attribute on options.
* Change OnOptionSelected to not effectively call SetSelected on when an option
  with the "selected" attribute set is added to the <select>.  This makes
  setting defaultSelected to false actually work to unselect options which have
  never had SetSelected called.
* Make nsGenericElement::UnsetAttr call Before/AfterSetAttr, and remove some
  code that's no longer needed if we do that.  Fix the one SVG consumer that
  assumed that aValue is never null in BeforeSetAttr.
* Add tests

This patch fixes bug 401554 as well.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Jonas, I think this is risky enough that we should land it ASAP...

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, I hate the options code :(. Does this patch really need to be this big? Bz, would be great if you could give an overview of what's happening, either here on on irc.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Ugh, I hate the options code 

Amen.

> Does this patch really need to be this big?

Not quite.  I could implement set/unset hooks in nsHTMLOptionElement instead of making nsGenericElement::UnsetAttr call BeforeSetAttr.  Doing it the way I did seems simpler, though...  If you prefer, I can revert this part of the patch (so nsGenericElement, nsGenericHTMLElement, and the SVG and XUL changes.  I do think we need the rest of it, though.

For the rest, like I said in comment 11, the basic problem is that the <select> needs to be told any time the .selected state of an option changes.  When it becomes true, the <select> (if it's a single select) goes through and unselects all other options.  Since we now map .selected to .defaultSelected unless .selected has been explicitly set, we need to notify the <select> if .defaultSelected changes and .selected hasn't been set.

That's what the patch does in nsHTMLOptionElement::BeforeSetAttr.  The nsHTMLSelectElement changes are to address the second bullet of comment 11.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 286646 [details] [diff] [review]
Proposed fix

Why not let XUL::unsetattr also call before/aftersetattr? I'd prefer the consistency to the saved cycles I think

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 286646 [details] [diff] [review]
Proposed fix

r/sr = me either way.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Why not let XUL::unsetattr also call before/aftersetattr?

Because the things nsXULElement::UnsetAttr and nsXULElement::AfterSetAttr do subtly different things to the same attributes.  So it was more a matter of complexity and regression risk than cycles...

Thanks for the review!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Need this to build too (missing header change)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked in.