Last Comment Bug 562932 - Implement control attribute for label element
: Implement control attribute for label element
Status: RESOLVED FIXED
[parity-webkit]
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.3a5
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
http://dev.w3.org/html5/spec/forms.ht...
Depends on:
Blocks: html5forms 556743 559750 426082
  Show dependency treegraph
 
Reported: 2010-04-30 08:55 PDT by Mounir Lamouri (:mounir)
Modified: 2010-08-20 13:32 PDT (History)
10 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests v1 (3.83 KB, patch)
2010-04-30 17:58 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Patch v1 (16.77 KB, patch)
2010-04-30 18:24 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (20.42 KB, patch)
2010-05-04 06:43 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Tests v1.1 (4.92 KB, patch)
2010-05-06 07:37 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v1.2 (5.31 KB, patch)
2010-05-11 06:21 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.1 (20.36 KB, patch)
2010-05-11 06:24 PDT, Mounir Lamouri (:mounir)
bugs: superreview+
Details | Diff | Splinter Review
Tests v1.3 (5.17 KB, patch)
2010-05-17 06:19 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.2 (20.36 KB, patch)
2010-05-17 06:20 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v1.4 (5.18 KB, patch)
2010-05-19 18:58 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.3 (17.84 KB, patch)
2010-05-19 19:00 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-04-30 08:55:25 PDT
The 'control' attribute should return the form control the label element is labeling.
Comment 1 Mounir Lamouri (:mounir) 2010-04-30 17:58:21 PDT
Created attachment 442873 [details] [diff] [review]
Tests v1
Comment 2 Mounir Lamouri (:mounir) 2010-04-30 18:24:57 PDT
Created attachment 442877 [details] [diff] [review]
Patch v1
Comment 3 Markus Stange [:mstange] 2010-05-01 01:35:56 PDT
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?
Comment 4 Mounir Lamouri (:mounir) 2010-05-01 05:37:45 PDT
(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 :)
Comment 5 Mounir Lamouri (:mounir) 2010-05-01 05:41:05 PDT
David, I'm wondering if a11y could have any use of this new attribute ?
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-03 13:56:31 PDT
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 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-03 13:58:50 PDT
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
Comment 8 Olli Pettay [:smaug] 2010-05-03 16:17:36 PDT
(Not that it matters at all, but I've always hated the jQuery-like syntax $("id"))
Comment 9 Mounir Lamouri (:mounir) 2010-05-03 17:02:23 PDT
(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.
Comment 10 Mounir Lamouri (:mounir) 2010-05-03 17:04:16 PDT
(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().
Comment 11 alexander :surkov 2010-05-03 17:26:26 PDT
(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?
Comment 12 Mounir Lamouri (:mounir) 2010-05-03 17:35:36 PDT
(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.
Comment 13 alexander :surkov 2010-05-03 19:56:59 PDT
We need to get labels from control and controls from label.
Comment 14 Mounir Lamouri (:mounir) 2010-05-04 03:02:03 PDT
(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).
Comment 15 Mounir Lamouri (:mounir) 2010-05-04 06:43:34 PDT
Created attachment 443335 [details] [diff] [review]
Patch v2

Jonas, could you re-review this patch ?
Comment 16 David Bolter [:davidb] 2010-05-04 07:08:35 PDT
(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!
Comment 17 Mounir Lamouri (:mounir) 2010-05-06 07:37:19 PDT
Created attachment 443878 [details] [diff] [review]
Tests v1.1

r=sicking

I've added a few tests for checking tree order search of labelable elements.
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-10 15:25:18 PDT
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.
Comment 19 Mounir Lamouri (:mounir) 2010-05-11 06:18:23 PDT
(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.
Comment 20 Mounir Lamouri (:mounir) 2010-05-11 06:21:18 PDT
Created attachment 444648 [details] [diff] [review]
Tests v1.2

r=sicking
Comment 21 Mounir Lamouri (:mounir) 2010-05-11 06:24:14 PDT
Created attachment 444650 [details] [diff] [review]
Patch v2.1

r=sicking
Comment 22 Olli Pettay [:smaug] 2010-05-17 05:33:58 PDT
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*
Comment 23 Mounir Lamouri (:mounir) 2010-05-17 06:19:55 PDT
Created attachment 445692 [details] [diff] [review]
Tests v1.3

r=sicking

Updated tests applying correctly with HEAD.
Comment 24 Mounir Lamouri (:mounir) 2010-05-17 06:20:42 PDT
Created attachment 445693 [details] [diff] [review]
Patch v2.2

r=sicking sr=smaug

Applying requested changes by Olli.
Comment 25 Mounir Lamouri (:mounir) 2010-05-19 18:58:21 PDT
Created attachment 446425 [details] [diff] [review]
Tests v1.4

Updated patch against the current tip.
Comment 26 Mounir Lamouri (:mounir) 2010-05-19 19:00:12 PDT
Created attachment 446426 [details] [diff] [review]
Patch v2.3

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.

Note You need to log in before you can comment on or make changes to this bug.