Closed Bug 562932 Opened 14 years ago Closed 14 years ago

Implement control attribute for label element

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, html5, Whiteboard: [parity-webkit])

Attachments

(2 files, 8 obsolete files)

The 'control' attribute should return the form control the label element is labeling.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attached patch Tests v1 (obsolete) — Splinter Review
Attachment #442873 - Flags: review?(jonas)
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #442877 - Flags: review?(jonas)
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?
Blocks: 426082
(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.
Attachment #442877 - Flags: review?(jonas)
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
Attachment #442873 - Flags: review?(jonas) → review+
(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).
Attached patch Patch v2 (obsolete) — Splinter Review
Jonas, could you re-review this patch ?
Attachment #442877 - Attachment is obsolete: true
Attachment #443335 - Flags: review?(jonas)
(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!
Attached patch Tests v1.1 (obsolete) — Splinter Review
r=sicking

I've added a few tests for checking tree order search of labelable elements.
Attachment #442873 - Attachment is obsolete: true
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.
Attachment #443335 - Flags: review?(jonas) → review+
(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.
Attached patch Tests v1.2 (obsolete) — Splinter Review
r=sicking
Attachment #443878 - Attachment is obsolete: true
Attached patch Patch v2.1 (obsolete) — Splinter Review
r=sicking
Attachment #443335 - Attachment is obsolete: true
Attachment #444650 - Flags: superreview?(Olli.Pettay)
Whiteboard: [parity-webkit]
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*
Attachment #444650 - Flags: superreview?(Olli.Pettay) → superreview+
Blocks: 556743
Attached patch Tests v1.3 (obsolete) — Splinter Review
r=sicking

Updated tests applying correctly with HEAD.
Attachment #444648 - Attachment is obsolete: true
Attached patch Patch v2.2 (obsolete) — Splinter Review
r=sicking sr=smaug

Applying requested changes by Olli.
Attachment #444650 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch Tests v1.4Splinter Review
Updated patch against the current tip.
Attachment #445692 - Attachment is obsolete: true
Attached patch Patch v2.3Splinter Review
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.
Attachment #445693 - Attachment is obsolete: true
Pushed as: http://hg.mozilla.org/mozilla-central/rev/6f38122aed35 and http://hg.mozilla.org/mozilla-central/rev/2133c2b61351
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.