883390 - Work - NewUI - Implement new half-height autocomplete popup

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: App Bar, defect)

Comment 1

[Jonathan Wilde [:jwilde]]

Yuan's mockups have a new half-height autocomplete popup. This should be implemented. On top of this, there's some architectural changes that should happen to the existing autocomplete XBL bindings, as discussed by fryn in review comments on bug 811413:

"It seems to me that all the code in browser-ui.js should be moved into
autocomplete.xml, especially since we keep referring to this._edit in
browser-ui.js, which is the element to which autocomplete.xml is bound.
To reflect more clearly that autocomplete.xml is a singleton only used for the
urlbar, we could rename the file and the binding ID."

Comment 2

[Jonathan Wilde [:jwilde]]

Attachment: patch WIP0

Comment 3

[Jonathan Wilde [:jwilde]]

Attachment: patch WIP1

Comment 4

[Jonathan Wilde [:jwilde]]

Attachment: patch WIP2

No longer breaks the urlbar tests. This should work on m-I or m-c with the patches for bug 873251. Please ignore all of the added whitespace.

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 765148 [details] [diff] [review]
patch WIP2

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

Since this bitrotted, I'll do a more thorough review on the next version, but it looks good so far. :)

Comment 6

[Jonathan Wilde [:jwilde]]

Attachment: autocomplete-part1-v1

Comment 7

[Jonathan Wilde [:jwilde]]

Attachment: patch for part 1, v1.1

dropping blank spaces

Comment 8

[Jonathan Wilde [:jwilde]]

Attachment: patch for part 1, v1.2

Fixed bitrot from patch I just dumped on inbound.

Comment 9

[Jonathan Wilde [:jwilde]]

Attachment: autocomplete-part1-v1.3

Fixing the hamburger button.

Comment 10

[Frank Yan (:fryn)]

Comment on attachment 769276 [details] [diff] [review]
autocomplete-part1-v1.3

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

With this patch, tapping/clicking an Internet Searches tile selects it rather than navigates.

Comment 11

[Jonathan Wilde [:jwilde]]

Attachment: Part 1 - Moving autocomplete into half-height overlay, overall refactor.

Comment 12

[Jonathan Wilde [:jwilde]]

Attachment: part1 - implement overlay

Comment 13

[Jonathan Wilde [:jwilde]]

Attachment: part1.1 - ensure browser search engines update

Comment 14

[Jonathan Wilde [:jwilde]]

Attachment: part1.2 - fix for 887120

Comment 15

[Jonathan Wilde [:jwilde]]

Attachment: part2 - initial tests

Comment 16

[Jonathan Wilde [:jwilde]]

Attachment: part1 - implement overlay, centralize urlbar code

Comment 17

[Jonathan Wilde [:jwilde]]

Attachment: part1.1 - defect - browser search engines don't update

Comment 18

[Jonathan Wilde [:jwilde]]

Attachment: part1.2 - fix for 887120

Comment 19

[Jonathan Wilde [:jwilde]]

Attachment: part1.3 - move unrelated code out of navigation section

Comment 20

[Jonathan Wilde [:jwilde]]

Attachment: part1.4 - update urlbar more intelligently, use tasks in doOpenSearch like we do in goToURI

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 773476 [details] [diff] [review]
part1 - implement overlay, centralize urlbar code

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

Enjoy seeing all this split out from browser-ui!

::: browser/metro/base/content/bindings/urlbar.xml
@@ +464,5 @@
> +      <handler event="keypress" phase="capturing">
> +        <![CDATA[
> +          // If the user types in the address bar, cancel the pending
> +          // navbar autohide if set.
> +          ContextUI.cancelDismiss();

note, this handler was removed recently as it wasn't needed. We don't want to cancel tab bar dismissal when the user is editing the navbar. This was leftovers from the combined nav/tab bar logic.

Comment 22

[Jim Mathies [:jimm]]

I think part1 needs to merge to tip.

Comment 23

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 773476 [details] [diff] [review]
part1 - implement overlay, centralize urlbar code

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

Huge props for getting this done. Looks good and seems to work well. r+ with comments

::: browser/metro/base/content/bindings/urlbar.xml
@@ +76,5 @@
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="formatValue">

I know this is a code move and not written by you, but we should have tests for this - I only set a test covering _canonizeURL. In a follow-up is fine but lets file and track it

@@ +510,2 @@
>            Services.obs.addObserver(this, "browser-search-engine-modified", false);
> +

Yay, less race conditions!

@@ +707,3 @@
>          <body>
>            <![CDATA[
> +            // Move between grids if we're at the edge of one.

I cannot vouch for the state of keyboard and selection handling in richgrid today as we've not had tests or use-cases that cover it while its been evolving. Does this work, if so great.

@@ +758,5 @@
> +        <body>
> +          <![CDATA[
> +            // XXX this should probably be supported in grid.xml
> +            // somehow via some single-selection option.
> +            if (this._grid) this._grid.clearSelection();

Agreed. We already use and support a seltype="single|multiple" attribute for the richgrid. You should be able to simplify this by watching for selectionchange instead and just calling submitSelected

Comment 24

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 773484 [details] [diff] [review]
part1.1 - defect - browser search engines don't update

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

::: browser/metro/base/content/bindings/urlbar.xml
@@ +780,4 @@
>                  aData != "engine-changed")
>                return;
>  
> +          this.updateSearchEngineGrid();

nit: indentation

@@ +819,4 @@
>            let grid = event.originalTarget;
>  
>            if (grid == this._searches)
> +          this.initSearchEngines();

nit: indentation

Comment 25

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 773499 [details] [diff] [review]
part1.4 - update urlbar more intelligently, use tasks in doOpenSearch like we do in goToURI

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

Looks good.

Comment 26

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 773499 [details] [diff] [review]
part1.4 - update urlbar more intelligently, use tasks in doOpenSearch like we do in goToURI

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

Note added re: content.focus()

::: browser/metro/base/content/browser-ui.js
@@ +366,4 @@
>      // Make sure we're online before attempting to load
>      Util.forceOnline();
>      BrowserUI.showContent();
> +    content.focus();

Should be Browser.selectedBrowser.focus() per discussion in channel and bug https://bugzilla.mozilla.org/show_bug.cgi?id=691925

Comment 27

[Jonathan Wilde [:jwilde]]

Attachment: part1.1 - defect - browser search engines don't update

The previous patch only fixed /adding/ engines. This one now handles removing them, too.

Comment 28

[Jonathan Wilde [:jwilde]]

Attachment: part 2 - some initial tests

This is just a basic set of tests to verify functionality. More coverage to be added in followups.

Comment 29

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 774245 [details] [diff] [review]
part1.1 - defect - browser search engines don't update

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

Will initSearchEngines get called from outside the binding ever (and can it be safely done without borking things if say it got called at the wrong time?) Seems like we should make this "_initSearchEngines" unless we expect and intend it to be public - no point creating more API surface-area than is necessary. Otherwise, looks good.

Comment 30

[Jonathan Wilde [:jwilde]]

> Will initSearchEngines get called from outside the binding ever (and can it
> be safely done without borking things if say it got called at the wrong
> time?) Seems like we should make this "_initSearchEngines" unless we expect
> and intend it to be public - no point creating more API surface-area than is
> necessary. Otherwise, looks good.

Yes. Will do before landing.

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 774250 [details] [diff] [review]
part 2 - some initial tests

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

Can this run on tip or is it dependent on patches here that haven't landed?

::: browser/metro/base/tests/mochitest/browser_urlbar.js
@@ +130,5 @@
> +    yield addMockSearchDefault();
> +    ok(gEdit.popup._searches.itemCount == numSearches + 1, "added search engine count");
> +    ok(getEngineItem(), "added search engine item");
> +
> +    yield removeMockSearchDefault();

If we fail before this, the engine sticks around. Maybe add calls to this in your tearDowns to be sure we get cleaned up?

Comment 32

[Jonathan Wilde [:jwilde]]

(In reply to Jim Mathies [:jimm] from comment #31)
> Comment on attachment 774250 [details] [diff] [review]
> part 2 - some initial tests
> 
> Review of attachment 774250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can this run on tip or is it dependent on patches here that haven't landed?

Dependent on the part1 patches which haven't landed. Most of the part1.[1-4] patches were often bugfixes to problems found while writing the tests.

> ::: browser/metro/base/tests/mochitest/browser_urlbar.js
> @@ +130,5 @@
> > +    yield addMockSearchDefault();
> > +    ok(gEdit.popup._searches.itemCount == numSearches + 1, "added search engine count");
> > +    ok(getEngineItem(), "added search engine item");
> > +
> > +    yield removeMockSearchDefault();
> 
> If we fail before this, the engine sticks around. Maybe add calls to this in
> your tearDowns to be sure we get cleaned up?

Will add.

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 774250 [details] [diff] [review]
part 2 - some initial tests

I didn't have any luck getting your set to apply, but generally the logic here looks good.

Comment 34

[Jonathan Wilde [:jwilde]]

Attachment: part1.5 - cleanup of urlbar.xml, using single-selection to simplify things

one more patch!

Comment 35

[Jonathan Wilde [:jwilde]]

Attachment: part1.5 - cleanup of urlbar.xml

Comment 36

[Jonathan Wilde [:jwilde]]

Attachment: part1.6 - use grid.xml to manage mouse/touch selection

Comment 37

[Jonathan Wilde [:jwilde]]

Attachment: part1.2 - fix for 887120

Switching over to SpecialPowers function for copying text to clipboard.

Comment 38

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 774894 [details] [diff] [review]
part1.5 - cleanup of urlbar.xml

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

Looks good, r+ with comments

::: browser/metro/base/content/bindings/urlbar.xml
@@ +605,5 @@
> +      <method name="_isGridBound">
> +        <parameter name="aGrid"/>
> +        <body>
> +          <![CDATA[
> +            return aGrid && aGrid.itemCount != undefined;

The richgrid binding now provides a isBound property that you can test e.g. "isBound" in aGrid && aGrid.isBound or similar.

@@ +674,4 @@
>        <method name="_initSearchEngines">
>          <body>
>            <![CDATA[
> +            Services.search.init(this.updateSearchEngineGrid.bind(this));

nit: indentation

Comment 39

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 774896 [details] [diff] [review]
part1.6 - use grid.xml to manage mouse/touch selection

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

Good catch.

Comment 40

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9bb67dba984
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a9f741253c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9434d96e85b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c112dc25fa8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/79e6b9d02f22
https://hg.mozilla.org/integration/mozilla-inbound/rev/45a9aeb25008
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b11d90b294c
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e84638eb6d

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e9bb67dba984
https://hg.mozilla.org/mozilla-central/rev/47a9f741253c
https://hg.mozilla.org/mozilla-central/rev/9434d96e85b5
https://hg.mozilla.org/mozilla-central/rev/c112dc25fa8c
https://hg.mozilla.org/mozilla-central/rev/79e6b9d02f22
https://hg.mozilla.org/mozilla-central/rev/45a9aeb25008
https://hg.mozilla.org/mozilla-central/rev/5b11d90b294c
https://hg.mozilla.org/mozilla-central/rev/38e84638eb6d

Comment 42

[Ed Morley [:emorley]]

Please land on fx-team now (see newsgroup post) - this unfortunately caused conflicts with metro landings on fx-team. Thank you :-)

Comment 43

[Jonathan Wilde [:jwilde]]

Sorry about that. >.<