The default bug view has changed. See this FAQ.

Implement the labels attribute

NEW
Unassigned

Status

()

Core
DOM: Core & HTML
7 years ago
2 months ago

People

(Reporter: mounir, Unassigned)

Tracking

(Blocks: 4 bugs, {dev-doc-needed, html5})

Trunk
dev-doc-needed, html5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
"Labelable form-associated elements have a NodeList object associated with them that represents the list of label elements, in tree order, whose labeled control is the element in question. The labels IDL attribute of labelable form-associated elements, on getting, must return that NodeList object.
"

Labellable form elements are: button, input, keygen, meter, output, progress, select, textarea.
Not implemented at the moment: keygen, meter, output and progress.

Updated

7 years ago
Blocks: 559750
(Reporter)

Comment 1

7 years ago
I was going to work on that when I realized it wasn't so easy. The basic situation is:
<label><input></label>
When we can just look at all parents until we found a labelable element.

But this situation is much more complicated:
<input id='i'>
<label for='i'>
Here, I was thinking of calling something like |AppendLabel| when a label has a valid @for and |RemoveLabel| when @for is changed or removed.
Unfortunately it doesn't work in this situation:
<label for='i'>
<script>/* We append an input element with id='i' in the document</script>
Indeed, when @for is added to the label, no element has the 'i' id.

I was thinking of adding something in the middle: something like a manager which would collect label/id tuples and when .control is called, if the labelable element has an id, it can ask this manager which labels are associated to it with @for.
That sounds a bit heavy so I was wondering if there wasn't another solution I am missing.
(Reporter)

Comment 2

7 years ago
Created attachment 445682 [details] [diff] [review]
Tests - WIP
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
(Reporter)

Comment 3

7 years ago
Created attachment 445683 [details] [diff] [review]
Patch - WIP
(Reporter)

Comment 4

7 years ago
I got some issues with this patch and the Cycle Collector. When the CC ran after the tests, I got a lot of asserts and FF is shutting down (I will attach a log).

Peter, by any chance, do you see what I am doing wrong ?
(Reporter)

Comment 5

7 years ago
Created attachment 445685 [details]
Tests log
(Reporter)

Updated

7 years ago
Keywords: html5
(Reporter)

Updated

7 years ago
Depends on: 562932
(Reporter)

Comment 6

7 years ago
Created attachment 445694 [details] [diff] [review]
Ready-to-apply patch

As requested by Olli on IRC, this patch is including "label.control" patch and  has just been done against HEAD. It may help if you want to build and have a look.
As I mentioned on IRC. nsHTMLLabelElement::GetControlContent()
shouldn't return content.get()
(Reporter)

Comment 8

7 years ago
Created attachment 445717 [details]
Tests log

Thank you Olli !
Unfortunately, I got other CC errors :(
Attachment #445685 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Blocks: 567740
Blocks: 583514
(Reporter)

Updated

7 years ago
Keywords: dev-doc-needed

Comment 9

7 years ago
What's the status on this? We'd really need this for a11y, otherwise we need to implement something similar on a11y side because it's perf problem.
(Reporter)

Comment 10

7 years ago
(In reply to comment #9)
> What's the status on this? We'd really need this for a11y, otherwise we need to
> implement something similar on a11y side because it's perf problem.

That's not going to be done for Gecko 2.0 but very likely for the next major version. Do you need that for Firefox 4?

Comment 11

7 years ago
(In reply to comment #10)

> That's not going to be done for Gecko 2.0 but very likely for the next major
> version. Do you need that for Firefox 4?

Yes. It's really huge perf problem for a11y because we look for label@for through mostly whole tree under certain circumstances. So no chance? Tree rules doesn't allow this?
(Reporter)

Comment 12

7 years ago
(In reply to comment #11)
> (In reply to comment #10)
> 
> > That's not going to be done for Gecko 2.0 but very likely for the next major
> > version. Do you need that for Firefox 4?
> 
> Yes. It's really huge perf problem for a11y because we look for label@for
> through mostly whole tree under certain circumstances. So no chance? Tree rules
> doesn't allow this?

Indeed, that would be a new feature and we've just reached the feature freeze.
Maybe we could try to add the feature without the interface change. But I would need some time to do that and I've no idea if that would be possible.

Comment 13

7 years ago
(In reply to comment #12)

> Indeed, that would be a new feature and we've just reached the feature freeze.
> Maybe we could try to add the feature without the interface change. 

Right. You could put this on nsIContent or new specific Gecko internal interface.

> But I would
> need some time to do that and I've no idea if that would be possible.

I'm sure that's possible :) If you get time to finish this then it's great improvement for a11y. I see your patch caches labels that's exactly what I look for.
(Reporter)

Comment 14

6 years ago
Created attachment 509083 [details] [diff] [review]
WIP Patch

I had this in an old laptop. Moving it here.
No longer blocks: 583514

Updated

6 years ago
Blocks: 617528

Updated

6 years ago
No longer blocks: 617528

Comment 15

6 years ago
(In reply to comment #1)
> I was going to work on that when I realized it wasn't so easy. The basic
> situation is:
> <label><input></label>
> When we can just look at all parents until we found a labelable element.
> 
> But this situation is much more complicated:
> <input id='i'>
> <label for='i'>
> Here, I was thinking of calling something like |AppendLabel| when a label
> has a valid @for and |RemoveLabel| when @for is changed or removed.
> Unfortunately it doesn't work in this situation:
> <label for='i'>
> <script>/* We append an input element with id='i' in the document</script>
> Indeed, when @for is added to the label, no element has the 'i' id.
> 
> I was thinking of adding something in the middle: something like a manager
> which would collect label/id tuples and when .control is called, if the
> labelable element has an id, it can ask this manager which labels are
> associated to it with @for.
> That sounds a bit heavy so I was wondering if there wasn't another solution
> I am missing.

What about adding a new hashtable to nsDocument or nsHTMLDocument that would hold label elements for each "for" group.

Actually we need something very similar to GetElementById().

I thought we would need to build a subtree of nodes and then traverse it to get labels in the tree order.
However, I found out that GetElementById() returns the first element in tree order (each addition to id hashtable builds a chain of parents, etc., so elements with the same id are kept always in tree order).

So I think we don't have to reinvent the wheel and just add something similar for "for" attribute like we have for "id".

GetLabels() should be fast and it would cost only additional 4 bytes for each document.

Comment 16

6 years ago
(In reply to comment #15)

> GetLabels() should be fast and it would cost only additional 4 bytes for
> each document.

err, I meant 4 bytes if there is no label with "for" attribute
sorry for the spam

Comment 17

6 years ago
Created attachment 536082 [details] [diff] [review]
prototype patch

Comment 18

6 years ago
Created attachment 536083 [details]
test for prototype patch

Comment 19

6 years ago
Ok, it seems a fix for bug 656377 will create all needed infrastructure for this bug too.

Updated

6 years ago
Attachment #536082 - Attachment is obsolete: true
(Reporter)

Comment 20

6 years ago
It seems that your approach doesn't take into account this case:
<label>
  <input>
</label>

IOW, you don't seem to consider that a label can labelize a child if the for attribute isn't set.

Comment 21

6 years ago
Actually, I do:

nsHTMLInputElement::GetLabels(nsIDOMNodeList** aLabels)

...

nsIContent* parent = GetParent();
if (parent->NodeInfo()->Equals(nsGkAtoms::label, kNameSpaceID_XHTML)) {
  if (l->Length() == 0)
    l->AppendElement(parent);

...

anyway, bz pointed out that there are other issues:
- if two controls have the same id="foo" then <label for="foo"> will only label one of them
- it seems that GetLabels() should return a live list
(Reporter)

Comment 22

6 years ago
(In reply to comment #21)
> Actually, I do:
>
> nsIContent* parent = GetParent();
> if (parent->NodeInfo()->Equals(nsGkAtoms::label, kNameSpaceID_XHTML)) {
>   if (l->Length() == 0)
>     l->AppendElement(parent);

Indeed, but it doesn't see the label if there is an element in-between, like:
<label><label><input></label></label>

> anyway, bz pointed out that there are other issues:
> - if two controls have the same id="foo" then <label for="foo"> will only
> label one of them
> - it seems that GetLabels() should return a live list

I think there are some issues with having .labels being a live list. For example, when the element is removed from the document.
Blocks: 802882
I don't see anything in the spec which says the list should be live.
(Reporter)

Comment 24

5 years ago
(In reply to Olli Pettay [:smaug] from comment #23)
> I don't see anything in the spec which says the list should be live.

That's the magic of HTML specs ;)

According to the specs, "Labelable elements have a NodeList object associated with them that represents the list of label elements, in tree order, whose labeled control is the element in question." [1].
Then, you find out that NodeList is defined in DOMCore and you read that "A NodeList object is a collection of nodes." [2].
So you check what a collection is and you read the first paragraph that says "A collection is an object that represents a lists of DOM nodes. A collection can be either live or static. Unless otherwise stated, a collection must be live." [3].
A that point, you go back to [1], you realize nothing is specified, so the list must be live. No wonder why web developers don't read the specs...

Actually, IIRC, this could be an issue if the element is moved in/out of a document: a collection has a root and the collection should always be created from that root. I remember sending an email to the WHATWG list about that but Hixie waved those concerns (can't find the email though :().

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-lfe-labels
[2] http://dom.spec.whatwg.org/#nodelist
[3] http://dom.spec.whatwg.org/#concept-collection
(Reporter)

Updated

4 years ago
Duplicate of this bug: 850365

Comment 26

3 years ago
As May 2014 – four years after filing this bug, Firefox 29 is still the only browser that does not support the `labels` property for HTMLInputElement.

Is there any progress to that bug? Does this bug retrieve any attention?
All right, all right.

Mounir, I hope you don't mind that I take over this bug. I'll post a patch later today, just need a bit of testing.
Assignee: mounir → agi.novanta
Created attachment 8428845 [details] [diff] [review]
Implements @labels attribute

OK I can't find any more bugs in this patch.

This patch is loosely adapted from Mounir WIPs (thanks!)
I'm using nsReferencedElement to observe the element pointed by the @for attribute. And as in Mounir's patch I'm using nsIMutationObserver to observe mutations in the Control element pointed by the Label. 

The only part of which I'm not really sure is in HTMLLabelElement::BindToTree, I'm using nsIDocumentObserver to defer SetControl() if we are in the middle of Loading process, because if we call SetControl immediately, we don't have childrens! And @lables is not properly set.

I added a testcase with a few corner cases that caused problems (e.g. the <label><script></label> case) but I can improve with other cases if you feel it's necessary!

Olli do you think you can take a look at this patch? I would really appreciate it :)

Try here: https://tbpl.mozilla.org/?tree=Try&rev=636ef05ff0d5

Thanks!
Attachment #8428845 - Flags: review?(bugs)
Will take a look. Just a tiny bit slower reviews this week.
Created attachment 8430524 [details] [diff] [review]
Implements @labels attribute

I fixed some problems that I found during try, I'll post a diff of the other patch in case you started looking at it.

* Removed an unused member variable

* Removed some whitespace changhes

* There was a leaking because both the labeled element and the label were keeping strong references to each other. I made mControl a weak reference to fix this. Alternatively I could create a new Class to have a weak-referenced version of nsBasicContentList.

* Changed |this| to |MOZ_THIS_IN_INITIALIZER_LIST()| to fix compiler warnings in here.

    9.26  public:
    9.27    HTMLLabelElement(already_AddRefed<nsINodeInfo>& aNodeInfo)
    9.28      : nsGenericHTMLFormElement(aNodeInfo),
    9.29 -      mHandlingEvent(false)
    9.30 +      mHandlingEvent(false),
    9.31 +      mControl(nullptr),
    9.32 +      mForTarget(MOZ_THIS_IN_INITIALIZER_LIST())
    9.33    {


Thank you Olli and take your time :)

Try Android & Linux: https://tbpl.mozilla.org/?tree=Try&rev=3e8cd22c3223
Try Windows (fixed warning):  https://tbpl.mozilla.org/?tree=Try&rev=58e17078c995
Attachment #8428845 - Attachment is obsolete: true
Attachment #8428845 - Flags: review?(bugs)
Attachment #8430524 - Flags: review?(bugs)
Created attachment 8430525 [details] [diff] [review]
Diff patch
Comment on attachment 8430524 [details] [diff] [review]
Implements @labels attribute

>+class nsILabelableElement : public nsISupports
>+{
>+public:
>+
>+  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ILABELABLEELEMENT_IID);
>+
>+  virtual ~nsILabelableElement() {}
>+
>+  void AppendLabel(nsIContent* aContent) {
Nit, { goes to its own line in case of a method

>@@ -60,37 +60,40 @@ HTMLButtonElement::HTMLButtonElement(alr
>   : nsGenericHTMLFormElementWithState(aNodeInfo),
>     mType(kButtonDefaultType->value),
>     mDisabledChanged(false),
>     mInInternalActivate(false),
>     mInhibitStateRestoration(!!(aFromParser & FROM_PARSER_FRAGMENT))
> {
>   // Set up our default state: enabled
>   AddStatesSilently(NS_EVENT_STATE_ENABLED);
>+
>+  mLabelList = new nsSimpleContentList(this);
This kind of list should be created only if needed. We don't want to use
extra memory and keep the list up-to-date in case no one is using .labels

>+HTMLLabelElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+                               const nsAttrValue* aValue, bool aNotify)
>+{
>+  nsresult rv = nsGenericHTMLFormElement::AfterSetAttr(aNameSpaceID, aName,
>+                                                       aValue, aNotify);
>+  if (aName == nsGkAtoms::_for) {
You want to check namespace too


>+HTMLLabelElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
>+                             nsIContent* aBindingParent,
>+                             bool aCompileEventHandlers)
>+{
>+  nsresult rv = nsGenericHTMLFormElement::BindToTree(aDocument, aParent,
>+                                                     aBindingParent,
>+                                                     aCompileEventHandlers);
>+  // If the document is still loading we need to defer SetControl()
>+  // until the document is loaded, becase it's possible that this label's
>+  // childrens have not been constructed yet and thus SetControl() may act on
>+  // inconsistent data.
>+  if (aDocument &&
>+      aDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_LOADING) {
>+    aDocument->AddObserver(this);
>+    NS_ADDREF_THIS();
>+  } else {
>+    SetControl();
>+  }
I don't quite understand this setup. 
.labels would not work before load event has fired?
I assume you want something similar to DoneAddingChildren.

Addrefing here and releasing in EndLoad feels very error prone, and I
don't see any need for such addref/release. When label element is about to be deleted, it must
unregister itself from observing mutations.

Also, making all the labels to observe document can cause quite bad performance issues,
especially if all the changes end up calling SetControl() which QIs.
That isn't exactly cheap in such a hot code path.


>+HTMLLabelElement::EndLoad(nsIDocument *aDocument)
Nit, nsIDocument* aDocument


>+
>+  // The form control the label element is labeling
>+  nsILabelableElement* mControl;
Scary to have raw pointers, but looks like the patch does clear mControl when the element is about to be deleted.
Attachment #8430524 - Flags: review?(bugs) → review-

Comment 33

3 years ago
I feel like we should not introduce any more live lists given that developers dislike them.

Instead this attribute should return a snapshot of the data in a JavaScript array. With

var x = i.labels
x === i.labels

being true. We should replace the snapshot when the DOM has changed. We should implement that lazily (either you return the cached object, or if some flag is set that indicates changes, you check if you need to return something new).
But are other implementations ready to change the behavior?

Comment 35

3 years ago
I did not realize this was already implemented. Disregard comment 33 in that case. Don't want to chase this around across multiple vendors for such a small feature.

Comment 36

2 years ago
Are you still working on this bug?
Flags: needinfo?(agi.novanta)
Oh no sorry!
Assignee: agi.novanta → nobody
Flags: needinfo?(agi.novanta)

Updated

2 years ago
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.