Closed Bug 1296638 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: qawanted)

Attachments

(4 files, 1 obsolete file)

Bug 1296638 - Make Form Autocomplete popup use a richlistbox instead of a tree.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Testing form
231 bytes, text/html
Details
Bug 1296638 - AutoCompletePopup in browser-content.js and AutoCompletePopup.jsm need to have popupOpen be in sync.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1296638 - Adjust autocomplete test helpers to account for the new richlistbox implementation.
58 bytes, text/x-review-board-request
MattN
: review+
Details 
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
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 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 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: nobody → mconley
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)
(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.
Attached file Testing form — 

    
  

        Attachment #8787246 -
        Attachment mime type: text/plain → text/html

    
    

    
  
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)

    
    

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

Outstanding, thank you!

    
  
Blocks: 1300995

    
  
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk

    
  
Blocks: 1304224

    
  
Blocks: 1217162

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.

    
    

    
  
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.

    
    

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

    
  
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)

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8793895 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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+

    
    

    
  
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

    
    

    
  
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)

    
    

    
  
I filed bug 1310942 for comment 47, I'm leaving the needinfo just as an heads up.

    
  
Depends on: 1311189

    
  
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)

    
  
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.

    
  
Depends on: 1313067
Depends on: 1313130
Depends on: 1313131
Depends on: 1313132
Depends on: 1313134
No longer depends on: 1313132
No longer depends on: 1313131
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

    
  
Depends on: 1318915

    
  
Depends on: 1328557

    
  
Depends on: 1350243

    
  
Depends on: 1352620

    
  
Regressions: 1542994

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: