Last Comment Bug 556743 - Implement the labels attribute
: Implement the labels attribute
Status: NEW
: dev-doc-needed, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 33 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
: 850365 (view as bug list)
Depends on: 562932
Blocks: html5forms 559750 567740 html5test
  Show dependency treegraph
 
Reported: 2010-04-02 05:44 PDT by Mounir Lamouri (:mounir)
Modified: 2016-07-11 12:34 PDT (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests - WIP (3.14 KB, patch)
2010-05-17 05:22 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch - WIP (41.09 KB, patch)
2010-05-17 05:22 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests log (52.11 KB, text/plain)
2010-05-17 05:34 PDT, Mounir Lamouri (:mounir)
no flags Details
Ready-to-apply patch (64.45 KB, patch)
2010-05-17 06:26 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests log (355.51 KB, text/plain)
2010-05-17 08:06 PDT, Mounir Lamouri (:mounir)
no flags Details
WIP Patch (53.59 KB, patch)
2011-02-02 05:40 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
prototype patch (8.84 KB, patch)
2011-05-30 06:40 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
test for prototype patch (919 bytes, text/html)
2011-05-30 06:44 PDT, Jan Varga [:janv]
no flags Details
Implements @labels attribute (42.91 KB, patch)
2014-05-26 11:39 PDT, Giovanni Sferro [:agi]
no flags Details | Diff | Splinter Review
Implements @labels attribute (42.90 KB, patch)
2014-05-28 21:43 PDT, Giovanni Sferro [:agi]
bugs: review-
Details | Diff | Splinter Review
Diff patch (7.84 KB, patch)
2014-05-28 21:47 PDT, Giovanni Sferro [:agi]
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-04-02 05:44:07 PDT
"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.
Comment 1 Mounir Lamouri (:mounir) 2010-05-01 07:25:17 PDT
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.
Comment 2 Mounir Lamouri (:mounir) 2010-05-17 05:22:11 PDT
Created attachment 445682 [details] [diff] [review]
Tests - WIP
Comment 3 Mounir Lamouri (:mounir) 2010-05-17 05:22:33 PDT
Created attachment 445683 [details] [diff] [review]
Patch - WIP
Comment 4 Mounir Lamouri (:mounir) 2010-05-17 05:34:12 PDT
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 ?
Comment 5 Mounir Lamouri (:mounir) 2010-05-17 05:34:52 PDT
Created attachment 445685 [details]
Tests log
Comment 6 Mounir Lamouri (:mounir) 2010-05-17 06:26:45 PDT
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.
Comment 7 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2010-05-17 07:31:23 PDT
As I mentioned on IRC. nsHTMLLabelElement::GetControlContent()
shouldn't return content.get()
Comment 8 Mounir Lamouri (:mounir) 2010-05-17 08:06:50 PDT
Created attachment 445717 [details]
Tests log

Thank you Olli !
Unfortunately, I got other CC errors :(
Comment 9 alexander :surkov 2010-11-02 08:10:09 PDT
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.
Comment 10 Mounir Lamouri (:mounir) 2010-11-02 08:11:59 PDT
(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 alexander :surkov 2010-11-02 08:21:57 PDT
(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?
Comment 12 Mounir Lamouri (:mounir) 2010-11-02 08:27:16 PDT
(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 alexander :surkov 2010-11-02 08:31:07 PDT
(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.
Comment 14 Mounir Lamouri (:mounir) 2011-02-02 05:40:06 PST
Created attachment 509083 [details] [diff] [review]
WIP Patch

I had this in an old laptop. Moving it here.
Comment 15 Jan Varga [:janv] 2011-05-28 04:16:06 PDT
(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 Jan Varga [:janv] 2011-05-28 04:18:54 PDT
(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 Jan Varga [:janv] 2011-05-30 06:40:58 PDT
Created attachment 536082 [details] [diff] [review]
prototype patch
Comment 18 Jan Varga [:janv] 2011-05-30 06:44:09 PDT
Created attachment 536083 [details]
test for prototype patch
Comment 19 Jan Varga [:janv] 2011-05-31 12:08:28 PDT
Ok, it seems a fix for bug 656377 will create all needed infrastructure for this bug too.
Comment 20 Mounir Lamouri (:mounir) 2011-06-01 03:40:01 PDT
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 Jan Varga [:janv] 2011-06-01 04:44:14 PDT
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
Comment 22 Mounir Lamouri (:mounir) 2011-06-01 07:11:04 PDT
(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.
Comment 23 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-10-18 04:29:24 PDT
I don't see anything in the spec which says the list should be live.
Comment 24 Mounir Lamouri (:mounir) 2012-10-18 04:57:18 PDT
(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
Comment 25 Mounir Lamouri (:mounir) 2013-03-12 20:41:14 PDT
*** Bug 850365 has been marked as a duplicate of this bug. ***
Comment 26 technik 2014-05-15 03:38:46 PDT
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?
Comment 27 Giovanni Sferro [:agi] 2014-05-26 06:29:25 PDT
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.
Comment 28 Giovanni Sferro [:agi] 2014-05-26 11:39:35 PDT
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!
Comment 29 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2014-05-26 11:54:01 PDT
Will take a look. Just a tiny bit slower reviews this week.
Comment 30 Giovanni Sferro [:agi] 2014-05-28 21:43:59 PDT
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
Comment 31 Giovanni Sferro [:agi] 2014-05-28 21:47:12 PDT
Created attachment 8430525 [details] [diff] [review]
Diff patch
Comment 32 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2014-06-02 10:24:37 PDT
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.
Comment 33 Anne (:annevk) 2014-06-09 00:37:51 PDT
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).
Comment 34 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2014-06-09 06:42:45 PDT
But are other implementations ready to change the behavior?
Comment 35 Anne (:annevk) 2014-06-09 06:47:23 PDT
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 Florian Bender 2014-11-22 05:57:19 PST
Are you still working on this bug?
Comment 37 Giovanni Sferro [:agi] 2014-11-22 08:33:50 PST
Oh no sorry!

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