The 'control' attribute should return the form control the label element is labeling.

In bug 426082 part 3 and part 4 I used a different way to expose the labeled control to C++. Should I change my part 4 there to use nsIDOMNSLabelElement::GetControl instead of nsHTMLLabelElement::GetForContent and drop part 3?

(In reply to comment #3) > In bug 426082 part 3 and part 4 I used a different way to expose the labeled > control to C++. Should I change my part 4 there to use > nsIDOMNSLabelElement::GetControl instead of nsHTMLLabelElement::GetForContent > and drop part 3? Indeed. I've add a comment on bug 426082 and marked this bug as a blocker of bug 426082. That's good you've noticed that so quickly :)

David, I'm wondering if a11y could have any use of this new attribute ?

Comment on attachment 442877 [details] [diff] [review] Patch v1 >+nsGenericHTMLFormElement::IsLabelableControl() const >+{ >+ // Check for non-labelable form controls as they are not numerous. >+ // TODO: datalist should be added to this list. >+ PRInt32 type = GetType(); >+ return type != NS_FORM_FIELDSET && >+ type != NS_FORM_LABEL && >+ type != NS_FORM_OPTION && >+ type != NS_FORM_OPTGROUP && >+ type != NS_FORM_OBJECT && >+ type != NS_FORM_LEGEND; >+} We're starting to have an aweful lot of these. We really should set up a table where all these flags are listed for each control. That way we'll have much less code and no risk of forgetting to list anything in any function. We could rid of the debug-only assertions that check that everything is listed where it should be. We might even get better perf out of it. Mind setting that up? >+nsHTMLLabelElement::GetControl(nsIDOMHTMLElement** aElement) >+{ >+ *aElement = nsnull; >+ >+ nsCOMPtr<nsIContent> content = GetControlContent(); >+ nsCOMPtr<nsIDOMHTMLElement> element = do_QueryInterface(content); >+ >+ NS_IF_ADDREF(*aElement = element); >+ >+ return NS_OK; >+} Just do element.swap(aElement); >+ // We have a @for. The id has to be linked to an element in the same document >+ // and this element should be a labelable form control. >+ nsCOMPtr<nsIDOMDocument> domDoc; >+ rv = GetOwnerDocument(getter_AddRefs(domDoc)); Just do nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(GetOwnerDoc()); >+ nsCOMPtr<nsIDOMElement> domElement; >+ rv = domDoc->GetElementById(elementId, getter_AddRefs(domElement)); >+ NS_ENSURE_SUCCESS(rv, nsnull); Though even better would be to have GetElementById() on nsIDocument which would return an nsIContent*. The current code results in converting nsIContent->nsIDOMElement->nsIFormControl->nsIContent->nsIDOMElement which seems excessive :) Not that perf is really critical here, but I'm sure we could use such a function on nsIDocument anyway (I think it's even being added in other bugs). If you really really prefer not to do any of these things in this bug. Feel free to rerequest.

Comment on attachment 442873 [details] [diff] [review] Tests v1 Instead of document.getElementById, you could simply use the $ function. I.e. $("foo") is the same as document.getElementById("foo") r=me either way

(Not that it matters at all, but I've always hated the jQuery-like syntax $("id"))

(In reply to comment #6) > (From update of attachment 442877 [details] [diff] [review]) > >+nsGenericHTMLFormElement::IsLabelableControl() const > >+{ > >+ // Check for non-labelable form controls as they are not numerous. > >+ // TODO: datalist should be added to this list. > >+ PRInt32 type = GetType(); > >+ return type != NS_FORM_FIELDSET && > >+ type != NS_FORM_LABEL && > >+ type != NS_FORM_OPTION && > >+ type != NS_FORM_OPTGROUP && > >+ type != NS_FORM_OBJECT && > >+ type != NS_FORM_LEGEND; > >+} > > We're starting to have an aweful lot of these. We really should set up a table > where all these flags are listed for each control. That way we'll have much > less code and no risk of forgetting to list anything in any function. We could > rid of the debug-only assertions that check that everything is listed where it > should be. We might even get better perf out of it. > > Mind setting that up? I think we should do something and maybe I should do that as some of my top priority bugs are blocked by other bugs (that's why I worked on that one). However, in this case, I was going to introduce a nsILabelableControl interface for labelable elements for bug 556743. So, if you don't mind, let's consider bug 556743 is going to remove the |IsLabelableControl| method. And I will think on a better system and provide a patch as soon as possible. > >+nsHTMLLabelElement::GetControl(nsIDOMHTMLElement** aElement) > >+{ > >+ *aElement = nsnull; > >+ > >+ nsCOMPtr<nsIContent> content = GetControlContent(); > >+ nsCOMPtr<nsIDOMHTMLElement> element = do_QueryInterface(content); > >+ > >+ NS_IF_ADDREF(*aElement = element); > >+ > >+ return NS_OK; > >+} > > Just do element.swap(aElement); Ok. > >+ // We have a @for. The id has to be linked to an element in the same document > >+ // and this element should be a labelable form control. > >+ nsCOMPtr<nsIDOMDocument> domDoc; > >+ rv = GetOwnerDocument(getter_AddRefs(domDoc)); > > Just do > > nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(GetOwnerDoc()); Ok. > >+ nsCOMPtr<nsIDOMElement> domElement; > >+ rv = domDoc->GetElementById(elementId, getter_AddRefs(domElement)); > >+ NS_ENSURE_SUCCESS(rv, nsnull); > > Though even better would be to have GetElementById() on nsIDocument which would > return an nsIContent*. The current code results in converting > > nsIContent->nsIDOMElement->nsIFormControl->nsIContent->nsIDOMElement > > which seems excessive :) Not that perf is really critical here, but I'm sure we > could use such a function on nsIDocument anyway (I think it's even being added > in other bugs). > > If you really really prefer not to do any of these things in this bug. Feel > free to rerequest. I will do that.

(In reply to comment #7) > (From update of attachment 442873 [details] [diff] [review]) > Instead of document.getElementById, you could simply use the $ function. I.e. > > $("foo") > > is the same as > > document.getElementById("foo") > > r=me either way FWIW, I prefer to keep getElementById().

(In reply to comment #5) > David, I'm wondering if a11y could have any use of this new attribute ? Yes. It allows a11y to not search label element for the form control through the tree. If it's quicker than DOM tree traversal then it's worth to use it. How is it implemented?

(In reply to comment #11) > (In reply to comment #5) > > David, I'm wondering if a11y could have any use of this new attribute ? > > Yes. It allows a11y to not search label element for the form control through > the tree. If it's quicker than DOM tree traversal then it's worth to use it. > How is it implemented? It looks like you are talking about bug 556743 which will let a11y to know which labels linked to a specific form control. This bug let you know which form control (if any) is linked to a specific label. It is implemented by using @for if available then looking at children until a labelable element is found if any.

We need to get labels from control and controls from label.

(In reply to comment #13) > We need to get labels from control and controls from label. Yes, you need this bug (control for labels) and bug 556743 (labels for control).

Jonas, could you re-review this patch ?

(In reply to comment #5) > David, I'm wondering if a11y could have any use of this new attribute ? Thanks for the ping Mounir and please keep cc'ing me when you are unsure of a11y involvement. Marco Zehe has taken on organizing html5 form control a11y work. Please be sure to ping and cc him as well - thanks!

r=sicking I've added a few tests for checking tree order search of labelable elements.

Comment on attachment 443335 [details] [diff] [review] Patch v2 >+nsDocument::GetElementById(const nsAString& aElementId) >+{ >+ nsCOMPtr<nsIAtom> idAtom(do_GetAtom(aElementId)); >+ NS_ENSURE_TRUE(idAtom, nsnull); >+ if (!CheckGetElementByIdArg(idAtom)) >+ return NS_OK; >+ >+ nsIdentifierMapEntry *entry = GetElementByIdInternal(idAtom); >+ NS_ENSURE_TRUE(entry, nsnull); >+ >+ return entry->GetIdContent(); >+} Don't use NS_ENSURE_TRUE here. It warns on failure and there's not necessarily anything gone wrong if we don't find an entry here. >+already_AddRefed<nsIContent> >+nsHTMLLabelElement::GetControlContent() ... >+ nsAutoString elementId; >+ nsresult rv = GetHtmlFor(elementId); >+ NS_ENSURE_SUCCESS(rv, nsnull); ... >+ if (elementId.IsEmpty()) { >+ // No @for, so we are a label for our first form control element. >+ // Do a depth-first traversal to look for the first form control element. > return GetFirstFormControl(this); > } This is technically not correct. For something like <label for=""> you'll use GetFirstFormControl, however the spec seems to say that you should not. Use GetAttr(nsGkAtoms::_for, ...) instead of GetHtmlFor, and check the return value of GetAttr. >+ // We have a @for. The id has to be linked to an element in the same document >+ // and this element should be a labelable form control. >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(GetOwnerDoc()); >+ if (!doc) { >+ return nsnull; >+ } First of all, why do a QueryInterface here? GetOwnerDoc() returns an nsIDocument. Second, no need to hold an owning reference to doc. So simply do: nsIDocument* doc = GetOwnerDoc(); Though come to think of it, you probably want to use GetCurrentDoc(). If the <label> isn't inserted into any document, then it seems strange that <label>.control would return an element that is. The spec is pretty ambiguous though, but I suspect that the intent is the the two elements both have to be *in* the document for the attribute to apply. Feel free to poke hixie if you want clarification. r=me with that fixed.

(In reply to comment #18) > >+ nsAutoString elementId; > >+ nsresult rv = GetHtmlFor(elementId); > >+ NS_ENSURE_SUCCESS(rv, nsnull); > ... > >+ if (elementId.IsEmpty()) { > >+ // No @for, so we are a label for our first form control element. > >+ // Do a depth-first traversal to look for the first form control element. > > return GetFirstFormControl(this); > > } > > This is technically not correct. For something like <label for=""> you'll use > GetFirstFormControl, however the spec seems to say that you should not. Use > GetAttr(nsGkAtoms::_for, ...) instead of GetHtmlFor, and check the return value > of GetAttr. I remember thinking of that but it looks like I've forgot ;) > >+ // We have a @for. The id has to be linked to an element in the same document > >+ // and this element should be a labelable form control. > >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(GetOwnerDoc()); > >+ if (!doc) { > >+ return nsnull; > >+ } > > First of all, why do a QueryInterface here? GetOwnerDoc() returns an > nsIDocument. Second, no need to hold an owning reference to doc. So simply do: > > nsIDocument* doc = GetOwnerDoc(); > > Though come to think of it, you probably want to use GetCurrentDoc(). If the > <label> isn't inserted into any document, then it seems strange that > <label>.control would return an element that is. > > The spec is pretty ambiguous though, but I suspect that the intent is the the > two elements both have to be *in* the document for the attribute to apply. Feel > free to poke hixie if you want clarification. > > r=me with that fixed. Actually, the specs are not ambiguous: the label is labeling a control with @for if they are both in the same document. I had a test with a control not in the document but I didn't have one with a label not in the document... I'm going to attach a patch fixing these errors.

r=sicking

r=sicking

Comment on attachment 444650 [details] [diff] [review] Patch v2.1 >+ /** >+ * This method is similar to GetElementById() from nsIDOMDocument but it >+ * returns a nsIContent instead of a nsIDOMElement. >+ * It prevents converting nsIDOMElement to nsIContent which is already >+ * converted from nsIContent. >+ */ >+ virtual nsIContent* GetElementById(const nsAString& aElementId) = 0; >+ Now that we have Element, this should return Element*

r=sicking Updated tests applying correctly with HEAD.

r=sicking sr=smaug Applying requested changes by Olli.

Updated patch against the current tip.

Updated patch against the current tip. Actually, I removed the new |GetElementById| method because a recent commit from Peter is adding a similar method (but with an extra |nsresult*| argument). I've added this function to nsIDocument and I changed nsHTMLLabelElement to use it.

Pushed as: http://hg.mozilla.org/mozilla-central/rev/6f38122aed35 and http://hg.mozilla.org/mozilla-central/rev/2133c2b61351