1024437 - [e10s] HTML5 datalist does not work

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Alice0775 White]

Attachment: testcase

Steps To Reproduce:
1. Open e10s window
2. Open attached
3. Double click input field
   --- observe dropdown pop up
4. Type "red"
   --- observe dropdown pop up

Actual Results
No dropdown

Expected Results:
Dropdown list should be popped up

Comment 1

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58f1a387f984

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

The patch that I pushed to try just now passes my local (limited) testing. Assuming that it's green on try (and once it is green on try) I'm going to break it up into a couple of smaller patches for easier reviewing.

One more cosmetic thing that needs to be fixed as well is that the "separator" comment doesn't work in e10s meaning that the <datalist> autocomplete suggestions aren't separated from the form history suggestions. It should be fairly straightforward to implement it, but I won't block this bug on doing so.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e2bfd50e36f

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4983100f29f

Matt, I don't have a nicely broken up patch for you, sorry :-/ I'll still try to push this to reviewboard (with a long comment about it) if this try run goes green.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: MozReview Request: bz://1024437/mrbkap

/r/6791 - Bug 1024437 - Get rid of a deprecated API.
/r/6793 - Bug 1024437 - Make <datalist> work in e10s.

Pull down these commits:

hg pull -r ad1c4163bb603006ddaa91bc85546a30cf22e310 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Matthew N. [:MattN]]

Sorry for the delay on reviewing. I was on PTO more than expected in the last week. I'll try to take a look at this today.

Comment 10

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/6791/#review6239

Removing autoCompleteSearch should be fine.

Comment 11

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/6793/#review6241

I still need to look closer at the C++ and understand the new architecture better to see if there are other things that need to be updated.

::: toolkit/components/satchel/nsFormAutoComplete.js:8
(Diff revision 1)
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
> +const Cu = Components.utils;

Nit: You can switch to using object destructuring on `Components`

::: toolkit/components/satchel/AutoCompleteE10S.jsm:139
(Diff revision 1)
> +    if (message.data.datalistResult) {
> +      message.data.datalistResult =
> +        new FormAutoCompleteResult(message.data.untrimmedSearchString,
> +                                   Ci.nsIAutoCompleteResult.RESULT_SUCCESS,
> +                                   0, "", message.data.datalistResult.values,
> +                                   message.data.datalistResult.labels,
> +                                   [], null);

Maybe add a comment that you're converting between a vanilla object to a FormAutoCompleteResult instance?

::: toolkit/components/satchel/AutoCompleteE10S.jsm:152
(Diff revision 1)
> +                                             message.data.mockField,

Ugh, where does message.data.mockField get set? It looks like it should be .field but then that makes me wonder what this is used for if tests are passing.

::: toolkit/components/satchel/nsFormAutoComplete.js:23
(Diff revision 1)
> +function isAutocompleteDisabled(aField) {

It would be nice to just expose nsContentUtils::IsAutocompleteEnabled to JS since we have a few copies of this now.

::: toolkit/components/satchel/nsFormAutoComplete.js:148
(Diff revision 1)
> +                                        aPreviousResult,
> +                                        aDatalistResult,
> +                                        aListener) {

Can you add aDatalistResult to the JSDoc comment?

::: toolkit/components/satchel/nsFormAutoComplete.js:245
(Diff revision 1)
> +    mergeResults : function(historyResult, datasetResult) {

Nit: s/datsetResult/datalistResult/

You can also use newer method syntax without the function keyword:
mergeResults(historyResult, datasetResult) {

::: toolkit/components/satchel/nsFormAutoComplete.js:270
(Diff revision 1)
> +        // now put the history results above the suggestions

Both results could be seen as "suggestions" so maybe add the word "datalist" before "suggestions".

::: toolkit/components/satchel/nsFormAutoComplete.js:275
(Diff revision 1)
> +        let {FormAutoCompleteResult} = Cu.import("resource://gre/modules/nsFormAutoCompleteResult.jsm", {});

This might be obvious but why not import this lazily at the top of the file?

::: toolkit/components/satchel/nsFormAutoComplete.js:261
(Diff revision 1)
> +        // fill out the comment column for the suggestions
> +        // if we have any suggestions, put a label at the top
> +        if (values.length) {
> +            comments[0] = "separator";
> +        }
> +        for (let i = 1; i < values.length; ++i) {
> +            comments.push("");
> +        }

Nit: Could use Array.prototype.fill or a for…of loop to make this easier to read and less chance of mixing of the iterator variable (and then move the separator assignment after). I see now that you're just moving this code so you can leave it if you want.

::: toolkit/components/satchel/nsFormAutoComplete.js:418
(Diff revision 1)
> +      let mockField = isAutocompleteDisabled(aField) ? { autocomplete: "off" } : null;

The mockField should also be passing along maxLength, right? Can you double-check why that didn't get caught in a test? Maybe some satchel tests are disabled in e10s?

Comment 12

[Matthew N. [:MattN]]

I see that tests in https://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/test/mochitest.ini are disabled in e10s so that's why you didn't notice some of the issues. We should probably fix that first or at least do manual tests of all things those tests are checking. Refactoring with tests disabled is scary.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

https://reviewboard.mozilla.org/r/6793/#review6603

> Ugh, where does message.data.mockField get set? It looks like it should be .field but then that makes me wonder what this is used for if tests are passing.

I tested this manually but I think I changed the variable name just before pushing to reviewboard. I'm working on getting the tests working now.

> It would be nice to just expose nsContentUtils::IsAutocompleteEnabled to JS since we have a few copies of this now.

I can't find the other uses of this and it isn't obvious where to stick the function. I'd put it on nsIDOMWindowUtils, but that's actually not useful here because we don't have a window to use in nsFormAutoComplete.js (it's a module and we can't get a window in the e10s case since we're in the wrong process). If there's a better place to put this, let me know and I'll make the change.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I have no idea how to use reviewboard, apparently. I tried a few times to update the existing review with the result that I made https://reviewboard.mozilla.org/r/8135/ but that thinks that I'm working on bug 6793 (which, coincidentally, is the review id for the previous review). Matt, can you review that? Do you know how to fix the situation? If not, I might just switch to attaching patches to this bug as the fastest way to make progress.

The patch I just attached makes autocomplete=off work correctly. I have a patch that makes the test for <datalist> somewhat work in e10s, but it currently fails due to a preexisting bug in the e10s autocomplete. I'm going to work on that tomorrow in order to get the tests to pass, but I'd like to get the ball rolling on this review in the meantime.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8590432 [details]
MozReview Request: bz://1024437/mrbkap

/r/6791 - Bug 1024437 - Get rid of a deprecated API. r=MattN
/r/6793 - Bug 1024437 - Make <datalist> work in e10s.

Pull down these commits:

hg pull -r d66a353e82a9bdcaba87b0612d4d400f7cd1473c https://reviewboard-hg.mozilla.org/gecko/

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

mconley came to my rescue to figure out how to get reviewboard to do what I want!

Matt, I'm working on making test_form_autocomplete_with_list.html pass in e10s (it passes without) but I'm running into a bunch of existing bugs in the e10s autocomplete code. I probably won't have it passing entirely before your review here is done, but I'll keep working on it in the meantime.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 1162329 on making the test pass.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4d20ca7ee61

Comment 19

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/6793/#review7725

::: toolkit/components/satchel/AutoCompleteE10S.jsm:163
(Diff revision 2)
>      formAutoComplete.autoCompleteSearchAsync(message.data.inputName,
>                                               message.data.untrimmedSearchString,
> +                                             message.data.mockField,
>                                               null,

Add a comment about why we're using wrappedJSObject here (like in nsFormAutocomplete.js)

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8590432 [details]
MozReview Request: bz://1024437/mrbkap

https://reviewboard.mozilla.org/r/6789/#review7087

::: toolkit/components/satchel/nsFormAutoComplete.js:316
(Diff revisions 1 - 2)
> +        // tree, one in a module and one in this file. Datalist results need to
> +        // use the one defined in the module but the rest of this file assumes
> +        // that the one defined here. To get around that, we explicitly import

"that the one defined here" is missing some words or something

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8590432 [details]
MozReview Request: bz://1024437/mrbkap

/r/6791 - Bug 1024437 - Get rid of a deprecated API. r=MattN
/r/6793 - Bug 1024437 - Make <datalist> work in e10s.
/r/9079 - Bug 1024437 - Fix dynamic updates

Pull down these commits:

hg pull -r f7aa309b3390695c24777ddfc110739452be51a3 https://reviewboard-hg.mozilla.org/gecko/

Comment 22

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/6793/#review7733

I think you forgot to update android code: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=1be077af56e3#5883

::: toolkit/components/satchel/nsFormAutoComplete.js:467
(Diff revision 3)
> +      let mockField = {};
> +      if (isAutocompleteDisabled(aField))
> +          mockField.autocomplete = "off";
> +      if (aField.maxLength > -1)
> +          mockField.maxLength = aField.maxLength;

You don't seem to be passing mockField.form.autocomplete.

Could we just move the autocomplete disabled checks to here in the child? I'm not sure what that would look like for non-E10S though.

I think if we also move the searchbar-history blocking out a level too then we don't need to pass the field to autocompleteSearchAsync. That seems cleaner to me.

::: toolkit/components/satchel/nsFormFillController.cpp:1
(Diff revision 3)
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 sts=2 sw=2 et tw=80: */

Nit: since 2 spaces is the default for m-c, can we just delete both of the mode lines?

::: toolkit/components/satchel/nsFormFillController.cpp:651
(Diff revision 3)
> -    // It appears that mFocusedInput is always null when we are focusing a XUL
> -    // element. Scary :)
> -    if (!mFocusedInput || nsContentUtils::IsAutocompleteEnabled(mFocusedInput)) {
> +    nsCOMPtr<nsIAutoCompleteResult> datalistResult;
> +    rv = PerformInputListAutoComplete(aSearchString,
> +                                      getter_AddRefs(datalistResult));

The previous code skipped datalist support for XUL nodes and we should probably preserve that.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

https://reviewboard.mozilla.org/r/6793/#review7805

> Nit: since 2 spaces is the default for m-c, can we just delete both of the mode lines?

My experience has been that nobody respects any sort of default in the tree. Some files are 2-space indent, some are 4-space indent. I'd prefer to leave the modelines so I don't have to worry about where I am.

> You don't seem to be passing mockField.form.autocomplete.
> 
> Could we just move the autocomplete disabled checks to here in the child? I'm not sure what that would look like for non-E10S though.
> 
> I think if we also move the searchbar-history blocking out a level too then we don't need to pass the field to autocompleteSearchAsync. That seems cleaner to me.

I took a shortcut here (to avoid having to have a mock aField.form as well). Since autocomplete is off if the attribute exists on either the element or its form, I check in both places in the child and set autocomplete=off on the mock element in the parent, which conveys the same information.

I don't want to have too much additional logic in the child, partly because this code has to work in the parent as well. Even if I did the autocomplete=off checks in the child, I'd still have to send a message to the parent. I don't think it's worth doing that.

I'd like to do the searchbar-history moving in a separate patch (or bug, if you wouldn't mind).

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8590432 [details]
MozReview Request: bz://1024437/mrbkap

/r/6791 - Bug 1024437 - Get rid of a deprecated API. r=MattN
/r/6793 - Bug 1024437 - Make <datalist> work in e10s.
/r/9079 - Bug 1024437 - Fix dynamic updates

Pull down these commits:

hg pull -r 6aea66064d108579151213a52880891f0ab40270 https://reviewboard-hg.mozilla.org/gecko/

Comment 25

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/6793/#review8147

> I took a shortcut here (to avoid having to have a mock aField.form as well). Since autocomplete is off if the attribute exists on either the element or its form, I check in both places in the child and set autocomplete=off on the mock element in the parent, which conveys the same information.
> 
> I don't want to have too much additional logic in the child, partly because this code has to work in the parent as well. Even if I did the autocomplete=off checks in the child, I'd still have to send a message to the parent. I don't think it's worth doing that.
> 
> I'd like to do the searchbar-history moving in a separate patch (or bug, if you wouldn't mind).

> Even if I did the autocomplete=off checks in the child, I'd still have to send a message to the parent.

Why is that again?

Comment 26

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/6793/#review8149

> My experience has been that nobody respects any sort of default in the tree. Some files are 2-space indent, some are 4-space indent. I'd prefer to leave the modelines so I don't have to worry about where I am.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style is pretty clear that 2 spaces is the default (mentioned more than one) so my point is that if you editor default to 2 spaces for m-c then only files which differ from the coding style need the mode line.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e69f9c1639e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7837f943758c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e69f9c1639e
https://hg.mozilla.org/mozilla-central/rev/7837f943758c

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Matthew N. [:MattN] from comment #22)
> I think if we also move the searchbar-history blocking out a level too then
> we don't need to pass the field to autocompleteSearchAsync. That seems
> cleaner to me.

This doesn't actually buy us anything because we still have to pass the field name and the field around (because we still give <datalist> suggestions when autocomplete=off) so I won't file a followup here.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: MozReview Request: Bug 1024437 - Make <datalist> work in e10s.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: MozReview Request: Bug 1024437 - Get rid of a deprecated API. r=MattN

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: MozReview Request: Bug 1024437 - Fix dynamic updates

Comment 34

[Darren Kalck [:D-Kalck]]

Is bug #1314586 related to this one ?