942321 - Changing a select option field via javascript doesn't update the validity status of the form

Status: RESOLVED FIXED

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

Description

[psychicsurgeon]

If you have a form with a required <select> element with a single <option> item without a value, and then change the value to something valid, the form still returns invalid.

See this jsfiddle:

http://jsfiddle.net/DxWUs/

It works in chromium, but not firefox. I'm using 23.0a1 (2013-05-09), but it was also reported on FireFox 25.0.1

Comment 1

[Loic]

What's the result in Chomium?

With FF25 on Win 7, I have false/false/true.

Comment 2

[Boris Zbarsky [:bzbarsky]]

In Chromium it's false/true/true.

Jonas, who's doing forms stuff now, again?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I hear it's jwatt?  ;)

Comment 4

[Jonathan Watt [:jwatt]]

I've been doing some forms work, but not touched <select>. I can have a look but I can't see me getting to that before January.

Comment 5

[psychicsurgeon]

(In reply to Loic from comment #1)
> What's the result in Chomium?
> 
> With FF25 on Win 7, I have false/false/true.

Chromium returns false/true/true, which is what I would think would be "correct".

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

I think I know where the problem is, I'll post a patch by next week.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Changing a select option field via javascript updates validity state

So here the problem is what one would expect, namely that when Option's @value changes the Select element is not notified and consequently the validity state not updated.

What I'm doing here is adding a new function to the Select element, OptionValueChanged. This function is called in Option's AfterSetAttr.

I'm not very sure of adding this function, but I haven't find a better way of notifying the Select element. Johnny what do you think?

Ah, I also added a mochitest, but if you feel is not necessary I don't mind removing it.

Thanks!

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 8416288 [details] [diff] [review]
Changing a select option field via javascript updates validity state

This generally looks good to me, asking for additional feedback from bz cause the one thing that's not clear is whether or not there are any performance implications here that I'm not seeing.

Thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8416288 [details] [diff] [review]
Changing a select option field via javascript updates validity state

Having a test is good.  ;)

Having the new function is ok, but we could also just make UpdateValueMissingValidityState public, no?  Was there a reason not to do that?

However, we should probably only call it if Selected() is true, since otherwise our value doesn't affect the value-missing state, and UpdateValueMissingValidityState() is not exactly cheap.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Changing a select option field via javascript updates validity state

I thought that exposing the "UpdateValidityState" was bad because from the perspective of a user of HTMLSelectElement the validity should always be current (does that make sense?).

But now I'm seeing that, e.g. HTMLInputElement exposes the same member function, so it's probably pointless to hide this bit of the implementation.

Thank you both!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8416956 [details] [diff] [review]
Changing a select option field via javascript updates validity state

Why mIsSelected instead of Selected()?  Or if we think they mean the same thing, can we simplify Selected()?

>+    HTMLSelectElement* selectInt = GetSelect();

Just call that "select"?

r=me with those dealt with.

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Changing a select option field via javascript updates validity state ; r=bz

Oh, I missed Selected()! No, probably it's not the same thing. Thanks Boris! Not asking to review again since it's just a name change.

Already pushed to Try here: https://tbpl.mozilla.org/?tree=Try&rev=7e7b0bfad8dc

Good to go! Thank you.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7414cee1d8

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed7414cee1d8