Implement the required attribute for HTMLSelectElement

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on 1 bug, {dev-doc-complete, html5})

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(5 attachments, 4 obsolete attachments)

Assignee

Description

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

Comment 1

9 years ago
Attachment #475456 - Flags: review?(Olli.Pettay)
Attachment #475456 - Flags: review?(Olli.Pettay) → review+
Attachment #475460 - Flags: review?(Olli.Pettay) → review+
Assignee

Updated

9 years ago
Whiteboard: [mounir-g2.0]
Assignee

Updated

9 years ago
Whiteboard: [mounir-g2.0]
Are we aiming to land this for 2.0 (and hence need approval)?
Assignee

Comment 4

9 years ago
(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.
Assignee

Comment 5

9 years ago
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)
Assignee

Comment 6

9 years ago
Should that be a blocker for completeness of the feature?
blocking2.0: --- → ?
Assignee

Comment 7

9 years ago
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)
Assignee

Comment 10

9 years ago
Inter diff for aNotify changes.
Assignee

Comment 11

9 years ago
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)
Assignee

Updated

9 years ago
Whiteboard: [needs review]
UpdateValueMissingValidityState() needs to take aNotify so that it can forward it to nsHTMLFormElement::UpdateValidity :(
Assignee

Comment 13

9 years ago
(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

9 years ago
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.

Comment 16

9 years ago
(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.
Assignee

Comment 17

9 years ago
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-
Assignee

Comment 19

9 years ago
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+
Assignee

Updated

9 years ago
Blocks: 612730
Assignee

Comment 21

9 years ago
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: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla2.0b8

Comment 23

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

9 years ago
Posted image Invalid hint width
Assignee

Comment 25

9 years ago
(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.

Updated

8 years ago
Blocks: 679464

Updated

7 years ago
Depends on: 778101
You need to log in before you can comment on or make changes to this bug.