556743 - Implement the labels attribute

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

"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)]

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)]

Attachment: Tests - WIP

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch - WIP

Comment 4

[Mounir Lamouri (:mounir)]

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)]

Attachment: Tests log

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: 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][[e-mail]]]

As I mentioned on IRC. nsHTMLLabelElement::GetControlContent()
shouldn't return content.get()

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Tests log

Thank you Olli !
Unfortunately, I got other CC errors :(

Comment 9

[alexander :surkov (:asurkov)]

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)]

(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 (:asurkov)]

(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)]

(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 (:asurkov)]

(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)]

Attachment: WIP Patch

I had this in an old laptop. Moving it here.

Comment 15

[Jan Varga [:janv]]

(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]]

(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]]

Attachment: prototype patch

Comment 18

[Jan Varga [:janv]]

Attachment: test for prototype patch

Comment 19

[Jan Varga [:janv]]

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

Comment 20

[Mounir Lamouri (:mounir)]

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]]

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)]

(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][[e-mail]]]

I don't see anything in the spec which says the list should be live.

Comment 24

[Mounir Lamouri (:mounir)]

(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 26

[technik]

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

[[ex-Mozilla] Agi Sferro | :agi]

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

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: 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][[e-mail]]]

Will take a look. Just a tiny bit slower reviews this week.

Comment 30

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: 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

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Diff patch

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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)]

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][[e-mail]]]

But are other implementations ready to change the behavior?

Comment 35

[Anne (:annevk)]

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]

Are you still working on this bug?

Comment 37

[[ex-Mozilla] Agi Sferro | :agi]

Oh no sorry!

Comment 40

[John Dai[:jdai]]

I would like to take this bug.

Comment 41

[John Dai[:jdai]]

Attachment: patch, v1

- Support .labels in input, select, textarea, button, meter, output, progress,  except keygen. It seems like keygen has been deprecated[1][2]. 
- Revise labelable-elements.html web platform test because wpt test was wrong in test hidden input element portion.
- Remove expected: FAIL in /web-platform .ini files.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=101019
[2] Google discussion: https://bugs.chromium.org/p/chromium/issues/detail?id=514767

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53784be828f7c2c06d1d9d400fd3e9bd444adb3&filter-tier=1&group_state=expanded

Comment 42

[John Dai[:jdai]]

Keygen .labels was passed by Chromium and Safari at labelable-elements.html. Hence, I didn't remove labelable-elements.html keygen portion.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8858792 [details] [diff] [review]
patch, v1

Jessica, do you think you could take a look at this. You're rather familiar with form control elements these days ;)

One things I noticed is that nsGenericHTMLElement::Labels() seems to always create a new object. That isn't what Chrome does.
inputElement.labels == inputElement.labels is true there.

The spec says for example about input element:
"The labels IDL attribute of labelable elements and input elements, on getting, must return that NodeList object, and that same value must always be returned, unless this element is an input element whose type attribute is in the Hidden state, in which case it must instead return null."

It is unclear to me why Element needs to have Labels() method.

Comment 44

[John Dai[:jdai]]

Thank you for your review.
(In reply to Olli Pettay [:smaug] from comment #43)
> Comment on attachment 8858792 [details] [diff] [review]
> patch, v1
> 
> Jessica, do you think you could take a look at this. You're rather familiar
> with form control elements these days ;)
> 
> One things I noticed is that nsGenericHTMLElement::Labels() seems to always
> create a new object. That isn't what Chrome does.
> inputElement.labels == inputElement.labels is true there.
> 
Yes, you are right. It looks like they cache it. After thinking about cache mechanism, I have some questions want to ask:
1) For no id's node, e.g.,<label><input></label>. It means that I can't use <id, NodeList, isDirty> as a hashtable.
2) For duplicate id nodes, 
ex:
<label for="test">label:</label>
<input id="test"/>
<input id="test"/>
In Chrome, only the first <input> has .labels value. It seems that we can't only use id as a key.
3) The isDirty bit is for lazy update hashtable, it defers until user really call .labels, then update the NodeList. Also, it'll listen mutation events, when there is a node changed, we set the isDirty bit. Does that make sense? 
 
> The spec says for example about input element:
> "The labels IDL attribute of labelable elements and input elements, on
> getting, must return that NodeList object, and that same value must always
> be returned, unless this element is an input element whose type attribute is
> in the Hidden state, in which case it must instead return null."
> 
> It is unclear to me why Element needs to have Labels() method.
It's redundant, I'll remove it in next patch.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

I would expect that using nsContentList (not nsSimpleContentList) would be able to deal with the live-ness of the list correctly.

Comment 46

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #45)
> I would expect that using nsContentList (not nsSimpleContentList) would be
> able to deal with the live-ness of the list correctly.

I'll try to use nsContentList to deal with the live-ness of the list. Thanks for the direction.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Not sure where to store the labels list. DOMSlots is one place, other is some hashtable. Hashtable + [Cached] in .webidl might be good enough and not use too much memory.

Comment 48

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8858792 [details] [diff] [review]
patch, v1

Review of attachment 8858792 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at the previous patches, are we considering adding a LabelableElement class so that labelable elements can inherit from it? Then we can store the labels list in it. It may add a little delay when creating the elements though.

::: dom/html/nsGenericHTMLElement.cpp
@@ +1694,5 @@
> +
> +  for (nsIContent* cur = root->GetFirstChild(); cur;
> +       cur = cur->GetNextNode()) {
> +    if (cur->IsHTMLElement(nsGkAtoms::label)) {
> +      HTMLLabelElement* lableElement = HTMLLabelElement::FromContent(cur);

nit: typo, should be labelElement.

::: testing/web-platform/tests/html/semantics/forms/the-label-element/labelable-elements.html
@@ -102,5 @@
>    testLabelsAttr("testHidden", "lbl5");
>    var labels = hiddenInput.labels;
>  
>    hiddenInput.type = "hidden";
> -  assert_equals(labels.length, 0, "Retained .labels NodeList should be empty after input type changed to hidden");

This test is correct, please don't remove it, see the last example in https://html.spec.whatwg.org/multipage/forms.html#the-label-element.
But do still keep the test you added.

@@ -107,3 @@
>  
>    hiddenInput.type = "checkbox";
> -  assert_true(labels === hiddenInput.labels, ".labels property must return the [SameObject] after input type is toggled back from 'hidden'");

We should keep this one too.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jessica Jong [:jessica] from comment #48)
> Comment on attachment 8858792 [details] [diff] [review]
> patch, v1
> 
> Review of attachment 8858792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking at the previous patches, are we considering adding a
> LabelableElement class so that labelable elements can inherit from it? Then
> we can store the labels list in it. It may add a little delay when creating
> the elements though.
I'm not worried about creation time. If the constructor doesn't really do anything, compiler should optimize it out. I'm more worried about the sizeof() these elements. 
(like in that HTMLInputElement case where we seem to easily move to 1Kb buckets, meaning worse memory locality)

Comment 50

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #49)
> (In reply to Jessica Jong [:jessica] from comment #48)
> > Comment on attachment 8858792 [details] [diff] [review]
> > patch, v1
> > 
> > Review of attachment 8858792 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looking at the previous patches, are we considering adding a
> > LabelableElement class so that labelable elements can inherit from it? Then
> > we can store the labels list in it. It may add a little delay when creating
> > the elements though.
> I'm not worried about creation time. If the constructor doesn't really do
> anything, compiler should optimize it out. I'm more worried about the
> sizeof() these elements. 
> (like in that HTMLInputElement case where we seem to easily move to 1Kb
> buckets, meaning worse memory locality)

Yeah, that too. Although the labels list can be allocated dynamically, inheriting another class will still increase the size of these elements.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

The pointer to the labels list would add 8 bytes on 64 bit systems. (just inheriting from class without new virtual stuff doesn't increase the size ).
Hashtable would use memory only when the feature is actually used. But since hashtables can be a bit slow, using [Cached] in .webidl would keep accessing the property fast.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Though, then one needs to think how to clear the hashtable in a fast manner.

Comment 53

[brunoais]

(In reply to John Dai[:jdai] from comment #44)
> Thank you for your review.
> (In reply to Olli Pettay [:smaug] from comment #43)
> > Comment on attachment 8858792 [details] [diff] [review]
> > patch, v1
> > 
> > Jessica, do you think you could take a look at this. You're rather familiar
> > with form control elements these days ;)
> > 
> > One things I noticed is that nsGenericHTMLElement::Labels() seems to always
> > create a new object. That isn't what Chrome does.
> > inputElement.labels == inputElement.labels is true there.
> > 
> Yes, you are right. It looks like they cache it. After thinking about cache
> mechanism, I have some questions want to ask:
> 1) For no id's node, e.g.,<label><input></label>. It means that I can't use
> <id, NodeList, isDirty> as a hashtable.
> 2) For duplicate id nodes, 
> ex:
> <label for="test">label:</label>
> <input id="test"/>
> <input id="test"/>
> In Chrome, only the first <input> has .labels value. It seems that we can't
> only use id as a key.

I think you are overthinking it. By definition, ´id´ attributes are unique. If more than one element in the DOM has the same ´íd´, it is not following the rules, hence, the result should be undefined although it must not throw an Error or make the browser crash in the process. Firefox may register a warning in the console, though.

Comment 54

[Simon Pieters [:zcorpan]]

Behavior is not undefined, though. It's defined in HTML. If you don't like the defined behavior, maybe it can be changed; file an issue on whatwg/html.

Comment 55

[brunoais]

https://www.w3.org/TR/html5/dom.html#the-id-attribute
Am I seeing anything wrong?

Comment 56

[Simon Pieters [:zcorpan]]

Yes. ^_^

1. Use https://html.spec.whatwg.org/multipage/
2. The requirement that id must be unique is an authoring requirement. It doesn't follow that UA requirement is undefined if the authoring requirement is violated. See first note in https://html.spec.whatwg.org/multipage/infrastructure.html#conformance-classes
3. The UA requirements here are:

> Labelable elements and all input elements have a live 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 elements and input elements, on getting, must return that NodeList object, and that same value must always be returned, unless this element is an input element whose type attribute is in the Hidden state, in which case it must instead return null.

https://html.spec.whatwg.org/multipage/forms.html#dom-lfe-labels

where in particular the requirements around "labeled control" are of interest https://html.spec.whatwg.org/multipage/forms.html#labeled-control

Comment 57

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #52)
> Though, then one needs to think how to clear the hashtable in a fast manner.

I am thinking of keep a WeakPtr as a hashtable's key. Hence, that would be easier to clear the hashtable in a fast manner. Does that make sense?

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

How would WeakPtr help? I assume you mean WeakPtr to element. How would one know that an entry needs to be removed from hashtable?

The easiest option for now would be to use DOMSlots. Put the strong pointer to the list there and add the member variable to traverse/unlink.


Or, hmm, which all elements have .labels? Do all those elements inherit nsIFormControl? Since we do have plenty of free bits there. If that is the case, use a flag telling whether there is
'element (raw pointer) -> list' in the hashtable and then in the destructor of the element check the flag and if set, remove the entry from hashtable.


If .labels is needed for non-nsIFormControls, and we'd like to use hashtable, we'd need a spare bit in Element, but we don't have one atm.
There is bug 1338723 to free one bit.
But using DOMSlots might be just simpler in this case, even though it uses more memory in some cases.
(DOMSlots is getting rather fat.)

Comment 59

[John Dai[:jdai]]

Thank you for your valuable feedback. 

> Or, hmm, which all elements have .labels? 
Only input, select, textarea, button, meter, output, progress and keygen have .labels.

> Do all those elements inherit nsIFormControl? 
Input, select, textarea, button, and output inherit nsIFormControl, meter and progress aren't inherit nsIFormControl.

Comment 60

[John Dai[:jdai]]

Hi bz,
I encountered a build error when I was adding [Cached] at labels attribute in HTMLSelectElement.webidl. It shows “We can't have extra reserved slots for proxy interface HTMLSelectElement”[1]. It’s because this interface has NamedGetter attribute[2]. Do we have another way around to cache the returned labels value on the JS object? Thank you.

[1] http://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#12693-12698
[2] http://searchfox.org/mozilla-central/source/dom/webidl/HTMLSelectElement.webidl#37

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Do we have another way around to cache the returned labels value on the JS object?

Not a sane one.  Basically, you're running into bug 1237504.

You should probably just add a JS::Heap<JSObject*> member to the relevant things, trace it in their CC impls, and cache it that way...  I don't think any of the things listed in comment 59 would be problematic in terms of adding a word to them.

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

HTMLInputElement is very big. It is being refactored a bit, and that should give some more room, but 
we're quite close to going over 512 bytes, and when that happens , memory locality seems to suffer quite a bit (shows up in microbenchmarks), and we end up using 1kB for each HTMLInputElement (because of  mozjemalloc buckets).
There will be static assert in HTMLInputElement to keep it <= 512.

.labels is rare enough, that perhaps slots isn't too bad place.
I wouldn't cache the wrapper using JSObject*, just have the list object in slots.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Oh, comment 33 is not relevant and we have a C++ list object?

In that case yeah, you could do slots, or even just a node property...

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

depending on how commonly these are used. Accessing node properties can be a bit slow.

Comment 65

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v2

- Use DOMSlots to keep labels nodelist.
- Introduce a nsLabelsNodeList which inherits from nsContentList in order to return NodeList.
- Override AttributeChanged mutation update in order to maintain live NodeList object when there is a attritube change on input element.

Hi Jessica,
Could you help to review this patch? Thank you.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e62225c30e8ead88576754f800132f21012794a5&filter-tier=1&group_state=expanded

Comment 66

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v2

- Address comment #48, which is revert original testcase, also append a new added test from mine.

Hi Jessica,
Could you help to review this patch? Thank you.

Comment 67

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8861919 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v2

Review of attachment 8861919 [details] [diff] [review]:
-----------------------------------------------------------------

I think I can just give you feedback, you still need a peer to review this. :)

::: dom/base/nsContentList.cpp
@@ +1093,5 @@
> +void
> +nsLabelsNodeList::AttributeChanged(nsIDocument *aDocument, Element* aElement,
> +                                      int32_t aNameSpaceID, nsIAtom* aAttribute,
> +                                      int32_t aModType,
> +                                      const nsAttrValue* aOldValue)

nit: indentation

@@ +1099,5 @@
> +  NS_ASSERTION(aElement, "Must have a content node to work with");
> +  // We need to handle input is hidden. Just dirty ourselves; this is simpler
> +  // than trying to figure out aElement changed to other type from hidden, or,
> +  // aElement changed its type to hidden.
> +  if (aElement->IsHTMLElement(nsGkAtoms::input)) {

If this is only for cases that input type changes to or from "hidden", can we also check that the attribute is "type"?

@@ +1106,5 @@
> +  }
> +
> +  nsContentList::AttributeChanged(aDocument, aElement, aNameSpaceID, aAttribute,
> +                                  aModType, aOldValue);
> +}
\ No newline at end of file

Do we need to handle the case of label's child removal/addition?
For example: <label><input>/label>, then input is moved to somewhere else.

::: dom/base/nsContentList.h
@@ +583,5 @@
>    static const ContentListType sType;
>  #endif
>  };
>  
> +class nsLabelsNodeList

nit: no need to add new line here if total width is under 80

::: dom/html/nsGenericHTMLElement.cpp
@@ +1699,5 @@
> +
> +
> +already_AddRefed<nsINodeList>
> +nsGenericHTMLElement::Labels()
> +{

Since this method is public, early return on non-labelable elements.

::: dom/html/nsGenericHTMLElement.h
@@ +864,5 @@
>    virtual bool IsLabelable() const override;
> +
> +  static bool MatchLabelsElement(Element* aElement, int32_t aNamespaceID,
> +                                 nsIAtom* aAtom, void* aData);
> +  virtual already_AddRefed<nsINodeList> Labels();

nit: add a extra new line here

Comment 68

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8861922 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v2

Review of attachment 8861922 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/semantics/forms/the-label-element/labelable-elements.html
@@ +103,5 @@
>    var labels = hiddenInput.labels;
>  
>    hiddenInput.type = "hidden";
>    assert_equals(labels.length, 0, "Retained .labels NodeList should be empty after input type changed to hidden");
> +  assert_equals(hiddenInput.labels, null, "Retained .labels NodeList should be null after input type changed to hidden");

nit: remove "Retained" in this message.

Comment 69

[John Dai[:jdai]]

(In reply to Jessica Jong [:jessica] from comment #67)
> Comment on attachment 8861919 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v2
> 
> Review of attachment 8861919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I can just give you feedback, you still need a peer to review this.
> :)
> 
Thanks for your feedback.
> @@ +1099,5 @@
> > +  NS_ASSERTION(aElement, "Must have a content node to work with");
> > +  // We need to handle input is hidden. Just dirty ourselves; this is simpler
> > +  // than trying to figure out aElement changed to other type from hidden, or,
> > +  // aElement changed its type to hidden.
> > +  if (aElement->IsHTMLElement(nsGkAtoms::input)) {
> 
> If this is only for cases that input type changes to or from "hidden", can
> we also check that the attribute is "type"?
> 
Yes, we can. I'll address them in next patch.
> @@ +1106,5 @@
> > +  }
> > +
> > +  nsContentList::AttributeChanged(aDocument, aElement, aNameSpaceID, aAttribute,
> > +                                  aModType, aOldValue);
> > +}
> \ No newline at end of file
> 
> Do we need to handle the case of label's child removal/addition?
> For example: <label><input>/label>, then input is moved to somewhere else.
> 
Yes. we need. I'll address them and add into wpt as well.

Comment 70

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v3

- Address comment #67.
- Revise Labels() to non-virtual function.
- Fix labelable elements moved to somewhere case.
- Add an additional check for the attribute is "type" to AttributeChanged mutation callback.
- Address nits.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5a60ff0296d6860ac5b86de49ad2438d7815d84&filter-tier=1&group_state=expanded

Comment 71

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v3

- Add a testcase for labelable elements moved to somewhere case.

Comment 72

[Boris Zbarsky [:bzbarsky]]

Some comments based on a brief skim:

1)  In nsGenericHTMLElement::MatchLabelsElement, why do you do both the IsHTMLElement() check and the FromContent check?  The FromContent call already checks whether its arg is an HTML <label> and returns null if not.

2)  nsGenericHTMLElement::Labels is using the wrong root node for the nodelist.  In the simplest case, consider a subtree not in the document containing <label><input></label>.  The inputs .labels should contain that label, but a nodelist rooted at the document won't find it.  Needs more tests.

3) nsLabelsNodeList::ContentAppended/Inserted could use some comments about why they're checking IsLabelable(); presumably because if a new labelable element is inserted one of our labels can now become a label for that other labelable element, right?

4) nsLabelsNodeList::ContentAppended needs to consider all the appended content, not just the first one.  Needs more tests.

Comment 73

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #72)
> Some comments based on a brief skim:
> 
> 1)  In nsGenericHTMLElement::MatchLabelsElement, why do you do both the
> IsHTMLElement() check and the FromContent check?  The FromContent call
> already checks whether its arg is an HTML <label> and returns null if not.
> 
You are right, we don't need another IsHTMLElement() check, just use FromContent to determine null or not. 

> 2)  nsGenericHTMLElement::Labels is using the wrong root node for the
> nodelist.  In the simplest case, consider a subtree not in the document
> containing <label><input></label>.  The inputs .labels should contain that
> label, but a nodelist rooted at the document won't find it.  Needs more
> tests.
> 
Yes, I'll use nsINode::SubtreeRoot() to instead and add more tests as well.

> 3) nsLabelsNodeList::ContentAppended/Inserted could use some comments about
> why they're checking IsLabelable(); presumably because if a new labelable
> element is inserted one of our labels can now become a label for that other
> labelable element, right?
> 
Right, I'll add more comments. Thanks for reminding me.

> 4) nsLabelsNodeList::ContentAppended needs to consider all the appended
> content, not just the first one.  Needs more tests.
I'll address them in next patch and add more tests.

Comment 74

[Boris Zbarsky [:bzbarsky]]

> Yes, I'll use nsINode::SubtreeRoot() to instead and add more tests as well.

And make sure to test what happens when the subtree root changes....

Comment 75

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8862408 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v3

Review of attachment 8862408 [details] [diff] [review]:
-----------------------------------------------------------------

I guess there's a new patch coming, so clearing feedback for now.

Comment 76

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v4

I would like to explain about this change,

- nsGenericHTMLElement::Labels uses nsINode::SubtreeRoot() to set default root node. It's because labelable elements in the DocumentFragment should return empty NodeList while using .labels. 

- I also add a check at nsGenericHTMLElement::MatchLabelsElement() in order to make sure all the associated labels should be in the document before adding to the content list.

- nsLabelsNodeList::ContentAppended/Inserted/Removed add a function named SetChildsToDirty(), which is to clean all the labelable child nodes' content list. It's because we need to consider labelable elements are moved to outside or inside of nested associated labels via node.appendChild() or node.insertBefore().

- If a labels element is moved to different subtree which has a different root, I reset root node of labelable element in nsGenericHTMLElement::BindToTree.

- Add more tests to reflect above changed, it will be in the latter patch. 

Hi Olli,
Could you help review this patch when you have time? Thank you.

Comment 77

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v4

Comment 78

[Boris Zbarsky [:bzbarsky]]

>+        slots->mLabelsList->Root() != aDocument) {
>+      slots->mLabelsList->SetRoot(aDocument);

That doesn't look right in the aDocument == nullptr case.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Also the root might need to change on unbind.

And you need to dirty the list on SetRoot, right?

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866332 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v4

>+nsLabelsNodeList::SetChildsToDirty(nsIContent* aContainer, nsIContent* aChild)
s/Childs/Children/
But perhaps the method name should be something else, since we aren't setting the children dirty but 'this'.
Perhaps MarkDirtyIfLabelableChildren ?

And why does the method need to iterate all the descendant nodes?


>+
>+    // We need to consider a labels element is moved to another subtree
>+    // which is different root.
>+    nsDOMSlots *slots = GetExistingDOMSlots();
Nit, nsDOMSlots* slots

>+    if (slots && slots->mLabelsList &&
>+        slots->mLabelsList->Root() != aDocument) {
>+      slots->mLabelsList->SetRoot(aDocument);
>+    }
As bz said, what if aDocument is nullptr?

And what about UnbindFromTree?

Comment 81

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866343 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v4

Based on the previous patch, this is missing cases when dealing with elements not in document or when removing elements from dom.

Comment 82

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #78)
> >+        slots->mLabelsList->Root() != aDocument) {
> >+      slots->mLabelsList->SetRoot(aDocument);
> 
> That doesn't look right in the aDocument == nullptr case.
Thanks for your feedback. I'll fix them in next patch. 

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #79)
> Also the root might need to change on unbind.
> 
> And you need to dirty the list on SetRoot, right?
Right, I'll fix them in next patch.

Comment 83

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #80)
> Comment on attachment 8866332 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v4
> 
> >+nsLabelsNodeList::SetChildsToDirty(nsIContent* aContainer, nsIContent* aChild)
> s/Childs/Children/
> But perhaps the method name should be something else, since we aren't
> setting the children dirty but 'this'.
> Perhaps MarkDirtyIfLabelableChildren ?
> 
> And why does the method need to iterate all the descendant nodes?
> 
Thanks for your review. It's because it'll append/insert/remove all the descendant nodes and file a mutation event.

> >+    if (slots && slots->mLabelsList &&
> >+        slots->mLabelsList->Root() != aDocument) {
> >+      slots->mLabelsList->SetRoot(aDocument);
> >+    }
> As bz said, what if aDocument is nullptr?
> 
> And what about UnbindFromTree?
We need to also handle root node change at UnbindFromTree. I'll fix them in next patch. 

(In reply to Olli Pettay [:smaug] from comment #81)
> Comment on attachment 8866343 [details] [diff] [review]
> Bug 556743 - Update web-platform tests for labels attribute. v4
> 
> Based on the previous patch, this is missing cases when dealing with
> elements not in document or when removing elements from dom.
I covered elements not in document test in label-attributes.html via createDocumentFragment(). I'll add removing elements from dom in the testcase. Thank you.

Comment 84

[John Dai[:jdai]]

I should also consider a labelable element inside the shadow DOM. I'll address them in next patch.

Comment 85

[Boris Zbarsky [:bzbarsky]]

> It's because it'll append/insert/remove all the descendant nodes and file a mutation event.

That's .. not what it (it == SetChildsToDirty) does.  It just does SetDirty() when it finds something labelable.  Why can't it just stop when it finds the first labelable thing?

Also, in the ContentInserted/ContentRemoved cases it's walking nodes that have nothing to do with the insertion/removal, no?

> I covered elements not in document

The thing that needs testing is the aDocument null in BindToTree case: an element not in a document being appended to another element not in a document.

Comment 86

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #85)
> > It's because it'll append/insert/remove all the descendant nodes and file a mutation event.
> 
> That's .. not what it (it == SetChildsToDirty) does.  It just does
> SetDirty() when it finds something labelable.  Why can't it just stop when
> it finds the first labelable thing?
> 
Thank you for your feedback. It's because we allow using 'for' to label an element. Consider a case:
<label id="lbl1"><input id="test1"><input id="test2"></label>
<label for="test2"></label>

Labels length of test1: 1
Labels length of test2: 1

Now, we removeChild of lbl1 or move test1 into another subtree which is different root.
Expect labels length of test1: 0
Expect labels length of test2: 0

If we stop SetChildsToDirty() when it finds the first labelable element, we can't handle correctly at test2, its labels length still remains 1. 

> Also, in the ContentInserted/ContentRemoved cases it's walking nodes that
> have nothing to do with the insertion/removal, no?
> 
You are right, GetNextNode() pass parent node, which will also traverse sibiling nodes, and it's wrong in this implementation. I should change GetNextNode() pass current node and it'll traverse child nodes only. Thank you. 

> > I covered elements not in document
> 
> The thing that needs testing is the aDocument null in BindToTree case: an
> element not in a document being appended to another element not in a
> document.

This case I didn't cover in testcase, I'll address them in next patch.

Comment 87

[Boris Zbarsky [:bzbarsky]]

> If we stop SetChildsToDirty() when it finds the first labelable element, we can't handle correctly at test2

Why, exactly?

In that testcase, there are two separate nodelists: one for test1 and one for test2.  They both get ContentRemoved notifications.  They both start walking the removed content.

For each one, once it's called SetDirty() once in SetChildsToDirty (which is rather misnamed), it has no more work to do in that function and can stop.

Comment 88

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #87)
> > If we stop SetChildsToDirty() when it finds the first labelable element, we can't handle correctly at test2
> 
> Why, exactly?
> 
> In that testcase, there are two separate nodelists: one for test1 and one
> for test2.  They both get ContentRemoved notifications.  They both start
> walking the removed content.
> 
> For each one, once it's called SetDirty() once in SetChildsToDirty (which is
> rather misnamed), it has no more work to do in that function and can stop.

Thanks for the explanation, after further trace code, the thing like you said, they both get ContentRemoved notifications.  So we can stop when it finds the first labelable element.

Comment 89

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v5

In this change,

- Support a labelable element inside the shadow DOM.

- Rename SetChildsToDirty() to MarkDirtyToFirstLabelableChildren().

- Revise MarkDirtyToFirstLabelableChildren() to mark dirty for the first labelable element.

- UnbindFromTree and BindToTree both need to reset root node, if a labels element is moved to another subtree which has different root.

- Add two test cases, one is for a labelable element inside the shadow DOM, another is for the aDocument null in BindToTree test case.

Hi Olli,
Could you help review this patch when you have time? Thank you.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a822f7d9b8946b893f231e49036f3800711ccf52&group_state=expanded&filter-tier=1

Comment 90

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v5

Comment 91

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8867619 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v5


>+bool
>+nsLabelsNodeList::MarkDirtyToFirstLabelableChildren(nsIContent* aContainer,
>+                                                    nsIContent* aChild)
Why does this method take aContainer argument?



>+void
>+nsLabelsNodeList::MaybeResetRoot(nsINode* aRootNode)
>+{
>+  NS_ASSERTION(aRootNode, "Must have root");
Use MOZ_ASSERT

>@@ -265,25 +266,30 @@ HTMLLabelElement::GetLabeledElement() const
>   if (!GetAttr(kNameSpaceID_None, nsGkAtoms::_for, elementId)) {
>     // 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 GetFirstLabelableDescendant();
>   }
> 
>   // 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.
>-  //XXXsmaug It is unclear how this should work in case the element is in
>-  //         Shadow DOM.
>-  //         See https://www.w3.org/Bugs/Public/show_bug.cgi?id=26365.
>-  nsIDocument* doc = GetUncomposedDoc();
>-  if (!doc) {
>+  if (!IsInComposedDoc()) {
Why we need this check here, if we have ShadowRoot from which we can get
element by id?

>+nsGenericHTMLElement::MatchLabelsElement(Element* aElement, int32_t aNamespaceID,
>+                                         nsIAtom* aAtom, void* aData)
>+{
>+  HTMLLabelElement* element = HTMLLabelElement::FromContent(aElement);
>+  // We have to filter out a label element not in the document.
Why? I don't see anything in the spec requiring label to be in document.
Is there no wpt test for this case?

In Chrome
var l = document.createElement("label"); l.innerHTML = "<input>"; l.firstChild.labels.length
does say 0, 
but
var l = document.createElement("label"); l.innerHTML = "<input>"; l.control
points to the input element, so I'd say Chrome has a bug with .labels handling.

Comment 92

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8867620 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v5

>@@ -52,23 +84,28 @@
>     assert_not_equals(label.control, document.getElementById("test1"),
>                       "A label element not in a document should not label an element in a document.");
>     document.body.appendChild(label);
>     assert_equals(label.control, document.getElementById("test1"));
>     label.remove();
>   }, "A label element not in a document can not label any element in the document.");
> 
>   test(function () {
>+    var labels = document.getElementById("test3").labels;
>     assert_equals(document.getElementById("lbl1").control, document.getElementById("test3"),
>                   "The first labelable descendant of a label element should be its labeled control.");
> 
>     var input = document.createElement("input");
>     document.getElementById("lbl1").insertBefore(input, document.getElementById("test2"));
>     assert_equals(document.getElementById("lbl1").control, input,
>                   "The first labelable descendant of a label element in tree order should be its labeled control.");
>+    assert_equals(input.labels.length, 1,
>+                  "The form control has an ancestor with no explicit associated label, and is the first labelable descendant.");
>+    assert_equals(labels.length, 0,
>+                  "The number of labels should be 0 if the form control has an ancestor label element that the for attribute points to another control.");
This comment looks wrong. There is no 'for' attribute around here.

>+  test(function () {
>+    var labels1 = document.getElementById("test9").labels;
>+    var labels2 = document.getElementById("test10").labels;
>+    assert_true(labels1 instanceof NodeList,
>+                "A form control's 'labels' property should be an instance of a NodeList.");
>+    assert_equals(labels1.length, 1,
>+                  "The form control has an ancestor with no explicit associated label, and it is the first labelable descendant.");
>+    assert_equals(labels2.length, 1,
>+                  "The number of labels should be 1 since there is a label with a 'for' attribute associated with this labelable element.");
>+    assert_array_equals(labels1, [document.getElementById("lbl9")],
>+                        "The labels for a form control should be returned in tree order.");
>+    assert_array_equals(labels2, [document.getElementById("lbl10")],
>+                        "The labels for a form control should be returned in tree order.");
>+    document.getElementById('div2').appendChild(document.getElementById('obj'));
>+    assert_equals(labels1.length, 0,
>+                  "The number of labels should be 0 after the labelable element is moved to outside of associated label.");
>+    assert_equals(labels2.length, 1,
>+                  "The number of labels should be 0 after the labelable element is moved to outside of associated label.");
The comment is wrong. assert checks for 1, but comment talks about 0


>+  test(function () {
>+    var labels = document.getElementById("test11").labels;
>+    assert_true(labels instanceof NodeList,
>+                "A form control's 'labels' property should be an instance of a NodeList.");
>+    assert_equals(labels.length, 1,
>+                  "The form control has an ancestor with no explicit associated label, and is the first labelable descendant.");
>+    assert_array_equals(labels, [document.getElementById("lbl11")],
>+                        "The labels for a form control should be returned in tree order.");
>+
>+    let iframe = document.getElementById("iframe");
>+    iframe.onload = function () {
>+      iframe.contentWindow.document.getElementById("div1").appendChild(document.getElementById("p1"));
>+      assert_equals(labels.length, 2,
>+                    "The number of labels should be 2 after the labelable element is moved to iframe.");
>+    };
This is racy test. The iframe is possibly already loaded here.



>+  test(function () {
>+    // <label><input id="test13"></label><label for="test13"></label>
>+    var frag = document.createDocumentFragment();
>+    var label = document.createElement('label');
>+    var input = document.createElement('input');
>+    input.setAttribute('id', 'test13');
>+    label.appendChild(input);
>+    var label2 = document.createElement('label');
>+    label2.setAttribute('for', 'test13');
>+    frag.appendChild(label);
>+    frag.appendChild(label2);
>+
>+    var labels = frag.getElementById("test13").labels;
>+    assert_equals(labels.length, 0,
>+                  "The number of labels should be 0 since a label element not in the document.");
Why does that matter? Am I missing something in the spec? Where does it limit .labels handling to in-document cases?

Comment 93

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #91)
> Comment on attachment 8867619 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v5
> 
> 
> >+bool
> >+nsLabelsNodeList::MarkDirtyToFirstLabelableChildren(nsIContent* aContainer,
> >+                                                    nsIContent* aChild)
> Why does this method take aContainer argument?
> 
Thanks for your review. I'll remove it, there is no need aContainer argument.

> >@@ -265,25 +266,30 @@ HTMLLabelElement::GetLabeledElement() const
> >   if (!GetAttr(kNameSpaceID_None, nsGkAtoms::_for, elementId)) {
> >     // 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 GetFirstLabelableDescendant();
> >   }
> > 
> >   // 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.
> >-  //XXXsmaug It is unclear how this should work in case the element is in
> >-  //         Shadow DOM.
> >-  //         See https://www.w3.org/Bugs/Public/show_bug.cgi?id=26365.
> >-  nsIDocument* doc = GetUncomposedDoc();
> >-  if (!doc) {
> >+  if (!IsInComposedDoc()) {
> Why we need this check here, if we have ShadowRoot from which we can get
> element by id?
> 
The reason I add IsInComposedDoc() check is because I want to keep the same the behavior as old version, which is make sure this element should in-document. It's because I use SubtreeRoot() to get root node, it can't tell me this element is in-document or not. Also, if I remove this check, it can't pass wpt[1].

[1] https://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/testing/web-platform/tests/html/semantics/forms/the-label-element/label-attributes.html#49-57
 
> >+nsGenericHTMLElement::MatchLabelsElement(Element* aElement, int32_t aNamespaceID,
> >+                                         nsIAtom* aAtom, void* aData)
> >+{
> >+  HTMLLabelElement* element = HTMLLabelElement::FromContent(aElement);
> >+  // We have to filter out a label element not in the document.
> Why? I don't see anything in the spec requiring label to be in document.
> Is there no wpt test for this case?
> 
I didn't see requiring label to be in document in the spec. However, I tested Chrome and Safari, they both have same behavior, which is to limit .labels handling to in-document. Should I file a spec issue about this? There is a wpt test that:
https://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/testing/web-platform/tests/html/semantics/forms/the-label-element/label-attributes.html#49-57

> In Chrome
> var l = document.createElement("label"); l.innerHTML = "<input>";
> l.firstChild.labels.length
> does say 0, 
> but
> var l = document.createElement("label"); l.innerHTML = "<input>"; l.control
> points to the input element, so I'd say Chrome has a bug with .labels
> handling.
This test Chrome and Safari have same behavior.

(In reply to Olli Pettay [:smaug] from comment #92)
> Comment on attachment 8867620 [details] [diff] [review]
> Bug 556743 - Update web-platform tests for labels attribute. v5
> 
> >+  test(function () {
> >+    var labels = document.getElementById("test11").labels;
> >+    assert_true(labels instanceof NodeList,
> >+                "A form control's 'labels' property should be an instance of a NodeList.");
> >+    assert_equals(labels.length, 1,
> >+                  "The form control has an ancestor with no explicit associated label, and is the first labelable descendant.");
> >+    assert_array_equals(labels, [document.getElementById("lbl11")],
> >+                        "The labels for a form control should be returned in tree order.");
> >+
> >+    let iframe = document.getElementById("iframe");
> >+    iframe.onload = function () {
> >+      iframe.contentWindow.document.getElementById("div1").appendChild(document.getElementById("p1"));
> >+      assert_equals(labels.length, 2,
> >+                    "The number of labels should be 2 after the labelable element is moved to iframe.");
> >+    };
> This is racy test. The iframe is possibly already loaded here.
> 
I'll use createElement('iframe') to avoid racing test. Thanks to point this out.

> 
> >+  test(function () {
> >+    // <label><input id="test13"></label><label for="test13"></label>
> >+    var frag = document.createDocumentFragment();
> >+    var label = document.createElement('label');
> >+    var input = document.createElement('input');
> >+    input.setAttribute('id', 'test13');
> >+    label.appendChild(input);
> >+    var label2 = document.createElement('label');
> >+    label2.setAttribute('for', 'test13');
> >+    frag.appendChild(label);
> >+    frag.appendChild(label2);
> >+
> >+    var labels = frag.getElementById("test13").labels;
> >+    assert_equals(labels.length, 0,
> >+                  "The number of labels should be 0 since a label element not in the document.");
> Why does that matter? Am I missing something in the spec? Where does it
> limit .labels handling to in-document cases?
I answered it in above comment.

Comment 94

[Boris Zbarsky [:bzbarsky]]

nsLabelsNodeList::MarkDirtyToFirstLabelableChildren as called from ContentAppended is buggy: it won't walk non-first appended content.

This is why it could use two arguments: one for the place to start walking and one for the root of the walk.  ContentInserted/Removed would pass the same value for both, but ContentAppended would not.

Comment 95

[Olli Pettay [:smaug][[e-mail]]]

(In reply to John Dai[:jdai] from comment #93)
> The reason I add IsInComposedDoc() check is because I want to keep the same
> the behavior as old version, which is make sure this element should
> in-document. It's because I use SubtreeRoot() to get root node, it can't
> tell me this element is in-document or not. Also, if I remove this check, it
> can't pass wpt[1].

But does anything in the spec require that behavior?

> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/testing/web-platform/tests/html/
> semantics/forms/the-label-element/label-attributes.html#49-57
That test relies on IDs. And even then "If the attribute is specified and there is an element in the tree whose ID is equal to the value of the for attribute". Nothing hints about in-document requirement.


What if there is just a disconnected subtree
<label><input></label>. Shouldn't input element's .labels contain one entry per spec?

Comment 96

[Olli Pettay [:smaug][[e-mail]]]

I filed https://github.com/whatwg/html/issues/2686
I think we need some spec clarifications.

Comment 97

[Olli Pettay [:smaug][[e-mail]]]

Also, https://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/testing/web-platform/tests/html/semantics/forms/the-label-element/label-attributes.html#49-57 doesn't test elements in same tree, but something else.

Comment 98

[Olli Pettay [:smaug][[e-mail]]]

John, do you think you could test Chrome (and other browsers) a bit and report findings to
https://github.com/whatwg/html/issues/2686 ?
Sounds like we may need spec changes.

Comment 99

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #95)
> (In reply to John Dai[:jdai] from comment #93)
> > The reason I add IsInComposedDoc() check is because I want to keep the same
> > the behavior as old version, which is make sure this element should
> > in-document. It's because I use SubtreeRoot() to get root node, it can't
> > tell me this element is in-document or not. Also, if I remove this check, it
> > can't pass wpt[1].
> 
> But does anything in the spec require that behavior?
> 
Here is a same discussion when the previous version code was added:
https://bugzilla.mozilla.org/show_bug.cgi?id=562932#c19

(In reply to Olli Pettay [:smaug] from comment #98)
> John, do you think you could test Chrome (and other browsers) a bit and
> report findings to
> https://github.com/whatwg/html/issues/2686 ?
> Sounds like we may need spec changes.
Sure. I reported on the spec issue.

Comment 100

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v6

- Per spec issue discussion[1], I revised our implementation to align HTML spec.

- Rename MarkDirtyToFirstLabelableChildren to ClearListAndMarkDirty and change this function behavior. If we modified nodes(remove/insert/appended) which contains a labelable element or a label element which has for attribute to indicate a form control, we should set dirty to its content list. We need two arguments for this function:  
 - aContainer which is aChild's parent that had children been modified. The reason we need aContainer is because we need to consider aChild's siblings became first or non-first labelable descendant. 
 - aChild which is the root of modified children.

- Revise nsLabelsNodeList mutation callback to not call nsContentList mutation callback. Since we'll mark dirty to labelable element when the node has been modified, there is no need to call nsContentList's mutation callback.

- Revise wpt to align HTML spec.

Hi Olli,
Could you help review this patch when you have time? Thank you.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8791b151d2d7a1f3a474ddc2c26ef8f35a4de662&filter-tier=1&group_state=expanded

[1]https://github.com/whatwg/html/issues/2686

Comment 101

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v6

Comment 102

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8869358 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v6

>   test(function () {
>     var label = document.createElement("label");
>     label.htmlFor = "test1";
>-    assert_not_equals(label.control, document.getElementById("test1"),
>-                      "A label element not in a document should not label an element in a document.");
>+    assert_equals(label.control, document.getElementById("test1"),
>+                  "A label element should label an element in the same document.");
This is wrong. label and test1 are not in the same tree. So if this test passes, the patch has some bug.

Comment 103

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8869357 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v6

Based on the test, this has some bug:

@@ -265,25 +266,26 @@ HTMLLabelElement::GetLabeledElement() const
   if (!GetAttr(kNameSpaceID_None, nsGkAtoms::_for, elementId)) {
     // 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 GetFirstLabelableDescendant();
   }
 
   // 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.
-  //XXXsmaug It is unclear how this should work in case the element is in
-  //         Shadow DOM.
-  //         See https://www.w3.org/Bugs/Public/show_bug.cgi?id=26365.
-  nsIDocument* doc = GetUncomposedDoc();
-  if (!doc) {
-    return nullptr;
+  nsINode* root = SubtreeRoot();
+  ShadowRoot* shadow = ShadowRoot::FromNode(root);
+  Element* element = nullptr;
+
+  if (shadow) {
+    element = shadow->GetElementById(elementId);
+  } else {
+    element = root->OwnerDoc()->GetElementById(elementId);
   }
Why do we use ownerDoc there. That is not what the spec says. It talks about elements being in the same tree. Sure, if both are in uncomposed doc, then using GetElementById is the faster way to find the element.

Comment 104

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v7

- Revise HTMLLabelElement::GetLabeledElement() logic to address comment #103. If we have for attribute to indicate a form control, and both are in uncomposed doc, then using GetElementById to find the labelable element for the faster way. If both aren't in uncomposed doc, then finding a labelable element which id is the same as for attribute in the same tree.

- Revise nsLabelsNodeList::ClearListAndMarkDirty which use Match() instead of MatchSelf(), because MatchSelf() has duplicate IsElement() check.

- Revise tests to address comment #103 and reflect first changed, it will be in the latter patch. 

Hi Olli,
Could you help review this patch when you have time? Thank you.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec1fd80f8cc359380cd7b3cd8ebc49c957b2c618&filter-tier=1&group_state=expanded

Comment 105

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v7

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8869942 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v7

>+  test(function () {
>+    var labels = document.getElementById("test11").labels;
>+    assert_true(labels instanceof NodeList,
>+                "A form control's 'labels' property should be an instance of a NodeList.");
>+    assert_equals(labels.length, 1,
>+                  "The form control has an ancestor with no explicit associated label, and is the first labelable descendant.");
>+    assert_array_equals(labels, [document.getElementById("lbl11")],
>+                        "The labels for a form control should be returned in tree order.");
>+
>+    let iframe = document.createElement('iframe');
>+    iframe.setAttribute('src', 'http://web-platform.test:8000/html/semantics/forms/the-label-element/iframe-label-attributes.html');
>+    iframe.onload = function () {
>+      iframe.contentWindow.document.getElementById("div1").appendChild(document.getElementById("p1"));
>+      assert_equals(labels.length, 2,
>+                    "The number of labels should be 2 after the labelable element is moved to iframe.");
>+    };
>+  }, "A labelable element is moved to iframe.");
Why is loading iframe in a sync test ok? Don't you need to use async_test?
Or perhaps you could just create a new HTML document using document.implementation.createHTMLDocument();
I would expect var test = async_test(...); test.done(); somewhere here.

>+  test(function () {
>+    // <label><input id="test14"></label><label for="test14"></label>
>+    var frag = document.createDocumentFragment();
Why we need the fragment?

>+    var label = document.createElement('label');
>+    var input = document.createElement('input');
>+    input.setAttribute('id', 'test14');
>+    label.appendChild(input);
>+    var label2 = document.createElement('label');
>+    label2.htmlFor = "test14";
>+    frag.appendChild(label);
>+    frag.appendChild(label2);
>+
>+    var labels = frag.getElementById("test14").labels;
>+    assert_equals(labels.length, 2,
>+                  "The number of labels associated with a form control should be the number of label elements for which it is a labeled control.");
>+    assert_true(labels instanceof NodeList,
>+                "A form control's 'labels' property should be an instance of a NodeList.");
>+    assert_equals(label.control, frag.getElementById("test14"), "The first labelable descendant of a label element should be its labeled control.");
>+    assert_equals(label2.control, frag.getElementById("test14"), "The labeled cotrol should be associated with the control whose ID is equal to the value of the 'for' attribute.");
>+  }, "A labelable element in the document fragment.");

Comment 107

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8869941 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v7

>+nsLabelsNodeList::ContentAppended(nsIDocument* aDocument,
>+                                  nsIContent* aContainer,
>+                                  nsIContent* aFirstNewContent,
>+                                  int32_t aNewIndexInContainer)
>+{
>+  // If a labelable element is moved to outside or inside of
>+  // nested associated labels, we're gonna have to modify
>+  // the content list.
>+  if (mState == LIST_DIRTY ||
>+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aContainer) ||
>+      ClearListAndMarkDirty(aContainer, aFirstNewContent)) {
>+    return;
>+  }
>+}
>+
>+void
>+nsLabelsNodeList::ContentInserted(nsIDocument* aDocument,
>+                                  nsIContent* aContainer,
>+                                  nsIContent* aChild,
>+                                  int32_t aIndexInContainer)
>+{
>+  // If a labelable element is moved to outside or inside of
>+  // nested associated labels, we're gonna have to modify
>+  // the content list.
>+  if (mState == LIST_DIRTY ||
>+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aChild) ||
>+      ClearListAndMarkDirty(aContainer, aChild)) {
>+    return;
>+  }
>+}
....
>+bool
>+nsLabelsNodeList::ClearListAndMarkDirty(nsIContent* aContainer,
>+                                        nsIContent* aChild)
>+{
>+  MOZ_ASSERT(aContainer, "Can't get the content if no container!");
>+  // We need to consider aChild's children have this labelable element or
>+  // a label element which has for attribute to indicate a form control.
>+  for (nsIContent* cur = aChild; cur; cur = cur->GetNextNode(aChild)) {
>+    if (cur->IsElement() &&
>+        (cur->AsElement() == mData || Match(cur->AsElement()))) {
>+      SetDirty();
>+      return true;
>+    }
>+  }
>+
>+  // We need to consider aChild's siblings became first or non-first
>+  // labelable descendant.
>+  for (nsIContent* cur = aContainer; cur; cur = cur->GetNextNode(aContainer)) {
>+    if (cur->IsElement() && cur->AsElement() == mData) {
>+      SetDirty();
>+      return true;
>+    }
>+  }
So in insertion/appending case don't we end up going through the same nodes two times here if mData points to some ancestor and we aren't
adding any nodes under that but to under some of its descendants.
In general I need to still understand what ClearListAndMarkDirty does, but looks like it is doing too much at least in some cases.
Could you explain it a bit.

>+  if (shadow) {
>+    element = shadow->GetElementById(elementId);
>+  } else {
>+    nsIDocument* doc = GetUncomposedDoc();
>+    if (doc) {
>+      element = doc->GetElementById(elementId);
>+    } else {
>+      for (nsIContent* cur = root->AsContent(); cur;
>+           cur = cur->GetNextNode(root)) {
>+        if (!cur->IsElement()) {
>+          continue;
>+        }
>+
>+        nsIAtom* id = cur->AsElement()->GetID();
>+        if (id && id->Equals(elementId)) {
Could you perhaps atomize elementId before that. then it would be just id == atomizedElementId comparison which is a lot faster than
string comparison.

Comment 108

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #106)
> Comment on attachment 8869942 [details] [diff] [review]
> Bug 556743 - Update web-platform tests for labels attribute. v7
> >+  test(function () {
> >+    // <label><input id="test14"></label><label for="test14"></label>
> >+    var frag = document.createDocumentFragment();
> Why we need the fragment?
> 
Thanks for your review. It's because I want to test if elements aren't in tree, we still can get correct length and control. Per spec issue discussion[1], it seems that Chrome and Safari didn't do correctly in this portion.

[1]https://github.com/whatwg/html/issues/2686

(In reply to Olli Pettay [:smaug] from comment #107)
> Comment on attachment 8869941 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v7
> >+bool
> >+nsLabelsNodeList::ClearListAndMarkDirty(nsIContent* aContainer,
> >+                                        nsIContent* aChild)
> >+{
> >+  MOZ_ASSERT(aContainer, "Can't get the content if no container!");
> >+  // We need to consider aChild's children have this labelable element or
> >+  // a label element which has for attribute to indicate a form control.
> >+  for (nsIContent* cur = aChild; cur; cur = cur->GetNextNode(aChild)) {
> >+    if (cur->IsElement() &&
> >+        (cur->AsElement() == mData || Match(cur->AsElement()))) {
> >+      SetDirty();
> >+      return true;
> >+    }
> >+  }
> >+
> >+  // We need to consider aChild's siblings became first or non-first
> >+  // labelable descendant.
> >+  for (nsIContent* cur = aContainer; cur; cur = cur->GetNextNode(aContainer)) {
> >+    if (cur->IsElement() && cur->AsElement() == mData) {
> >+      SetDirty();
> >+      return true;
> >+    }
> >+  }
> So in insertion/appending case don't we end up going through the same nodes
> two times here if mData points to some ancestor and we aren't
> adding any nodes under that but to under some of its descendants.
> In general I need to still understand what ClearListAndMarkDirty does, but
> looks like it is doing too much at least in some cases.
> Could you explain it a bit.
> 
I would like to explain more clearly. Your concern that we end up going through the same nodes two times in ClearListAndMarkDirty is right. It's because we registered root node on nsLabelsNodeList, it means a mutation observer will be notified when this node, or any of its descendants, are modified. If we want to fix doing too much ClearListAndMarkDirty in some cases. We can set root node to this labelable element's parent and we still need to keep this tree's root node in order to get correct length and list items via nsContentList::PopulateSelf.

Comment 109

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #108)
> (In reply to Olli Pettay [:smaug] from comment #106)
> > Comment on attachment 8869942 [details] [diff] [review]
> > Bug 556743 - Update web-platform tests for labels attribute. v7
> > >+  test(function () {
> > >+    // <label><input id="test14"></label><label for="test14"></label>
> > >+    var frag = document.createDocumentFragment();
> > Why we need the fragment?
> > 
> Thanks for your review. It's because I want to test if elements aren't in
> tree, we still can get correct length and control. Per spec issue
> discussion[1], it seems that Chrome and Safari didn't do correctly in this
> portion.
> 
> [1]https://github.com/whatwg/html/issues/2686
> 
We don't need the fragment if we want to test elements aren't in tree. I'll address them in next patch.

Comment 110

[John Dai[:jdai]]

> (In reply to Olli Pettay [:smaug] from comment #107)
> > Comment on attachment 8869941 [details] [diff] [review]
> > Bug 556743 - Implement the labels attribute. v7
> > >+bool
> > >+nsLabelsNodeList::ClearListAndMarkDirty(nsIContent* aContainer,
> > >+                                        nsIContent* aChild)
> > >+{
> > >+  MOZ_ASSERT(aContainer, "Can't get the content if no container!");
> > >+  // We need to consider aChild's children have this labelable element or
> > >+  // a label element which has for attribute to indicate a form control.
> > >+  for (nsIContent* cur = aChild; cur; cur = cur->GetNextNode(aChild)) {
> > >+    if (cur->IsElement() &&
> > >+        (cur->AsElement() == mData || Match(cur->AsElement()))) {
> > >+      SetDirty();
> > >+      return true;
> > >+    }
> > >+  }
> > >+
> > >+  // We need to consider aChild's siblings became first or non-first
> > >+  // labelable descendant.
> > >+  for (nsIContent* cur = aContainer; cur; cur = cur->GetNextNode(aContainer)) {
> > >+    if (cur->IsElement() && cur->AsElement() == mData) {
> > >+      SetDirty();
> > >+      return true;
> > >+    }
> > >+  }
> > So in insertion/appending case don't we end up going through the same nodes
> > two times here if mData points to some ancestor and we aren't
> > adding any nodes under that but to under some of its descendants.
> > In general I need to still understand what ClearListAndMarkDirty does, but
> > looks like it is doing too much at least in some cases.
> > Could you explain it a bit.
> > 
> I would like to explain more clearly. Your concern that we end up going
> through the same nodes two times in ClearListAndMarkDirty is right. It's
> because we registered root node on nsLabelsNodeList, it means a mutation
> observer will be notified when this node, or any of its descendants, are
> modified. If we want to fix doing too much ClearListAndMarkDirty in some
> cases. We can set root node to this labelable element's parent and we still
> need to keep this tree's root node in order to get correct length and list
> items via nsContentList::PopulateSelf.
Typo of this line "We can set root node to this labelable element's parent...". s/this labelable element's parent/this labelable element/. Also, we need to tweak ClearListAndMarkDirty in order to skip going through the same nodes two times in insertion/appending case.

Comment 111

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v8

- Revise ClearListAndMarkDirty to skip going through the same nodes two times in insertion/appending case. If there doesn't have for attribute to indicate this form control and it's an ancestor of aChild, it means this labelable element(a.k.a mData) can't be aChild's following siblings, we can do early-return.

- Override PopulateSelf to start searching at the root node. We need to override this function is because nsContentList::PopulateSelf start searching at the root node's child. It'll miss count root node if root node is a label element and it has a first labelable descendant.

- Revise tests to address comment #106 and add a test which test a labelable element is moved to inside of nested associated labels, it will be in the latter patch. 

Note: The reason we need ClearListAndMarkDirty is because we registered tree's root node on nsLabelsNodeList, it means a mutation observer will be notified when this root node, or any of its descendants, are modified. If there is any modified nodes under root node, all the labelable elements which had been registered mutation observer will receive a mutation notification, so we need to clear list and mark dirty to those modified labelable elements. I have to take back my reply at comment #108, we can't set root node at this labelable element, it's because it'll miss mutation notification if we modified an ancestor of this labelable element and it didn't register mutation observer.


Hi Olli,
Could you help review this patch when you have time? Thank you.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=813240e00692a2ee866d4c1f0c9bfd7b8502ddbd&filter-tier=1&group_state=expanded

Comment 112

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v8

Comment 113

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8871609 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v8

>+nsLabelsNodeList::ContentAppended(nsIDocument* aDocument,
>+                                  nsIContent* aContainer,
>+                                  nsIContent* aFirstNewContent,
>+                                  int32_t aNewIndexInContainer)
>+{
>+  // If a labelable element is moved to outside or inside of
>+  // nested associated labels, we're gonna have to modify
>+  // the content list.
>+  if (mState == LIST_DIRTY ||
>+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aContainer) ||
>+      ClearListAndMarkDirty(aContainer, aFirstNewContent)) {
>+    return;
>+  }
So the return value of ClearListAndMarkDirty isn't actually used anywhere.
Perhaps it could be void method and this code
if (mState != LIST_DIRTY &&
    nsContentUtils::IsInSameAnonymousTree(mRootNode, aContainer) {
  ClearListAndMarkDirty(aContainer, aFirstNewContent);
}
Same also elsewhere.

>+nsLabelsNodeList::ClearListAndMarkDirty(nsIContent* aContainer,
>+                                        nsIContent* aChild)
So this doesn't necessarily do anything. Perhaps call this MaybeClearListAndMarkDirty

>+{
>+  MOZ_ASSERT(aContainer, "Can't get the content if no container!");
This doesn't look right. If nsIMutationObserver is registered to document and one appends processing instruction for example, aContainer will be null
(but aDocument non-null).
You want to pass nsINode* aContainer here and then in the caller use NODE_FROM


>+  // If this labelable element isn't a aChild's following sibling,
>+  // we can do early-return.
>+  if (nsContentUtils::PositionIsBefore(static_cast<nsINode*>(mData), aChild)) {
>+    return false;
>+  }
I don't understand what this is trying to capture

>+
>+  // We need to consider aChild's following siblings became first or non-first
>+  // labelable descendant.
>+  for (nsIContent* cur = aContainer; cur; cur = cur->GetNextNode(aContainer)) {
>+    if (cur == aChild) {
>+      cur = cur->GetNextSibling();
>+      if (!cur) {
>+        return false;
>+      }
>+    }
>+
>+    if (cur->IsElement() && cur->AsElement() == mData) {
>+      SetDirty();
>+      return true;
>+    }
>+  }
So we still possibly iterate through the whole subtree twice. If mData is somewhere at the end of the document and there is some
dom mutation happening at the beginning of the document without any changes to labelable elements or label element.



>+  nsINode* root = SubtreeRoot();
>+  ShadowRoot* shadow = ShadowRoot::FromNode(root);
>+  Element* element = nullptr;
>+
>+  if (shadow) {
>+    element = shadow->GetElementById(elementId);
>+  } else {
>+    nsIDocument* doc = GetUncomposedDoc();
>+    if (doc) {
>+      element = doc->GetElementById(elementId);
>+    } else {
>+      element = nsContentUtils::MatchElementId(root->AsContent(), elementId);
>+    }
>   }
Looks like we have a helper for this. I had forgotten it:
nsINode::GetElementById. It is protected, but if needed, make it public.
The following should be then enough
nsINode* root = SubtreeRoot();
Element* element = root->GetElementById(elementId);



nsINode* root = Subtree

Comment 114

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #113)
> Comment on attachment 8871609 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v8
> 
> >+nsLabelsNodeList::ContentAppended(nsIDocument* aDocument,
> >+                                  nsIContent* aContainer,
> >+                                  nsIContent* aFirstNewContent,
> >+                                  int32_t aNewIndexInContainer)
> >+{
> >+  // If a labelable element is moved to outside or inside of
> >+  // nested associated labels, we're gonna have to modify
> >+  // the content list.
> >+  if (mState == LIST_DIRTY ||
> >+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aContainer) ||
> >+      ClearListAndMarkDirty(aContainer, aFirstNewContent)) {
> >+    return;
> >+  }
> So the return value of ClearListAndMarkDirty isn't actually used anywhere.
> Perhaps it could be void method and this code
> if (mState != LIST_DIRTY &&
>     nsContentUtils::IsInSameAnonymousTree(mRootNode, aContainer) {
>   ClearListAndMarkDirty(aContainer, aFirstNewContent);
> }
> Same also elsewhere.
> 
Will do.
> >+nsLabelsNodeList::ClearListAndMarkDirty(nsIContent* aContainer,
> >+                                        nsIContent* aChild)
> So this doesn't necessarily do anything. Perhaps call this
> MaybeClearListAndMarkDirty
> 
Will do.
> >+{
> >+  MOZ_ASSERT(aContainer, "Can't get the content if no container!");
> This doesn't look right. If nsIMutationObserver is registered to document
> and one appends processing instruction for example, aContainer will be null
> (but aDocument non-null).
> You want to pass nsINode* aContainer here and then in the caller use
> NODE_FROM
> 
Will do.
> 
> >+  // If this labelable element isn't a aChild's following sibling,
> >+  // we can do early-return.
> >+  if (nsContentUtils::PositionIsBefore(static_cast<nsINode*>(mData), aChild)) {
> >+    return false;
> >+  }
> I don't understand what this is trying to capture
> 
The main purpose is to skip labelable element which isn't a aChild sibling before looping aContainer's subtree. The reason is we only care aChild's following sibling which will become first descendant for remove case or non-first descendant for insert case. For those nodes which won't be aChild's following sibling, we don't need to handle them. After doing performance test, it doesn't really help for performance. I'll change another solution in next patch.

> >+
> >+  // We need to consider aChild's following siblings became first or non-first
> >+  // labelable descendant.
> >+  for (nsIContent* cur = aContainer; cur; cur = cur->GetNextNode(aContainer)) {
> >+    if (cur == aChild) {
> >+      cur = cur->GetNextSibling();
> >+      if (!cur) {
> >+        return false;
> >+      }
> >+    }
> >+
> >+    if (cur->IsElement() && cur->AsElement() == mData) {
> >+      SetDirty();
> >+      return true;
> >+    }
> >+  }
> So we still possibly iterate through the whole subtree twice. If mData is
> somewhere at the end of the document and there is some
> dom mutation happening at the beginning of the document without any changes
> to labelable elements or label element.
> 
The two loops intention are different. The first loop is just for going through aChild's subtree to see anything interesting.
The second loop is for going through aContainer's subtree to see aChild's siblings, because it'll become first or non-first labelable descendant.

> 
> 
> >+  nsINode* root = SubtreeRoot();
> >+  ShadowRoot* shadow = ShadowRoot::FromNode(root);
> >+  Element* element = nullptr;
> >+
> >+  if (shadow) {
> >+    element = shadow->GetElementById(elementId);
> >+  } else {
> >+    nsIDocument* doc = GetUncomposedDoc();
> >+    if (doc) {
> >+      element = doc->GetElementById(elementId);
> >+    } else {
> >+      element = nsContentUtils::MatchElementId(root->AsContent(), elementId);
> >+    }
> >   }
> Looks like we have a helper for this. I had forgotten it:
> nsINode::GetElementById. It is protected, but if needed, make it public.
> The following should be then enough
> nsINode* root = SubtreeRoot();
> Element* element = root->GetElementById(elementId);
> 
Will do.

Comment 115

[Jan de Mooij [:jandem]]

Just curious, are the patches already taking advantage of bug 1237504 being fixed? See bug 1237504 comment 1.

Comment 116

[Olli Pettay [:smaug][[e-mail]]]

it is not, but such thing could be done in a followup bug

Comment 117

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v9

- Change nsContentUtils::PositionIsBefore to nsContentUtils::ContentIsDescendantOf for MaybeClearListAndMarkDirty, the time complexity of nsContentUtils::ContentIsDescendantOf is O(logn). So that we can bailed-out if a labelable element isn't a aContainer's descendant. The reason we can't start the latter loop through aPreviousSibling->GetNextSibling() is because the next silbing can be a text node, which isn't aChild's labelable siblings.

- Add aContentAppended to bailed-out latter loop if aContentAppended is true.

- Address comment #113.

Note: I prefer keep fast way to get id, and not use |Element* element = root->GetElementById(elementId);| for all of nodes. Does it sound good to you?

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84ce459ce9aab236b6e5c98f934e356ad5420077&filter-tier=1&group_state=expanded
PGO build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34b1e6a95247483e75af86f25d60b8737f5d1629&selectedJob=104423534

Here is performance tests for innerHTML with and without .labels result:

PGO build with labels:
Running 10 times Ignoring warm-up run (116) 318 531 723 982 1179 1415 1624 1831 2052 2313 avg 1296.8 stdev 632.1480522788946 
Running 10 times Ignoring warm-up run (110) 311 511 708 962 1156 1384 1552 1768 1992 2209 avg 1255.3 stdev 605.0482708015948 
Running 10 times Ignoring warm-up run (111) 310 521 738 981 1156 1370 1575 1784 1989 2209 avg 1263.3 stdev 602.2952847233655 
Running 10 times Ignoring warm-up run (111) 312 510 714 1010 1159 1361 1565 1768 1989 2230 avg 1261.8 stdev 605.4436059617774 
Running 10 times Ignoring warm-up run (110) 312 521 716 960 1158 1365 1576 1809 1992 2204 avg 1261.3 stdev 606.6621877124039 
Running 10 times Ignoring warm-up run (110) 313 513 715 985 1166 1383 1584 1794 2020 2254 avg 1272.7 stdev 616.8944885472717 
Running 10 times Ignoring warm-up run (111) 311 512 740 981 1180 1426 1580 1781 2049 2306 avg 1286.6 stdev 626.5544190251953 
Running 10 times Ignoring warm-up run (111) 309 585 705 958 1222 1362 1559 1776 1987 2200 avg 1266.3 stdev 594.852258968561 
Running 10 times Ignoring warm-up run (111) 314 524 709 963 1159 1364 1564 1778 1996 2206 avg 1257.7 stdev 603.8822815748115 
Running 10 times Ignoring warm-up run (111) 311 529 729 975 1167 1370 1572 1786 2002 2239 avg 1268 stdev 608.3816236541008 

without labels:
Running 10 times Ignoring warm-up run (16) 14 14 13 13 13 14 13 13 13 13 avg 13.3 stdev 0.45825756949558405 
Running 10 times Ignoring warm-up run (15) 14 14 14 13 14 12 14 14 14 14 avg 13.7 stdev 0.6403124237432849 
Running 10 times Ignoring warm-up run (14) 14 14 13 13 13 13 13 13 13 13 avg 13.2 stdev 0.40000000000000013 
Running 10 times Ignoring warm-up run (15) 15 14 13 13 14 14 13 13 13 13 avg 13.5 stdev 0.6708203932499369 
Running 10 times Ignoring warm-up run (15) 15 16 13 12 14 14 14 14 13 13 avg 13.8 stdev 1.0770329614269007 
Running 10 times Ignoring warm-up run (15) 15 15 14 13 14 14 13 13 13 13 avg 13.7 stdev 0.7810249675906655 
Running 10 times Ignoring warm-up run (15) 15 14 13 12 14 13 13 13 12 13 avg 13.2 stdev 0.8717797887081348 
Running 10 times Ignoring warm-up run (15) 15 15 14 14 13 13 13 13 13 13 avg 13.6 stdev 0.7999999999999999 
Running 10 times Ignoring warm-up run (14) 15 15 13 13 14 13 14 13 13 13 avg 13.6 stdev 0.7999999999999999 
Running 10 times Ignoring warm-up run (17) 14 14 13 12 15 14 13 13 13 13 avg 13.4 stdev 0.7999999999999999 

Sadly, with the patches, it's huge slower... :( So, I took profiles on some cases.

With this patch:
Running 10 times Ignoring warm-up run (162) 440 731 1040 1358 1541 1785 2206 2449 2761 3034 avg 1734.5 stdev 825.956566654688 
https://perfht.ml/2qXpmAd

It took 1146ms at MaybeClearListAndMarkDirty(). If we only do SetDirty() and return which means no any for loops at the MaybeClearListAndMarkDirty(), it shows at the below: 

With turning patch:
Running 10 times Ignoring warm-up run (62) 131 186 257 324 360 424 493 521 591 664 avg 395.1 stdev 166.06772714769116 
https://perfht.ml/2qXr9oL

Is this a possible solution? Maybe you can notice something else.

Comment 118

[Olli Pettay [:smaug][[e-mail]]]

could you upload the test too?

Comment 119

[John Dai[:jdai]]

Attachment: Bug 556743 - Update web-platform tests for labels attribute. v9

Adjust id number in the test and carry r+.

Comment 120

[John Dai[:jdai]]

Attachment: innerhtmltest3.html

Comment 121

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #120)
> Created attachment 8874424 [details]
> innerhtmltest3.html

I modified the test from bug 934607. Thanks Olli for the test!

Comment 122

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v9-1 (faster version)

- Remove MaybeClearListAndMarkDirty(). It's because MaybeClearListAndMarkDirty() is the critical patch of .labels performance.

The patch is base on .labels is rarely used, but we don't want to slow down rest of the DOM too much.

Here is performance tests for innerHTML with and without .labels result:
Note: This test with node change which has mutation notification, so it's not the same as comment #117 test.

PGO build for old patch with .labels:
Running 10 times Ignoring warm-up run (110) 310 517 719 995 1194 1377 1628 1915 2010 2219 avg 1288.4 stdev 622.4198261623742 
Running 10 times Ignoring warm-up run (118) 334 535 730 979 1228 1439 1593 1810 2043 2287 avg 1297.8 stdev 619.9149619100994 
Running 10 times Ignoring warm-up run (113) 320 535 731 969 1228 1391 1607 1841 2071 2326 avg 1301.9 stdev 634.541007343103 
Running 10 times Ignoring warm-up run (111) 310 508 705 960 1159 1360 1562 1776 2001 2282 avg 1262.3 stdev 619.4515396703764 
Running 10 times Ignoring warm-up run (110) 309 510 716 963 1163 1370 1564 1766 1991 2232 avg 1258.4 stdev 608.2866429570848 

5 times average: 1281.76

PGO build for faster patch with .labels:
Running 10 times Ignoring warm-up run (35) 81 132 178 287 341 408 465 500 550 601 avg 354.3 stdev 171.87905631577104 
Running 10 times Ignoring warm-up run (35) 85 136 179 278 332 391 435 494 551 599 avg 348 stdev 168.1588534689744 
Running 10 times Ignoring warm-up run (34) 82 131 175 279 328 387 437 491 547 614 avg 347.1 stdev 171.22350889991714 
Running 10 times Ignoring warm-up run (33) 80 128 172 280 332 392 440 489 542 594 avg 344.9 stdev 168.64664242136575 
Running 10 times Ignoring warm-up run (35) 83 133 178 281 344 396 434 489 561 607 avg 350.6 stdev 170.82692996129154 

5 times average: 348.98

PGO build without labels:
Running 10 times Ignoring warm-up run (7) 7 7 6 57 55 66 55 56 62 56 avg 42.7 stdev 23.816170976880397 
Running 10 times Ignoring warm-up run (7) 8 8 7 59 64 57 57 57 57 60 avg 43.4 stdev 23.482759633399137 
Running 10 times Ignoring warm-up run (7) 7 8 6 59 56 66 59 57 66 61 avg 44.5 stdev 24.751767613647313 
Running 10 times Ignoring warm-up run (7) 7 7 6 59 58 71 58 59 68 60 avg 45.3 stdev 25.628304664959796 
Running 10 times Ignoring warm-up run (8) 8 7 7 61 58 68 59 58 67 59 avg 45.2 stdev 25.011197492323316 

5 times average: 44.22

Chrome with .labels:
Running 10 times Ignoring warm-up run (532) 361 398 442 587 665 757 743 955 1066 1159 avg 713.3 stdev 264.5135346253571
Running 10 times Ignoring warm-up run (77) 188 307 402 563 637 754 751 1022 1094 1186 avg 690.4 stdev 320.6004366809253
Running 10 times Ignoring warm-up run (72) 165 286 364 513 608 702 809 936 1065 1155 avg 660.3 stdev 317.1781991247192
Running 10 times Ignoring warm-up run (96) 205 311 391 660 729 905 1197 1231 1219 1551 avg 839.9 stdev 431.6369886837781
Running 10 times Ignoring warm-up run (117) 227 320 454 568 744 756 940 1026 1167 1268 avg 747 stdev 336.6957083183568

5 times average: 730.18

Chrome without .labels:
Running 10 times Ignoring warm-up run (22) 18 16 17 16 16 15 16 17 12 14 avg 15.7 stdev 1.6155494421403513
Running 10 times Ignoring warm-up run (26) 21 23 20 15 15 13 16 15 39 13 avg 19 stdev 7.416198487095663
Running 10 times Ignoring warm-up run (17) 19 18 17 16 15 15 15 21 25 18 avg 17.9 stdev 3.014962686336267
Running 10 times Ignoring warm-up run (22) 17 18 17 19 15 13 15 15 14 13 avg 15.6 stdev 1.9595917942265424
Running 10 times Ignoring warm-up run (21) 19 15 16 15 16 15 19 23 12 13 avg 16.3 stdev 3.0675723300355937

5 times average: 16.9

I'll upload test which called innerhtmltestforfastlabels.html in latter patch.

Comment 123

[John Dai[:jdai]]

Attachment: innerhtmltestforfastlabels.html

Comment 124

[John Dai[:jdai]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=092fe9e4df96eebe0d7820a9ef4ce1d4f5592b96&filter-tier=1&group_state=expanded
PGO build: https://queue.taskcluster.net/v1/task/CBGrjdkfQqe3dTBgh-72oA/runs/0/artifacts/public/build/target.tar.bz2

Comment 125

[Olli Pettay [:smaug][[e-mail]]]

I don't understand 
Running 10 times Ignoring warm-up run (7) 7 7 6 57 55 66 55 56 62 56 avg 42.7 stdev 23.816170976880397 
What kind of test is that. Why does it get slower?

Comment 126

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875609 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v9-1 (faster version)

>+  if (mDeep) {
>+    // If we already have nodes start searching at the last one, otherwise
>+    // start searching at the root.
>+    nsINode* cur = count ? mElements[count - 1] : mRootNode;
>+
>+    if (!cur) {
>+      return;
>+    }
>+
>+    if (cur->IsElement() && Match(cur->AsElement())) {
>+      mElements.AppendElement(cur->AsElement());
>+    }
>+  }
I don't understand this.
cur points to the last object in mElements and then it is re-appended
to mElements

Comment 127

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #125)
> I don't understand 
> Running 10 times Ignoring warm-up run (7) 7 7 6 57 55 66 55 56 62 56 avg
> 42.7 stdev 23.816170976880397 
> What kind of test is that. 
Thanks for your review. The test is innerhtmltestforfastlabels.html. I commented out len1, len2, and len3 in innerhtmltestforfastlabels.html

> Why does it get slower?
Per offline discussion, nsContentUtils::IsInSameAnonymousTree takes quite a bit time. I'll address them in next patch. 
Profile: https://perfht.ml/2rIt8RF

(In reply to Olli Pettay [:smaug] from comment #126)
> Comment on attachment 8875609 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v9-1 (faster version)
> 
> >+  if (mDeep) {
> >+    // If we already have nodes start searching at the last one, otherwise
> >+    // start searching at the root.
> >+    nsINode* cur = count ? mElements[count - 1] : mRootNode;
> >+
> >+    if (!cur) {
> >+      return;
> >+    }
> >+
> >+    if (cur->IsElement() && Match(cur->AsElement())) {
> >+      mElements.AppendElement(cur->AsElement());
> >+    }
> >+  }
> I don't understand this.
> cur points to the last object in mElements and then it is re-appended
> to mElements

It seems nsLabelsNodeList::PopulateSelf no need to take last object in mElements and re-append to mElements, just check root node and call default implementation. Thank you for catching this.

Comment 128

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v10

- Revise nsLabelsNodeList::PopulateSelf.

I didn't find a good way to tune nsContentUtils::IsInSameAnonymousTree performance, because it'll hit test failures[1]. 

Here is opt build with revised nsLabelsNodeList::PopulateSelf: 
Running 10 times Ignoring warm-up run (39) 90 141 189 235 280 328 376 426 472 522 avg 305.9 stdev 136.85134270441048 
Running 10 times Ignoring warm-up run (39) 89 139 187 237 290 339 387 436 484 533 avg 312.1 stdev 141.96721452504448 
Running 10 times Ignoring warm-up run (39) 90 142 191 235 286 341 391 438 473 528 avg 311.5 stdev 139.62897263820284 
Running 10 times Ignoring warm-up run (38) 91 140 189 234 281 329 385 435 480 522 avg 308.6 stdev 139.12382973452102 
Running 10 times Ignoring warm-up run (38) 91 143 189 255 291 340 388 434 476 523 avg 313 stdev 137.5034545020597 

Average: 310.22

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aef51cd16686b94163042ded1281292350d8adb&filter-tier=1&group_state=expanded

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=00babe7530febc4f081c749d73bfa01f97e88818&filter-tier=1

Comment 129

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876657 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v10

>+nsLabelsNodeList::AttributeChanged(nsIDocument* aDocument, Element* aElement,
>+                                   int32_t aNameSpaceID, nsIAtom* aAttribute,
>+                                   int32_t aModType,
>+                                   const nsAttrValue* aOldValue)
>+{
>+  MOZ_ASSERT(aElement, "Must have a content node to work with");
>+  if (mState == LIST_DIRTY ||
>+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aElement)) {
>+    return;
>+  }
>+
>+  // We need to handle input type changes to or from "hidden".
>+  if (aElement->IsHTMLElement(nsGkAtoms::input) &&
>+      aAttribute == nsGkAtoms::type) {
>+    SetDirty();
>+    return;
>+  }
I'm a bit worried about the performance affect this has.
At least check first aAttribute == nsGkAtoms::type and then aElement->IsHTMLElement(nsGkAtoms::input)
And there should be also check then for default namespace too.

>+nsLabelsNodeList::PopulateSelf(uint32_t aNeededLength)
>+{
>+  MOZ_ASSERT(mRootNode, "Must have root");
>+
>+  // Start searching at the root.
>+  nsINode* cur = mRootNode;
>+  if (cur->IsElement() && Match(cur->AsElement())) {
>+    mElements.AppendElement(cur->AsElement());
>+  }
What if rootnode is already in mElements?
Don't you want to append to mElements only when mElements is empty

>+
>+  nsContentList::PopulateSelf(aNeededLength);
>+}
>\ No newline at end of file
add new line



>-  for (nsIContent* kid = GetFirstChild(); kid; kid = kid->GetNextNode(this)) {
>-    if (!kid->IsElement()) {
>-      continue;
>-    }
>-    nsIAtom* id = kid->AsElement()->GetID();
>-    if (id && id->Equals(aId)) {
>-      return kid->AsElement();
>-    }
>-  }
>-  return nullptr;
>+  return nsContentUtils::MatchElementId(this->AsContent(), aId);
Why this change? MatchElementId doesn't do the same thing, since it starts from 'this', and the old code here start from GetFirstChild.

Those small changes still

Comment 130

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #129)
> Comment on attachment 8876657 [details] [diff] [review]
> Bug 556743 - Implement the labels attribute. v10
> 
> >+nsLabelsNodeList::AttributeChanged(nsIDocument* aDocument, Element* aElement,
> >+                                   int32_t aNameSpaceID, nsIAtom* aAttribute,
> >+                                   int32_t aModType,
> >+                                   const nsAttrValue* aOldValue)
> >+{
> >+  MOZ_ASSERT(aElement, "Must have a content node to work with");
> >+  if (mState == LIST_DIRTY ||
> >+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aElement)) {
> >+    return;
> >+  }
> >+
> >+  // We need to handle input type changes to or from "hidden".
> >+  if (aElement->IsHTMLElement(nsGkAtoms::input) &&
> >+      aAttribute == nsGkAtoms::type) {
> >+    SetDirty();
> >+    return;
> >+  }
> I'm a bit worried about the performance affect this has.
Thanks for your review. After ran a test with setAttribute() 10000 times:

Firefox without labels:
Running 10 times Ignoring warm-up run (217) 220 226 219 214 224 217 217 212 215 218 avg 218.2 stdev 4.093897898091744 

Firefox with labels:
Running 10 times Ignoring warm-up run (213) 207 215 221 221 227 241 243 248 255 260 avg 233.8 stdev 17.08683703907777 

Chrome without labels:
Running 10 times Ignoring warm-up run (39) 29 28 28 28 28 29 28 28 27 27 avg 28 stdev 0.6324555320336759

Chrome with labels:
Running 10 times Ignoring warm-up run (42) 29 29 28 29 29 29 29 29 29 29 avg 28.9 stdev 0.29999999999999993

I saw there is a general bug 1372552 for all content list.

> At least check first aAttribute == nsGkAtoms::type and then
> aElement->IsHTMLElement(nsGkAtoms::input)
> And there should be also check then for default namespace too.
> 
Will do.
> >+nsLabelsNodeList::PopulateSelf(uint32_t aNeededLength)
> >+{
> >+  MOZ_ASSERT(mRootNode, "Must have root");
> >+
> >+  // Start searching at the root.
> >+  nsINode* cur = mRootNode;
> >+  if (cur->IsElement() && Match(cur->AsElement())) {
> >+    mElements.AppendElement(cur->AsElement());
> >+  }
> What if rootnode is already in mElements?
> Don't you want to append to mElements only when mElements is empty
> 
Will do.
> >+
> >+  nsContentList::PopulateSelf(aNeededLength);
> >+}
> >\ No newline at end of file
> add new line
> 
Will do.
> >-  for (nsIContent* kid = GetFirstChild(); kid; kid = kid->GetNextNode(this)) {
> >-    if (!kid->IsElement()) {
> >-      continue;
> >-    }
> >-    nsIAtom* id = kid->AsElement()->GetID();
> >-    if (id && id->Equals(aId)) {
> >-      return kid->AsElement();
> >-    }
> >-  }
> >-  return nullptr;
> >+  return nsContentUtils::MatchElementId(this->AsContent(), aId);
> Why this change? MatchElementId doesn't do the same thing, since it starts
> from 'this', and the old code here start from GetFirstChild.
> 
> Those small changes still
It's because HTMLLabelElement::GetLabeledElement need to consider root node. However nsINode::GetElementById start from GetFirstChild, it probably has side-effect at SVG code which use nsINode::GetElementById. I'll revert back to version 8's patch of this portion.

Comment 131

[John Dai[:jdai]]

Attachment: bug_556743_attribute_change.html

Comment 132

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v11

Address comment #130. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80950790a7497d6199adc80ec2210e31651b8702&filter-tier=1&group_state=expanded

Comment 133

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8877392 [details] [diff] [review]
Bug 556743 - Implement the labels attribute. v11

>+nsLabelsNodeList::AttributeChanged(nsIDocument* aDocument, Element* aElement,
>+                                   int32_t aNameSpaceID, nsIAtom* aAttribute,
>+                                   int32_t aModType,
>+                                   const nsAttrValue* aOldValue)
>+{
>+  MOZ_ASSERT(aElement, "Must have a content node to work with");
>+  if (mState == LIST_DIRTY ||
>+      !nsContentUtils::IsInSameAnonymousTree(mRootNode, aElement)) {
>+    return;
>+  }
>+
>+  // We need to handle input type changes to or from "hidden".
>+  if (aElement->IsHTMLElement(nsGkAtoms::input) &&
>+      aNameSpaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::type) {
Could you do aAttribute == nsGkAtoms::type check first, since that is a bit faster than
aElement->IsHTMLElement(nsGkAtoms::input)

Comment 134

[John Dai[:jdai]]

Attachment: Bug 556743 - Implement the labels attribute. v12

Thanks for Olli review. Address comment #133, and carry r+. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf2584af2197c68b28cab85c675613a3ede4683&filter-tier=1&group_state=expanded

Comment 135

[John Dai[:jdai]]

(In reply to Jan de Mooij [:jandem] from comment #115)
> Just curious, are the patches already taking advantage of bug 1237504 being
> fixed? See bug 1237504 comment 1.

File bug 1373527 for followup bug.

Comment 136

[Ryan VanderMeulen [:RyanVM]]

Was it intentional that this patch doesn't include any new tests?

Comment 137

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8874422 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v9

lol, whoops!

Comment 138

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/170c00a5b2e0
Implement the labels attribute. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/766cadeccb1c
Update web-platform tests for labels attribute. r=smaug

Comment 139

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/170c00a5b2e0
https://hg.mozilla.org/mozilla-central/rev/766cadeccb1c

Comment 140

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks John :)

Comment 141

[Sebastian Zartner [:sebo]]

I've added a release note to https://developer.mozilla.org/en-US/Firefox/Releases/56#HTML and created/updated the following pages:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLButtonElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLButtonElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLProgressElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLProgressElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMeterElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMeterElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLOutputElement/labels
https://developer.mozilla.org/en-US/docs/Web/API/HTMLOutputElement

John, can you please quickly check whether the info on the pages is ok?

Sebastian

Comment 142

[John Dai[:jdai]]

This looks good! The info on the pages are correct. Thanks! However, I saw HTMLInputElement and HTMLTextAreaElement have different labels properties style. I guess it should align with other pages. ex: HTMLSelectElement[3]


[1] https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement
[2] https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement
[3] https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement

Comment 143

[Sebastian Zartner [:sebo]]

(In reply to John Dai[:jdai] from comment #142)
> This looks good! The info on the pages are correct. Thanks!

Great! Thank you for the review!

> However, I saw
> HTMLInputElement and HTMLTextAreaElement have different labels properties
> style. I guess it should align with other pages. ex: HTMLSelectElement[3]

Yes, somebody changed the layout from definition lists to tables. I already fixed some pages, though some - like those two - are still missing.

As that's out of the scope of the documentation of the changes here, I mark this as dev-doc-complete.

Sebastian

Comment 144

[Adrian Florinescu [:aflorinescu]]

As per San-Francisco meeting with :vchen, marking this bug as qe-.