1565075 - Convert textbox type="autocomplete" to HTML input

Status: RESOLVED FIXED

(Thunderbird :: General, task, P2)

Description

[Alessandro Castellani [:aleca]]

This should land as soon as bug 1534455 lands on m-c.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1565075-textbox-autocomplete-WIP.patch

This is an initial patch to convert the textbox type="autocomplete" to html:input is="autocomplete-input".

Lots of things are broken like the style (no idea why it doesn't apply), calendar attendees invite, and the address book list, but the basic UI and autocomplete functionalities are there.

I'm requesting an early feedback to be sure the edits I did to select the various elements are correct and I'm moving towards the right direction.

In order to test this you need to load bug 1534455, which is getting updated so most likely some things will change.

Comment 2

[Geoff Lankow (:darktrojan)]

Comment on attachment 9077514 [details] [diff] [review]
1565075-textbox-autocomplete-WIP.patch

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

Once step closer to cleaning up this nasty code. Nice.

I've made some comments on some XUL changes that apply to all of them.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ +94,5 @@
> +                        minresultsforpopup="1"
> +                        onblur="if (this.localName == 'textbox') { this.closest("calendar-event-attendees-list").returnHit(this, true) }"
> +                        ignoreblurwhilesearching="true"
> +                        oninput="this.setAttribute('dirty', 'true');"
> +                        ontextentered="this.closest("calendar-event-attendees-list").returnHit(this);"/>

These events will need checking. this.localName won't be textbox any more, it'll be input. I doubt ontextentered will fire, you might need to listen for keypress and check if the key pressed is return.

::: mail/components/addrbook/content/abEditListDialog.xul
@@ +59,5 @@
>        <richlistitem class="addressingWidgetItem" allowevents="true">
>          <hbox class="addressingWidgetCell" flex="1">
>            <image onclick="this.nextElementSibling.select();" class="person-icon"/>
> +          <html:input is="autocomplete-input"
> +                      id="addressCol1#1"

id should be on the same line as is.

@@ +61,5 @@
>            <image onclick="this.nextElementSibling.select();" class="person-icon"/>
> +          <html:input is="autocomplete-input"
> +                      id="addressCol1#1"
> +                      class="plain textbox-addressingWidget uri-element"
> +                      style="-moz-box-flex: 1;"

Can this go in a stylesheet somewhere?

@@ +72,5 @@
> +                      completeselectedindex="true"
> +                      minresultsforpopup="3"
> +                      ontextentered="awRecipientTextCommand(param, this); if (param &amp;&amp; this.value != '') param.preventDefault();"
> +                      onkeydown="awRecipientKeyDown(event, this);"
> +                      onclick="awNotAnEmptyArea(event);"/>

Same thing here about events.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +73,3 @@
>      inputElementType = document.getElementById("addressCol2#1").localName;
> +  }
> +  return `${inputElementType}[is="autocomplete-input"]`;

Probably best to rename this, since it's now returning a selector and not a name.

Actually, I think in all cases the return value of this function will be input[is="autocomplete-input"]. We can get rid of the function altogether if that's the case.

@@ +82,1 @@
>    return selectElementType;

And I think this will always return menulist.

@@ +276,4 @@
>    parentNode.appendChild(newNode); // we need to insert the new node before we set the value of the select element!
>  
> +  let input = newNode.querySelectorAll(awInputElementName());
> +  let select = newNode.querySelectorAll(awSelectElementName());

Since we only want the first, we could just use querySelector.

@@ +590,5 @@
>        // on a cloned row.
>        input[0].removeAttribute("nomatch");
>      }
> +
> +    let select = newNode.querySelectorAll(awSelectElementName());

Same here.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1565075-textbox-autocomplete-WIP.patch

Making progress with a few issues left.

The ontextentered works as expected, but I will wait till bug 1534455 before celebrating.
Also we need to wait for that patch to get reworked as some popup methods and styling still need to be converted.

The compose window works as expected.
The Address book works as expected with some minor styling issues.
The calendar attendees invite still doesn't work.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9077869 [details] [diff] [review]
1565075-textbox-autocomplete-WIP.patch

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

Looks ok

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +10,5 @@
>  top.MAX_RECIPIENTS = 1; /* for the initial listitem created in the XUL */
>  
> +let inputElementType = "";
> +let selectElementType = "";
> +let selectElementIndexTable = null;

please keep the top level vars

@@ +841,5 @@
>  
>  /* ::::::::::: addressing widget dummy rows ::::::::::::::::: */
>  
> +let gAWContentHeight = 0;
> +let gAWRowHeight = 0;

here too

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1565075-textbox-autocomplete.patch

This should be ready for a full review as all the issues have been solved and everything works as expected.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9078293 [details] [diff] [review]
1565075-textbox-autocomplete.patch

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

D33250 won't apply so hard to test...

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +944,5 @@
>  // Given an arbitrary block of text like a comma delimited list of names or a names separated by spaces,
>  // we will try to autocomplete each of the names and then take the FIRST match for each name, adding it the
>  // addressing widget on the compose window.
>  
> +let gAutomatedAutoCompleteListener = null;

this and some more top level declarations are still let

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1565075-textbox-autocomplete.patch

Sorry for missing those var, they should all be reverted now.
I'm attaching a rebased WIP of the D33250 patch so you can test this.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: D33250-WIP.diff

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9078476 [details] [diff] [review]
D33250-WIP.diff

autocomplete-input.js is not added in this patch

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: D33250.diff

Oopsie, sorry about that.
This should do it.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9078475 [details] [diff] [review]
1565075-textbox-autocomplete.patch

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

Looks good, thx! r=mkmelin

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +376,5 @@
>              if (event.originalTarget.localName == "input") {
> +                let input = event.originalTarget;
> +
> +                if (!this.inputID && input && input.ownerDocument &&
> +                    input.ownerDocument.documentURIObject.schemeIs("chrome")) {

what's this about? if there's not a good reason I'd remove it

Comment 12

[Alessandro Castellani [:aleca]]

With the updated patch of bug 1534455, any input with the ontextentered attribute causes TB to crash.
So, I have to rework this a little bit.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1565075-textbox-autocomplete.patch

Patch updated to remove the deprecated ontextentered.
I'm gonna wait on asking for a final review before the m-c patch lands.

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks for being proactive!

Comment 15

[Alessandro Castellani [:aleca]]

Bug 1534455 has landed on m-c
I'll get back to this bug and bug 1542720 to rebase the patches and get them ready for a full review.
Apologies if the autocomplete will be temporarily broken on trunk.

Comment 16

[Magnus Melin [:mkmelin]]

It was backed out from autoland, but I assume will be re-landing the next days.

Comment 17

[Jorg K (CEST = GMT+2)]

Quite a few test failures now that bug 1534455 landed. Not immediately obvious why test-general-content-policy.js, test-compose-mailto.js or test-about-support.js would be related. But then the M-C range is rather small, so it must come from there:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=04da00a61c6df437250e3af0d20f8ec72bbe9f48&tochange=9596d7f4a7457bccc78cadf9c39bcc9c4b5b97f8

Comment 18

[Jorg K (CEST = GMT+2)]

Well, bringing up a compose window already fails. It shows a collapsed attachment bucket with a paper clip. It's downhill from there.

Comment 19

[Alessandro Castellani [:aleca]]

Patch coming in a few minute.
I'm fixing some tests

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part1.patch

This is a WIP patch which it fixes the UI and autocomplete for the messengercompose and calendar attendees.
The "click-result-and-create-a-new-line" doesn't work anymore.
The Address book mailing list is partially broken.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 9093446 [details] [diff] [review]
1565075-Part1.patch

Hmm, about 50% of the last patch. I'll get it landed. As Magnus said, let's improve incrementally.

Comment 22

[Jorg K (CEST = GMT+2)]

The compose window looks a whole lot better with the patch. I tried one of the failing tests, and it passed. So the next push should look better.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d3bef0ac6ce
Convert textbox type=autocomplete to HTML input. rs=bustage-fix,jorgk

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part2.patch

This fixes the Mailing List dialogs.
Onto to the tests.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9093458 [details] [diff] [review]
1565075-Part2.patch

Thanks, one reviewer will do.

Comment 26

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part3.patch

This fixes a bunch of tests, not all done yet, more to come soon.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 9093480 [details] [diff] [review]
1565075-Part3.patch

Since I'm going to be on 4G/3G/HSPA/Edge or no internet for some of the day, I'll land those two now and we'll make it PLR on all the patches.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a19c6eb25d8b
Convert textbox type=autocomplete to HTML input, part 2, mailing lists. rs=bustage-fix,jorgk
https://hg.mozilla.org/comm-central/rev/d109b7903efb
Convert textbox type=autocomplete to HTML input, part 3, tests. rs=bustage-fix,jorgk DONTBUILD

Comment 29

[Jorg K (CEST = GMT+2)]

I pushed that with DONTBUILD since M-C have only merged three changesets since our last push, so here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6a9ea3516f87b74e1903930f0a24a66814bcd2e3

If all goes well, only one Gloda test (test-keyboard-interface.js::test_escape_does_not_reach_us_from_gloda_search) will remain broken, but that's for bug 1542720.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 9093458 [details] [diff] [review]
1565075-Part2.patch

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

::: mailnews/addrbook/content/abMailListDialog.js
@@ +428,3 @@
>      }
>    }
> +  return input ? input : null;

return null;

querySelector returns null if nothing's found

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 9093446 [details] [diff] [review]
1565075-Part1.patch

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

Too bitrotted to check.

Looks reasonable, but likely incomplete.
For accessibility, looks like you should wrap the autocomplete inputs in an <hbox role="combobox"> (or such), and maybe some aria-owns, see https://hg.mozilla.org/mozilla-central/rev/9ec431469e37

Comment 32

[Jorg K (CEST = GMT+2)]

Too bitrotted to check.

How can it be bitrotten if it landed already?

Comment 33

[Magnus Melin [:mkmelin]]

Ah, only noticed parts 2 and 3 landing since part 1 landed earlier.

Comment 34

[Jorg K (CEST = GMT+2)]

So, is that an r+ depending on getting the a11y stuff fixed?

Comment 35

[Magnus Melin [:mkmelin]]

Comment on attachment 9093446 [details] [diff] [review]
1565075-Part1.patch

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

I suppose.

Comment 36

[Alessandro Castellani [:aleca]]

Thanks for the reviews, I'll do those fixes.
Jorg, do you want me to replace them or add a new patch with the edits so you don't have to backout what already landed?

Comment 37

[Jorg K (CEST = GMT+2)]

No need for backouts, just add more parts, like: 4: a11y, 5: more tests, etc.

Comment 38

[Richard Marti (:Paenglab)]

Attachment: 1565075-dark-write-autocomplete-fix.patch

Alessandro, with the dark theme in composer, the addressingWidget (the From/CC/BCC textfield) has when selected/hovered a white background with white text, which isn't very optimal to read the address. Also the autocomplete-richlistbox is white.

Removing the html|input fixes the white on white. On this files is no XUL namespace defined and classes work without namespace prefix.

The autocomplete-popup needed some changes to work again after the de-XBL.

Comment 39

[Alessandro Castellani [:aleca]]

Comment on attachment 9093655 [details] [diff] [review]
1565075-dark-write-autocomplete-fix.patch

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

Looks good, thanks for taking care of this.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/14221930e600
Fix the textbox-addressingWidget and the autocomplete-richlistbox in composer with dark theme. r=aleca

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/51f6d35de7f4
temporarily disable testEventDialog and testEventDialogModificationPrompt. rs=bustage-fix

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to Alessandro Castellani (:aleca) from comment #26)

This fixes a bunch of tests, not all done yet, more to come soon.

Any progress? I've disabled those two tests now.

Comment 43

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part4.patch

This patch takes care of the accessibility issues.

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 9093974 [details] [diff] [review]
1565075-Part4.patch

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

Thx! r=mkmelin

Comment 45

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part5.patch

And this should be the last patch, fixing the calendar tests.
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a7952fa48ccf780c887e6a09992070ebf8de4c0e

There's still the toolkit/components/search/tests/xpcshell/test_json_cache_good.js test failing.
Is it OK to touch tests inside the toolkit? Is this up to us?

Comment 46

[Magnus Melin [:mkmelin]]

Wouldn't think such a test is related to this.
You can fix toolkit stuff if there is something wrong there. But it needs another patch (m-c only part) and other reviewer.

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 9094011 [details] [diff] [review]
1565075-Part5.patch

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

Looks good, if the try build is happy. r=mkmelin

Comment 48

[Jorg K (CEST = GMT+2)]

And this should be the last patch, fixing the calendar tests.

And the onclick issue?

toolkit/components/search/tests/xpcshell/test_json_cache_good.js --> Bug 1582086.

Comment 49

[Alessandro Castellani [:aleca]]

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #48)

And the onclick issue?

True, I just checked my notes and that's missing.
There's the possibility of that issue being related to m-c. I'll continue for another couple of hours trying to find a fix, but if I fail, I'll open a bug and ping Tim, which he kindly offered to help on this issue.

Which me luck!

Comment 50

[Jorg K (CEST = GMT+2)]

We can always open another bug. I wish you luck!!

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/592b0b1a57a9
Convert textbox type=autocomplete to HTML input, part 4, a11y. r=mkmelin
https://hg.mozilla.org/comm-central/rev/9e2073457474
Convert textbox type=autocomplete to HTML input, part 5, more tests. r=mkmelin

Comment 52

[Jorg K (CEST = GMT+2)]

Maybe best to do the onclick in a new bug.

Comment 53

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part6.patch

This fixes the click issue on autocomplete.
It's incomplete as I need to adapt this method to the Calendar attendees, which is a whole other beast.
Also, the click-and-go-to-next-line behaviour doesn't happen if you edit a row in the Address Book Mailing List, so that needs fixing as well.

Anyway, I'm uploading the patch to gather feedback and see if you guys like it as a solution.

Comment 54

[Alessandro Castellani [:aleca]]

Attachment: 1565075-Part6.patch

OK, I think I covered everything and this is ready for a full review.

Comment 55

[Magnus Melin [:mkmelin]]

Comment on attachment 9094072 [details] [diff] [review]
1565075-Part6.patch

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

Looks good to me, r=mkmelin

Comment 56

[Jorg K (CEST = GMT+2)]

You forgot to remove onRecipientClicked() which is now unused. I'll fix it.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5156145ef2bc
Fix autocomplete click behavior. r=mkmelin,jorgk DONTBUILD