Closed Bug 596511 Opened 11 years ago Closed 10 years ago

Implement the required attribute for HTMLSelectElement

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, html5)

Attachments

(5 files, 4 obsolete files)

This has been recently added to whatwg html specifications. However, the implementation might be different from what is specified. This may change after beta7.
Attachment #475456 - Flags: review?(Olli.Pettay)
Attachment #475456 - Flags: review?(Olli.Pettay) → review+
Attachment #475460 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [mounir-g2.0]
Whiteboard: [mounir-g2.0]
Are we aiming to land this for 2.0 (and hence need approval)?
(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.
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?
Attachment #481632 - Flags: feedback?(jonas)
Should that be a blocker for completeness of the feature?
blocking2.0: --- → ?
Attachment #481632 - Attachment is obsolete: true
Attachment #481810 - Flags: review?(jonas)
Attachment #481632 - Flags: feedback?(jonas)
Yes, let's finalize this for 4.0 if we can.
blocking2.0: ? → betaN+
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
Attachment #481810 - Flags: review?(jonas)
Inter diff for aNotify changes.
This patch should be aNotify compliant.
This thing is *so* annoying. We really need some kind of global variable instead of passing arguments everywhere.
Attachment #481810 - Attachment is obsolete: true
Attachment #484327 - Flags: review?(jonas)
Whiteboard: [needs review]
UpdateValueMissingValidityState() needs to take aNotify so that it can forward it to nsHTMLFormElement::UpdateValidity :(
(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.
Hmm, a new web facing feature which is a beta N blocker...  Not objecting, just pointing this out...
Well, the spec changed under us. @required didn't exist on <select> when Mounir implemented it, but has been added since.
(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.
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.
Attachment #475456 - Attachment is obsolete: true
Attachment #490851 - Flags: review?(Olli.Pettay)
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 <mounir.lamouri@gmail.com>
># 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
Attachment #490851 - Flags: review?(Olli.Pettay) → review-
Attachment #490851 - Attachment is obsolete: true
Attachment #490865 - Flags: review?(Olli.Pettay)
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.
Attachment #490865 - Flags: review?(Olli.Pettay) → review+
Blocks: 612730
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla2.0b8
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?
Attached image Invalid hint width
(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.
Blocks: 679464
Depends on: 778101
You need to log in before you can comment on or make changes to this bug.