Switch toolkit Form Autocomplete popup from using a <xul:tree> to using a <xul:richlistbox>

VERIFIED FIXED in Firefox 52

Status

()

Toolkit
Form Manager
P3
normal
VERIFIED FIXED
a year ago
4 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 3 bugs, {qawanted})

Trunk
mozilla52
qawanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
The Form Autocomplete popup that we use uses a slightly modified subclass of the autocomplete.xml#autocomplete-result-popup binding.

This popup renders an nsIAutoCompleteResult using a <xul:tree>.

This is problematic for a few reasons:

1) A XUL tree is total overkill for showing a single list of undecorated results like what is shown inside of an autocomplete popup.

2) Individual rows are limited in how they can be styled. Sure, we can use things like -moz-tree-cell, -moz-tree-row, or -moz-tree-image (see https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Styling_a_Tree) but there really is a limit to what we can do here. For example, it's not possible for different rows to have different heights. It's also not possible to use SVG filters for the images in a XUL tree, which limits what kind of icons we can put in (and UX is tending more towards SVGs)

I briefly investigated adding SVG filter support to XUL trees just for the heck of it, and it's non-trivial. Engineering effort would probably be best spent elsewhere.



In bug 1294502, I made it so that both e10s and non-e10s share a unified Form Autocomplete mechanism. This bug is the next step, in order to replace the popup with something more flexible and customizable for styling.

While it's technically possible at this time to have only Firefox use a richlistbox and keep the other XUL apps using a tree, I think this would lead to complications over time as Firefox's needs from the Form Autocomplete popup evolve and diverge.

So this bug will switch the mechanism over to using a richlistbox based popup. I don't believe Fennec will be affected by this at all, since they appear to query nsIFormAutoComplete directly and do their own thing with the results[1]. Other XUL app consumers (I'm looking at you Thunderbird and SeaMonkey), assuming they use Form Autocomplete, will likely be affected by this change.

[1]: http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/mobile/android/chrome/content/browser.js#5280
(Assignee)

Comment 1

a year ago
I'll also mention that xul:trees are notoriously difficult to style properly because the body of the tree is opaque to our devtools. The tree body is a monolithic DOM node that contains a bunch of internal frames that can't be individually inspected or manipulated outside of the -moz-tree-* pseudoselectors.
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8783069 [details]
Bug 1296638 - Make Form Autocomplete popup use a richlistbox instead of a tree.

https://reviewboard.mozilla.org/r/73028/#review70810

This behaves for me locally, but does not pass some tests (mainly because there are some tests that manually inspect the tree of the old popup that will need to be adjusted). I'll fix the tests in a patch that will follow this one.

I'm also not 100% sure I've done this in a way that doesn't somehow screw TB / SM. Almost surely they'll need to alter any of their form autocomplete popups to be of type="autocomplete-richlistbox", however.
Comment hidden (mozreview-request)
Comment on attachment 8783069 [details]
Bug 1296638 - Make Form Autocomplete popup use a richlistbox instead of a tree.

https://reviewboard.mozilla.org/r/73028/#review72332

f+ while waiting on tests.

::: toolkit/components/satchel/AutoCompletePopup.jsm:112
(Diff revision 2)
>  
>    handleEvent: function(evt) {
>      if (evt.type === "popuphidden") {
> +      AutoCompleteResultView.clearResults();
> +      this.sendToBrowser("FormAutoComplete:PopupClosed");
> +      // adjustHiehgt clears the height from the popup so that

typo: adjustHiehgt

::: toolkit/components/satchel/AutoCompletePopup.jsm:297
(Diff revision 2)
> +   * @param message (string)
> +   *        The name of the message to send.
> +   * @param data (object, optional)
> +   *        The data to send down with the message.
> +   */
> +  sendToBrowser(message, data={}) {

Nit: spaces around operators
Attachment #8783069 - Flags: review?(MattN+bmo)
(Assignee)

Updated

a year ago
Assignee: nobody → mconley
(Assignee)

Comment 6

a year ago
Hey Marco!

Gijs mentioned that this switch from a <xul:tree> to a <xul:richlistbox> might be problematic from an accessibility point of view, and suggested you might want to test it out. I'm not sure if you have the time for this, but I thought I'd check.

If so, here are some builds (I can't remember which OS you prefer!)

Linux: https://archive.mozilla.org/pub/firefox/try-builds/mconley@mozilla.com-c801e4d2254d4255d2fa13bf1ab538e826577a84/try-linux64/firefox-51.0a1.en-US.linux-x86_64.tar.bz2
OS X: https://archive.mozilla.org/pub/firefox/try-builds/mconley@mozilla.com-c801e4d2254d4255d2fa13bf1ab538e826577a84/try-macosx64/firefox-51.0a1.en-US.mac.dmg
Windows: https://archive.mozilla.org/pub/firefox/try-builds/mconley@mozilla.com-c801e4d2254d4255d2fa13bf1ab538e826577a84/try-win32/firefox-51.0a1.en-US.win32.zip

If you don't have time to do this, that's okay, just let me know!
Flags: needinfo?(mzehe)
I just looked this over and don't see anything wrong with it. But with all the self-driven autocompletes everywhere, it's actually hard to find a form that we still serve our UI to. :-) But from the forms I tried, I found none to be of a problem. So I actually hope I got that new UI at all.
Flags: needinfo?(mzehe)
(Assignee)

Comment 8

a year ago
(In reply to Marco Zehe (:MarcoZ) from comment #7)
> I just looked this over and don't see anything wrong with it. But with all
> the self-driven autocompletes everywhere, it's actually hard to find a form
> that we still serve our UI to. :-) But from the forms I tried, I found none
> to be of a problem. So I actually hope I got that new UI at all.

Thanks Marco. I'll attach a test page with a simple form that you can test on.
(Assignee)

Comment 9

a year ago
Created attachment 8787246 [details]
Testing form
(Assignee)

Updated

a year ago
Attachment #8787246 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 10

a year ago
Alright, attachment 8787246 [details] should show autocomplete results if you put a few things inside the text input and submit a few times.
Flags: needinfo?(mzehe)
All right, works fine.
Flags: needinfo?(mzehe)
(Assignee)

Comment 12

a year ago
(In reply to Marco Zehe (:MarcoZ) from comment #11)
> All right, works fine.

Outstanding, thank you!
Blocks: 1300995

Updated

11 months ago
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
(Assignee)

Comment 13

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=067f4d7330f3
Blocks: 1304224
Blocks: 1217162
(Assignee)

Comment 14

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e21e84e13f40
(Assignee)

Comment 15

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86a3c5031354
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69c4c91c6a93
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0bea5e6cc87
(Assignee)

Comment 25

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=884508e4d189
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

11 months ago
There are some backend patches in Bug 441414 (Treerows need a way to hold richer content) that might help.

> This is a major feature; major features add substantial maintenance cost
> over time, and we're not interested in adding to that cost by adding
> additional XUL features.

On the other hand now that we have a use case for a rich tree perhaps that bug can be revived.
(Assignee)

Comment 30

11 months ago
Review ping.
Comment on attachment 8793895 [details]
Bug 1296638 - Refactor some autocomplete tests to be more reliable.

https://reviewboard.mozilla.org/r/80504/#review83670

TL;DR: I would prefer we don't refactor these tests in this bug and ideally not do that before landing the new implementation as it's too hard to review properly.

So my main reason for putting off this bugs' reviews are because it seems like you're changing the behaviour of a test in the middle of a series of commits related to changing the behaviour of the feature. This makes it hard for me to know if a test change was because something broke vs. just updating it to a newer style. I personally don't find these tests very hard to read compared to how they were a few years ago and reviewing the changes is a PITA because diff tools have a hard time matching the old code up to the new code. Can we just leave these tests in the old style for now or make a diff that's easier to review? The timing of some autocomplete events is important and I worry that you're making the test no longer test some things it needs to. For example, we need to ensure that if the user hits enter while an autocomplete popup is open and an entry is selected that the value is completed into the input BEFORE the form gets submitted if the page is listening for certain key events. I don't think you're necessarily regress the behaviour but I worry that you may no longer catch such a regression. I haven't spent the long time to investigate if you are in fact changing relevant timing because it seems very tedious (which is why I've put off this review) and the benefits of the new style don't outweigh the costs IMO. I would also prefer that such a major change to the test harness was landed some days separate from a push that rewrites an implementation so intermittent test failures can be more easily be correlated with the test refactoring vs. the feature reimplementation.

If we want to refactor the tests, I suggest we do that in a new bug after landing the new implementation (so as not to block moving forward) and ideally that bug would split the refactoring into many smaller commits with easier to read diffs.
Attachment #8793895 - Flags: review?(MattN+bmo) → review-
Comment on attachment 8783069 [details]
Bug 1296638 - Make Form Autocomplete popup use a richlistbox instead of a tree.

https://reviewboard.mozilla.org/r/73028/#review83672

::: toolkit/components/satchel/AutoCompletePopup.jsm:74
(Diff revision 5)
> +  _focus() {
> +    AutoCompletePopup.requestFocus();
> +  },

Where is this used?

::: toolkit/content/browser-content.js:1393
(Diff revision 5)
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAutoCompletePopup]),
> +
> +  _connected: false,

Move these back to avoid changing blame unless there was a good reason for this?

When I first skimmed this patch a few weeks ago I was thrown off by this change because I didn't realize it was a move.
Attachment #8783069 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8793896 [details]
Bug 1296638 - Adjust autocomplete test helpers to account for the new richlistbox implementation.

https://reviewboard.mozilla.org/r/80506/#review83680

::: toolkit/components/satchel/test/parent_utils.js:74
(Diff revision 3)
> -          gAutocompletePopup.tree.view.getCellText(0, gAutocompletePopup.tree.columns[0]) ===
> +          gAutocompletePopup.view.getValueAt(0) === expectedFirstValue);
> -          expectedFirstValue);

For datalist (and soon Form Autofill), the value and label/comment can differ so I assume this matches the expectations of existing tests.
Attachment #8793896 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8793894 [details]
Bug 1296638 - AutoCompletePopup in browser-content.js and AutoCompletePopup.jsm need to have popupOpen be in sync.

https://reviewboard.mozilla.org/r/80502/#review83682

Hey Mike, can you provide more context for what this commit is all about? Perhaps in the commit message?
Apologies for the delay. I really should have just mentioned my concerns in comment 31 sooner rather than waiting on more time to distill them clearer. I was trying to be conscious of the time it would have taken you to update those tests and didn't want to just suggest putting that off without further thought. I also felt conflicted because I had said on IRC that I was fine with modernizing the tests but I didn't expect it to be done in the middle of the series of this bug nor did I expect diffs that were that hard to follow as dolske and I had recently updated other autocomplete tests in other bugs this year and split them into much smaller pieces for review.

(In reply to Philip Chee from comment #29)
> There are some backend patches in Bug 441414 (Treerows need a way to hold
> richer content) that might help.
> 
> > This is a major feature; major features add substantial maintenance cost
> > over time, and we're not interested in adding to that cost by adding
> > additional XUL features.

Yes, I'm aware of that bug and discussion and think I actually mentioned it to mconley before he started on this.

> On the other hand now that we have a use case for a rich tree perhaps that
> bug can be revived.

We don't need tree features in the autocomplete popups so richlistbox actually makes more sense to me TBH.
(Assignee)

Comment 36

11 months ago
(In reply to Matthew N. [:MattN] (behind on requests) from comment #35)
> Apologies for the delay. I really should have just mentioned my concerns in
> comment 31 sooner rather than waiting on more time to distill them clearer.
> I was trying to be conscious of the time it would have taken you to update
> those tests and didn't want to just suggest putting that off without further
> thought. I also felt conflicted because I had said on IRC that I was fine
> with modernizing the tests but I didn't expect it to be done in the middle
> of the series of this bug nor did I expect diffs that were that hard to
> follow as dolske and I had recently updated other autocomplete tests in
> other bugs this year and split them into much smaller pieces for review.

No problem. I understand that the test refactor is a massive patch. Sometimes I forget the big changes like this are a pain to review! I agree that splitting the test refactor out into its own bug is useful not only for sanity, but also to help figure out where intermittents might be introduced. Good thinking.

I will attempt to make the current tests pass with my changes, and then file a new bug for the test refactoring. :)
(Assignee)

Updated

11 months ago
Blocks: 1309654
Comment on attachment 8793894 [details]
Bug 1296638 - AutoCompletePopup in browser-content.js and AutoCompletePopup.jsm need to have popupOpen be in sync.

https://reviewboard.mozilla.org/r/80502/#review84310

See comment 34.
Attachment #8793894 - Flags: review?(MattN+bmo)
(Assignee)

Comment 38

10 months ago
mozreview-review-reply
Comment on attachment 8783069 [details]
Bug 1296638 - Make Form Autocomplete popup use a richlistbox instead of a tree.

https://reviewboard.mozilla.org/r/73028/#review83672

> Where is this used?

Called several times by the popup bindings in autocomplete.xml. It's a method that the autocomplete.xml#autocomplete binding (which implements nsIAutoCompleteInput) uses to move focus to the input.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8793895 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8793894 [details]
Bug 1296638 - AutoCompletePopup in browser-content.js and AutoCompletePopup.jsm need to have popupOpen be in sync.

https://reviewboard.mozilla.org/r/80502/#review84974
Attachment #8793894 - Flags: review?(MattN+bmo) → review+

Comment 45

10 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/336c201c32bc
AutoCompletePopup in browser-content.js and AutoCompletePopup.jsm need to have popupOpen be in sync. r=MattN
https://hg.mozilla.org/integration/autoland/rev/e4e0419afca7
Make Form Autocomplete popup use a richlistbox instead of a tree. r=MattN
https://hg.mozilla.org/integration/autoland/rev/4b2fef1aa33e
Adjust autocomplete test helpers to account for the new richlistbox implementation. r=MattN
Since the insecure passwords feature will depend on this and that is targeted for 52 (with only a few weeks left), it would be great if QA could verify that autocomplete popups still work properly in the following conditions:

E10S:
* e10s on
* e10s off

Field types:
* non-login fields with only form history
* non-login fields with datalist
* non-login fields with datalist and form history (e.g. language and icon fields at [1])

Interactions (and others):
* Opening the popup (down arrow, typing)
* Closing the popup with an entry selected/highlighted (hitting ESC, hitter enter, hitting TAB, clicking outside)
* Closing the popup without an entry selected/highlighted (hitting ESC, hitter enter, hitting TAB, clicking outside)
* Filtering the results (typing, backspacing, deleting, etc.)

[1] https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm
Flags: qe-verify+
Keywords: qawanted

Comment 47

10 months ago
At this point, the only consumer of browser-autocomplete-result-popup is browser-search-autocomplete-result-popup, so it's not worth to keep it around.
Do you have plans to also move the search autocomplete to RLB? Otherwise, it would still be wise to merge those 2 bindings in search.xml.
Let me know your plans and then we can file bugs depending on that.
Flags: needinfo?(MattN+bmo)

Comment 48

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/336c201c32bc
https://hg.mozilla.org/mozilla-central/rev/e4e0419afca7
https://hg.mozilla.org/mozilla-central/rev/4b2fef1aa33e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

10 months ago
Blocks: 1310942

Comment 49

10 months ago
I filed bug 1310942 for comment 47, I'm leaving the needinfo just as an heads up.
(Assignee)

Updated

10 months ago
Depends on: 1311189
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1288498

Updated

10 months ago
Blocks: 1300547

Updated

10 months ago
Depends on: 1312244
Comment 46 has a summary of things that might break with this that we should take a look at.
Flags: needinfo?(rares.bologa)

Updated

10 months ago
Flags: needinfo?(rares.bologa) → needinfo?(jwilliams)
Bug 1311189 is a known regression that will be fixed in tomorrow's Nightly. See the other dependencies for known regresions.
Flags: needinfo?(MattN+bmo)
(In reply to Marco Bonardo [::mak] from comment #49)
> I filed bug 1310942 for comment 47, I'm leaving the needinfo just as an
> heads up.

Fine with me since it seems to mostly be two methods.

Updated

10 months ago
Depends on: 1313067

Updated

10 months ago
Depends on: 1313130

Updated

10 months ago
Depends on: 1313131

Updated

10 months ago
Depends on: 1313132

Updated

10 months ago
Depends on: 1313134

Updated

10 months ago
No longer depends on: 1313132

Updated

10 months ago
No longer depends on: 1313131

Updated

10 months ago
No longer depends on: 1313130
I can verify this has been fixed. Only issue found was intermittent bug 1313134.
Flags: needinfo?(jwilliams)
Status: RESOLVED → VERIFIED

Updated

9 months ago
Depends on: 1318915
See Also: → bug 1326022

Updated

8 months ago
Depends on: 1328557
Based on comment 54, I will update the flags.
status-firefox52: fixed → verified
Flags: qe-verify+
Depends on: 1350243
Depends on: 1352620
You need to log in before you can comment on or make changes to this bug.