596511 - Implement the required attribute for HTMLSelectElement

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

This has been recently added to whatwg html specifications. However, the implementation might be different from what is specified. This may change after beta7.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Add the required attribute to the select element

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Have :required apply on <select> when appropriate

Comment 3

[Boris Zbarsky [:bzbarsky]]

Are we aiming to land this for 2.0 (and hence need approval)?

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> Are we aiming to land this for 2.0 (and hence need approval)?

Part 3 is still WIP (there is a bug when we have <select><option selected>).
But very likely, this should be in 2.0 for feature completeness.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - <select> can be invalid if no option is selected or all of them have empty value

I do not really like this patch but it seems to work now. I will try to clean that if I can and write some more tests to be sure I'm not missing something.

BTW, if there is <select><option selected>foo</option></select>, it seems that having selected sets make mSelectedIndex change (equals 0 instead of -1) and the validity is checked. However, when this code is called, the value is empty so <select> thinks to suffer from value missing. The simplest fix I've found is to update the validity state when all children are added. I don't really like that :(
This code is far from being nice so I will try to found a better way to fix that but I really doubt.

Jonas, could you tell me what you think of that patch in the meantime?

Comment 6

[Mounir Lamouri (:mounir)]

Should that be a blocker for completeness of the feature?

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - <select> can be invalid if no option is selected or all of them have empty value

Comment 8

[Johnny Stenback (:jst)]

Yes, let's finalize this for 4.0 if we can.

Comment 9

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

Comment on attachment 481810 [details] [diff] [review]
Part 3 - <select> can be invalid if no option is selected or all of them have empty value

This looks like it's getting the aNotify wrong too :( I really hate the setup we have for this, but I can't think of anything we can do for FF4.

Please re-request if you disagree

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Inter diff for part 3

Inter diff for aNotify changes.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - <select> can be invalid if no option is selected or all of them have empty value

This patch should be aNotify compliant.
This thing is *so* annoying. We really need some kind of global variable instead of passing arguments everywhere.

Comment 12

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

UpdateValueMissingValidityState() needs to take aNotify so that it can forward it to nsHTMLFormElement::UpdateValidity :(

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> UpdateValueMissingValidityState() needs to take aNotify so that it can forward
> it to nsHTMLFormElement::UpdateValidity :(

See: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLFormElement.cpp?force=1#1757
According to bz (bug 580575), if we were taking aNotify here, it would be correct to use it for the node it would come from but not for all form elements so it's better to just update for every nodes assuming everything would be fine.

Comment 14

[(no longer active)]

Hmm, a new web facing feature which is a beta N blocker...  Not objecting, just pointing this out...

Comment 15

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

Well, the spec changed under us. @required didn't exist on <select> when Mounir implemented it, but has been added since.

Comment 16

[(no longer active)]

(In reply to comment #15)
> Well, the spec changed under us. @required didn't exist on <select> when Mounir
> implemented it, but has been added since.

Fair enough.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Add the required attribute to the select element

Olli, can you review the change here? I've added a new interface per jst recommandation so I can push that for Gecko 2.0.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 490851 [details] [diff] [review]
Part 1 - Add the required attribute to the select element

># HG changeset patch
># Parent a42e9b001bc86834134c57d15f519a3b67eb8fcf
># User Mounir Lamouri <[e-mail]>
># Date 1289911636 -3600
>Bug ??? - r=smaug
>
>diff --git a/content/html/content/src/nsHTMLSelectElement.cpp b/content/html/content/src/nsHTMLSelectElement.cpp
>--- a/content/html/content/src/nsHTMLSelectElement.cpp
>+++ b/content/html/content/src/nsHTMLSelectElement.cpp
>@@ -177,18 +177,19 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> NS_IMPL_ADDREF_INHERITED(nsHTMLSelectElement, nsGenericElement)
> NS_IMPL_RELEASE_INHERITED(nsHTMLSelectElement, nsGenericElement)
> 
> 
> DOMCI_NODE_DATA(HTMLSelectElement, nsHTMLSelectElement)
> 
> // QueryInterface implementation for nsHTMLSelectElement
> NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED(nsHTMLSelectElement)
>-  NS_HTML_CONTENT_INTERFACE_TABLE3(nsHTMLSelectElement,
>+  NS_HTML_CONTENT_INTERFACE_TABLE4(nsHTMLSelectElement,
>                                    nsIDOMHTMLSelectElement,
>+                                   nsIDOMHTMLSelectElement_Mozilla20,


Not sure whether there is any standard way to mark interfaces to be 2.0 branch.
During 1.8 time we marked interface with _1_8_BRANCH
So maybe nsIDOMHTMLSelectElement_2_0_BRANCH

> 	nsIDOMHTMLSelectElement.idl		\
>+	nsIDOMHTMLSelectElement_Mozilla20.idl		\

Too many tabs?


>+[scriptable, uuid(9b63d2d3-ccb0-4eed-a9e5-d1c142a805b7)]
>+interface nsIDOMHTMLSelectElement_Mozilla20 : nsISupports
>+{
>+           attribute boolean                     required;
>+};
>+
Make the interface extend nsIDOMHTMLSelect element. That way you don't
need to increase the size of nsHTMLSelectElement objects.
(nsHTMLSelectElement should inherit only nsIDOMHTMLSelectElement_2_0_BRANCH)
And IMO, there is no need for new file for this temporary trivial interface.
Just put it to nsIDOMHTMLSelectElement.idl

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Add the required attribute to the select element

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 490865 [details] [diff] [review]
Part 1 - Add the required attribute to the select element

> // QueryInterface implementation for nsHTMLSelectElement
> NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED(nsHTMLSelectElement)
>   NS_HTML_CONTENT_INTERFACE_TABLE3(nsHTMLSelectElement,
>-                                   nsIDOMHTMLSelectElement,
>+                                   nsIDOMHTMLSelectElement_Mozilla_2_0_Branch,
You can't remove nsIDOMHTMLSelectElement from QI.
Just add the new interface to it.

Comment 21

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/433b919425d6
http://hg.mozilla.org/mozilla-central/rev/ccc6520fee81
http://hg.mozilla.org/mozilla-central/rev/96df19871028

Comment 22

[Eric Shepherd [:sheppy]]

This is already documented:

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

Comment 23

[gadjo]

If the <select> element is invalid the hint width is equal to field width, which sometimes doesn't look nice, but I guess hints will be replaces with arrow panels, right?

Comment 24

[gadjo]

Attachment: Invalid hint width

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to comment #23)
> If the <select> element is invalid the hint width is equal to field width,
> which sometimes doesn't look nice, but I guess hints will be replaces with
> arrow panels, right?

Yes, we are just waiting for some reviews. It will be done for Firefox 4.