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

RESOLVED FIXED in mozilla32

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: psychicsurgeon, Assigned: agi)

Tracking

Trunk
mozilla32
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
What's the result in Chomium?

With FF25 on Win 7, I have false/false/true.
Component: General → DOM: Core & HTML
Flags: needinfo?(psychicsurgeon)
Product: Firefox → Core
In Chromium it's false/true/true.

Jonas, who's doing forms stuff now, again?
Flags: needinfo?(psychicsurgeon) → needinfo?(jonas)
I hear it's jwatt?  ;)
Flags: needinfo?(jonas) → needinfo?(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.
Flags: needinfo?(jwatt)
(Reporter)

Comment 5

5 years ago
(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".
(Assignee)

Comment 6

5 years ago
I think I know where the problem is, I'll post a patch by next week.
Assignee: nobody → agi.novanta
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Version: 23 Branch → Trunk
(Assignee)

Comment 7

5 years ago
Created attachment 8416288 [details] [diff] [review]
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!
Attachment #8416288 - Flags: feedback?(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!
Attachment #8416288 - Flags: feedback?(jst)
Attachment #8416288 - Flags: feedback?(bzbarsky)
Attachment #8416288 - Flags: feedback+
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.
Attachment #8416288 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 10

5 years ago
Created attachment 8416956 [details] [diff] [review]
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!
Attachment #8416288 - Attachment is obsolete: true
Attachment #8416956 - Flags: review?(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.
Attachment #8416956 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 8418484 [details] [diff] [review]
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.
Attachment #8416956 - Attachment is obsolete: true
Attachment #8418484 - Flags: review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed7414cee1d8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.