1374724 - Use the .labels API to get form control labels

Status: RESOLVED WORKSFORME

(Toolkit :: Form Autofill, enhancement, P5)

Description

[Matthew N. [:MattN]]

Now that bug 556743 is implemented we should use it for standardized behaviour and possibly faster performance. It will also test out the implementation of bug 556743.

Comment 1

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

What code would use this? .labels being live nodelist might actually lead to slower performance, unfortunately.
But depends on the case.

Comment 2

[Matthew N. [:MattN]]

The code from bug 1347176 (that this blocks): https://hg.mozilla.org/mozilla-central/rev/a9a0bb47b38d 
It's the reason John implemented the API for us in the first place.

Comment 3

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

Worth to test behavior. If findLabelElements runs only rarely, it might be faster since it doesn't need to be live. Or we could implement that in C++?
Or we could make elements not own labels list so that it gets GC/CC even before the element itself.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Here is the Talos test link for comparing the result of applying `.labels` API:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=00a4321ab2af497172f91c2eb641cc1bdffe424b&newProject=try&newRevision=990a698d096397274800853364cfb9ae87b7b6ae&framework=1&showOnlyImportant=0

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1374724 - Use the .labels API to improve FormAutofillUtils.findLabelElements.

Review commit: https://reviewboard.mozilla.org/r/152544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152544/

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Hey MattN, do you think we should remain the wrapper `findLabelElement` and its test?

Comment 7

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

In which kinds of pages would this be used?
(I'm rather worried about the performance. If .labels was proposed now, it probably wouldn't added to the spec because of its performance characteristics,)

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Hey smaug,

We need label strings to recognize the field type. In FormAutofill heuristics, the attribute of `id` and `name` will be the first clue to determine the type, and the label strings will be the second option to determine it.

In the example[1], `id` and `name` can not provide the signature. Eventually, the label string, "Company Name:" will be the clue to recognize the input as `organization` type.

[1] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/browser/extensions/formautofill/test/fixtures/third_party/OfficeDepot/ShippingAddress.html#60-64

Comment 9

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

Does form auto fill follow all the dom mutations, or does it use the information just right after page load or so?

Comment 10

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

jdai, do you think you could take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1374724#c3
If we let the labels object to die when GC next time runs (assuming nothing is having references to it), it could be perhaps used safely here without causing extra performance penalty to DOM mutations.

Comment 11

[John Dai[:jdai]]

Sure, I'll take a look at it. Keep NI for tracking.

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Use `Array.from` instead of `Array.splice`.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d260c088a7c891b685e8d7099613604c9e88abec&newProject=try&newRevision=897fbff0342f3be6bbde9e7ef8baac50700522c0&framework=1&showOnlyImportant=0

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Hey John, do you have any new investigation for the Talos issue? How about filing a new bug to discuss the API implementation?

Comment 14

[John Dai[:jdai]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> Hey John, do you have any new investigation for the Talos issue?
Per comment #12, It's 11.82% slower at google.com with osx platform. It seems like a noisy because its input filed has autocomplete=off attribute. After bug 1372261 landed, I would like to send a new tps talos test to see new result. 

> How about filing a new bug to discuss the API implementation?
It would be nice, if you file a new bug to separate performance tuning on that bug.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8881382 [details]
Bug 1374724 - Use the .labels API to improve FormAutofillUtils.findLabelElements.

https://reviewboard.mozilla.org/r/152544/#review163190

::: browser/extensions/formautofill/FormAutofillUtils.jsm:133
(Diff revision 1)
>    findLabelElements(element) {
> -    let document = element.ownerDocument;
> +    return Array.prototype.slice.call(element.labels);

The comparison results aren't loading for me… was `from` slower than `slice`? If not, please use `from` though it seems like we can't land this yet anyways.

Comment 16

[John Dai[:jdai]]

(In reply to John Dai[:jdai] (military leave 7/29 ~ 8/4) from comment #14)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> > Hey John, do you have any new investigation for the Talos issue?
> Per comment #12, It's 11.82% slower at google.com with osx platform. It
> seems like a noisy because its input filed has autocomplete=off attribute.
> After bug 1372261 landed, I would like to send a new tps talos test to see
> new result. 
> 
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a5c66ed69bda6d6941d47607f0330de9aab98555&newProject=try&newRevision=4859a4d489958c2cd944c952686ea079798327c8&framework=1&showOnlyImportant=0

Comment 17

[Sergey Galich [:serg]]

Looks like we are using .labels already.