Implement control attribute for label element

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, html5})

Trunk
mozilla1.9.3a5
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-webkit], URL)

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

7 years ago
The 'control' attribute should return the form control the label element is labeling.
(Assignee)

Updated

7 years ago
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 442873 [details] [diff] [review]
Tests v1
Attachment #442873 - Flags: review?(jonas)
(Assignee)

Comment 2

7 years ago
Created attachment 442877 [details] [diff] [review]
Patch v1
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?
(Assignee)

Updated

7 years ago
Blocks: 426082
(Assignee)

Comment 4

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

Comment 5

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

Comment 8

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

Comment 9

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

Comment 10

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

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

Comment 12

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

7 years ago
We need to get labels from control and controls from label.
(Assignee)

Comment 14

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

Comment 15

7 years ago
Created attachment 443335 [details] [diff] [review]
Patch v2

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!
(Assignee)

Comment 17

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

Comment 19

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

Comment 20

7 years ago
Created attachment 444648 [details] [diff] [review]
Tests v1.2

r=sicking
Attachment #443878 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
Created attachment 444650 [details] [diff] [review]
Patch v2.1

r=sicking
Attachment #443335 - Attachment is obsolete: true
Attachment #444650 - Flags: superreview?(Olli.Pettay)

Updated

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

Updated

7 years ago
Blocks: 556743
(Assignee)

Comment 23

7 years ago
Created attachment 445692 [details] [diff] [review]
Tests v1.3

r=sicking

Updated tests applying correctly with HEAD.
Attachment #444648 - Attachment is obsolete: true
(Assignee)

Comment 24

7 years ago
Created attachment 445693 [details] [diff] [review]
Patch v2.2

r=sicking sr=smaug

Applying requested changes by Olli.
Attachment #444650 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 25

7 years ago
Created attachment 446425 [details] [diff] [review]
Tests v1.4

Updated patch against the current tip.
Attachment #445692 - Attachment is obsolete: true
(Assignee)

Comment 26

7 years ago
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.
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
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5

Updated

7 years ago
Blocks: 559750
(Assignee)

Updated

7 years ago
Flags: in-testsuite+

Updated

7 years ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.