Implement the labels attribute

ASSIGNED
Assigned to

Status

()

Core
DOM: Core & HTML
ASSIGNED
7 years ago
22 hours ago

People

(Reporter: mounir, Assigned: jdai)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [form autofill:M3], URL)

Attachments

(10 attachments, 6 obsolete attachments)

3.14 KB, patch
Details | Diff | Splinter Review
41.09 KB, patch
Details | Diff | Splinter Review
64.45 KB, patch
Details | Diff | Splinter Review
355.51 KB, text/plain
Details
53.59 KB, patch
Details | Diff | Splinter Review
919 bytes, text/html
Details
42.90 KB, patch
smaug
: review-
Details | Diff | Splinter Review
7.84 KB, patch
Details | Diff | Splinter Review
18.83 KB, patch
jdai
: feedback?
jessica
Details | Diff | Splinter Review
18.97 KB, patch
jdai
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

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

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

Updated

7 years ago
Blocks: 559750
(Reporter)

Comment 1

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

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

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

Comment 2

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

Comment 3

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

Comment 4

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

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

Comment 5

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

Updated

7 years ago
Keywords: html5
(Reporter)

Updated

7 years ago
Depends on: 562932
(Reporter)

Comment 6

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

As requested by Olli on IRC, this patch is including "label.control" patch and  has just been done against HEAD. It may help if you want to build and have a look.

Comment 7

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

Comment 8

7 years ago
Created attachment 445717 [details]
Tests log

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

Updated

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

Updated

7 years ago
Keywords: dev-doc-needed

Comment 9

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

Comment 10

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

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

Comment 11

7 years ago
(In reply to comment #10)

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

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

Comment 12

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

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

Comment 13

7 years ago
(In reply to comment #12)

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

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

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

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

Comment 14

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

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

Updated

6 years ago
Blocks: 617528

Updated

6 years ago
No longer blocks: 617528

Comment 15

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

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

Actually we need something very similar to GetElementById().

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

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

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

Comment 16

6 years ago
(In reply to comment #15)

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

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

Comment 17

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

Comment 18

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

Comment 19

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

Updated

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

Comment 20

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

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

Comment 21

6 years ago
Actually, I do:

nsHTMLInputElement::GetLabels(nsIDOMNodeList** aLabels)

...

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

...

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

Comment 22

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

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

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

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

Comment 24

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

That's the magic of HTML specs ;)

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

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

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

Updated

4 years ago
Duplicate of this bug: 850365

Comment 26

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

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

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

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

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

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

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

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

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

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

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

* Removed an unused member variable

* Removed some whitespace changhes

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

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

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


Thank you Olli and take your time :)

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

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

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

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


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

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

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


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


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

Comment 33

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

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

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

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

Comment 35

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

Comment 36

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

Updated

3 years ago
Status: ASSIGNED → NEW
(Assignee)

Updated

15 days ago
See Also: → bug 1339726
(Assignee)

Updated

11 days ago
Duplicate of this bug: 567740
(Assignee)

Updated

11 days ago
Duplicate of this bug: 1339726
(Assignee)

Comment 40

11 days ago
I would like to take this bug.
Assignee: nobody → jdai
Status: NEW → ASSIGNED

Updated

11 days ago
Whiteboard: [form autofill:MVP]
(Assignee)

Comment 41

11 days ago
Created attachment 8858792 [details] [diff] [review]
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
Attachment #8858792 - Flags: review?(bugs)
(Assignee)

Comment 42

11 days ago
Keygen .labels was passed by Chromium and Safari at labelable-elements.html. Hence, I didn't remove labelable-elements.html keygen portion.
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.
Attachment #8858792 - Flags: review?(bugs)
Attachment #8858792 - Flags: review-
Attachment #8858792 - Flags: feedback?(jjong)
(Assignee)

Comment 44

11 days ago
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.
I would expect that using nsContentList (not nsSimpleContentList) would be able to deal with the live-ness of the list correctly.
(Assignee)

Comment 46

10 days ago
(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.
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 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.
Attachment #8858792 - Flags: feedback?(jjong)
(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)
(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.
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.
Though, then one needs to think how to clear the hashtable in a fast manner.

Comment 53

10 days ago
(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

10 days ago
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

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

Comment 56

10 days ago
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
(Assignee)

Comment 57

10 days ago
(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?
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.)
(Assignee)

Comment 59

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

Comment 60

7 days ago
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
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
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...
depending on how commonly these are used. Accessing node properties can be a bit slow.
(Assignee)

Comment 65

2 days ago
Created attachment 8861919 [details] [diff] [review]
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
Attachment #8858792 - Attachment is obsolete: true
Attachment #8861919 - Flags: review?(jjong)
(Assignee)

Comment 66

2 days ago
Created attachment 8861922 [details] [diff] [review]
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.
Attachment #8861922 - Flags: review?(jjong)

Updated

2 days ago
Whiteboard: [form autofill:MVP] → [form autofill:M3]
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
Attachment #8861919 - Flags: review?(jjong)
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.
Attachment #8861922 - Flags: review?(jjong) → feedback+
(Assignee)

Comment 69

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

Comment 70

23 hours ago
Created attachment 8862408 [details] [diff] [review]
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
Attachment #8861919 - Attachment is obsolete: true
Attachment #8861922 - Attachment is obsolete: true
Attachment #8862408 - Flags: feedback?(jjong)
(Assignee)

Comment 71

23 hours ago
Created attachment 8862411 [details] [diff] [review]
Bug 556743 - Update web-platform tests for labels attribute. v3

- Add a testcase for labelable elements moved to somewhere case.
Attachment #8862411 - Flags: feedback+
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.
You need to log in before you can comment on or make changes to this bug.