Closed Bug 1565075 Opened 3 months ago Closed 26 days ago

Convert textbox type="autocomplete" to HTML input

Categories

(Thunderbird :: General, task, P2)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: aleca, Assigned: aleca)

References

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(7 files, 8 obsolete files)

34.47 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
3.81 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
3.89 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
6.49 KB, patch
aleca
: review+
Details | Diff | Splinter Review
6.49 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
4.35 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
10.51 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review

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

Assignee: nobody → alessandro
Priority: -- → P2

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.

Attachment #9077514 - Flags: feedback?(mkmelin+mozilla)
Attachment #9077514 - Flags: feedback?(geoff)
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.
Attachment #9077514 - Flags: feedback?(geoff) → feedback+

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.

Attachment #9077514 - Attachment is obsolete: true
Attachment #9077514 - Flags: feedback?(mkmelin+mozilla)
Attachment #9077869 - Flags: feedback?(mkmelin+mozilla)
Attachment #9077869 - Flags: feedback?(geoff)
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
Attachment #9077869 - Flags: feedback?(mkmelin+mozilla)
Attachment #9077869 - Flags: feedback?(geoff)
Attachment #9077869 - Flags: feedback+
Status: NEW → ASSIGNED

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

Attachment #9077869 - Attachment is obsolete: true
Attachment #9078293 - Flags: review?(mkmelin+mozilla)
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

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.

Attachment #9078293 - Attachment is obsolete: true
Attachment #9078293 - Flags: review?(mkmelin+mozilla)
Attachment #9078475 - Flags: review?(mkmelin+mozilla)
Attached patch D33250-WIP.diff (obsolete) — Splinter Review
Comment on attachment 9078476 [details] [diff] [review]
D33250-WIP.diff

autocomplete-input.js is not added in this patch
Attachment #9078476 - Attachment is obsolete: true
Mentor: alessandro
Attached patch D33250.diff (obsolete) — Splinter Review

Oopsie, sorry about that.
This should do it.

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
Attachment #9078475 - Flags: review?(mkmelin+mozilla) → review+

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.

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

Attachment #9078475 - Attachment is obsolete: true
Attachment #9078773 - Attachment is obsolete: true
Attachment #9079542 - Flags: review+

Thanks for being proactive!

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.

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

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

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

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

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.

Attachment #9079542 - Attachment is obsolete: true
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.
Attachment #9093446 - Flags: review?(mkmelin+mozilla)

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.

Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d3bef0ac6ce
Convert textbox type=autocomplete to HTML input. rs=bustage-fix,jorgk

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

Attachment #9093458 - Flags: review?(mkmelin+mozilla)
Attachment #9093458 - Flags: review?(jorgk)
Comment on attachment 9093458 [details] [diff] [review]
1565075-Part2.patch

Thanks, one reviewer will do.
Attachment #9093458 - Flags: review?(jorgk)
Attachment #9093446 - Attachment description: 1565075-WIP.patch → 1565075-Part1.patch
Attachment #9093446 - Attachment filename: 1565075-WIP.patch → 1565075-Part1.patch

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

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.
Attachment #9093480 - Flags: review?(mkmelin+mozilla)
Attachment #9093480 - Flags: review?(geoff)
Pushed by mozilla@jorgk.com:
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

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 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
Attachment #9093458 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9093480 - Flags: review?(mkmelin+mozilla)
Attachment #9093480 - Flags: review?(geoff)
Attachment #9093480 - Flags: review+
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
Attachment #9093446 - Flags: review?(mkmelin+mozilla)

Too bitrotted to check.

How can it be bitrotten if it landed already?

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

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

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

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

I suppose.
Attachment #9093446 - Flags: review+

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?

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

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.

Attachment #9093655 - Flags: review?(alessandro)
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.
Attachment #9093655 - Flags: review?(alessandro) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/14221930e600
Fix the textbox-addressingWidget and the autocomplete-richlistbox in composer with dark theme. r=aleca
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/51f6d35de7f4
temporarily disable testEventDialog and testEventDialogModificationPrompt. rs=bustage-fix

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

This patch takes care of the accessibility issues.

Attachment #9093974 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9093974 [details] [diff] [review]
1565075-Part4.patch

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

Thx! r=mkmelin
Attachment #9093974 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

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?

Attachment #9094011 - Flags: review?(mkmelin+mozilla)

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 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
Attachment #9094011 - Flags: review?(mkmelin+mozilla) → review+

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.

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

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

Keywords: leave-open

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Closed: 26 days ago
Keywords: checkin-needed
Resolution: --- → FIXED

Maybe best to do the onclick in a new bug.

Target Milestone: --- → Thunderbird 71.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch 1565075-Part6.patch (obsolete) — Splinter Review

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.

Attachment #9094043 - Flags: review?(mkmelin+mozilla)

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

Attachment #9094043 - Attachment is obsolete: true
Attachment #9094043 - Flags: review?(mkmelin+mozilla)
Attachment #9094072 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9094072 [details] [diff] [review]
1565075-Part6.patch

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

Looks good to me, r=mkmelin
Attachment #9094072 - Flags: review?(mkmelin+mozilla) → review+

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

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

Status: REOPENED → RESOLVED
Closed: 26 days ago26 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.