Work - NewUI - Implement new half-height autocomplete popup

RESOLVED FIXED

Status

Firefox for Metro
App Bar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwilde, Assigned: jwilde)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(8 attachments, 15 obsolete attachments)

52.50 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
3.47 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.48 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
1.76 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
10.33 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.92 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
2.59 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
2.98 KB, patch
jimm
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
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."
(Assignee)

Updated

5 years ago
Whiteboard: feature=work
Blocks: 831910
(Assignee)

Comment 2

5 years ago
Created attachment 764467 [details] [diff] [review]
patch WIP0
(Assignee)

Comment 3

5 years ago
Created attachment 765012 [details] [diff] [review]
patch WIP1
(Assignee)

Updated

5 years ago
Attachment #765012 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Attachment #764467 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 765148 [details] [diff] [review]
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.
Attachment #765148 - Flags: feedback?(fyan)
(Assignee)

Updated

5 years ago
Attachment #765148 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Attachment #765012 - Attachment is obsolete: true

Comment 5

5 years ago
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. :)
Attachment #765148 - Flags: feedback?(fyan) → feedback+
(Assignee)

Updated

5 years ago
Whiteboard: feature=work → [leave open] feature=work
(Assignee)

Comment 6

5 years ago
Created attachment 769118 [details] [diff] [review]
autocomplete-part1-v1
Attachment #765148 - Attachment is obsolete: true
Attachment #769118 - Flags: review?(fyan)
(Assignee)

Comment 7

5 years ago
Created attachment 769121 [details] [diff] [review]
patch for part 1, v1.1

dropping blank spaces
Attachment #769118 - Attachment is obsolete: true
Attachment #769118 - Flags: review?(fyan)
Attachment #769121 - Flags: review?(fyan)
(Assignee)

Comment 8

5 years ago
Created attachment 769208 [details] [diff] [review]
patch for part 1, v1.2

Fixed bitrot from patch I just dumped on inbound.
Attachment #769121 - Attachment is obsolete: true
Attachment #769121 - Flags: review?(fyan)
Attachment #769208 - Flags: review?(fyan)
(Assignee)

Comment 9

5 years ago
Created attachment 769276 [details] [diff] [review]
autocomplete-part1-v1.3

Fixing the hamburger button.
Attachment #769208 - Attachment is obsolete: true
Attachment #769208 - Flags: review?(fyan)
(Assignee)

Updated

5 years ago
Attachment #769276 - Flags: review?(fyan)

Comment 10

5 years ago
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.
Attachment #769276 - Flags: review?(fyan) → review-

Updated

5 years ago
Depends on: 887120
(Assignee)

Updated

5 years ago
Depends on: 891213
(Assignee)

Comment 11

5 years ago
Created attachment 772470 [details] [diff] [review]
Part 1 - Moving autocomplete into half-height overlay, overall refactor.
Attachment #769276 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #772470 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 773077 [details] [diff] [review]
part1 - implement overlay
(Assignee)

Comment 13

5 years ago
Created attachment 773079 [details] [diff] [review]
part1.1 - ensure browser search engines update
(Assignee)

Comment 14

5 years ago
Created attachment 773081 [details] [diff] [review]
part1.2 - fix for 887120
(Assignee)

Comment 15

5 years ago
Created attachment 773082 [details] [diff] [review]
part2 - initial tests
(Assignee)

Updated

5 years ago
Attachment #773082 - Attachment is patch: true
Attachment #773082 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 16

5 years ago
Created attachment 773476 [details] [diff] [review]
part1 - implement overlay, centralize urlbar code
Attachment #773077 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #773476 - Flags: review?(sfoster)
(Assignee)

Updated

5 years ago
Attachment #773476 - Attachment description: part1 - implement overlay autocomplete → part1 - implement overlay, centralize urlbar code
(Assignee)

Comment 17

5 years ago
Created attachment 773484 [details] [diff] [review]
part1.1 - defect - browser search engines don't update
Attachment #773079 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #773484 - Flags: review?(sfoster)
(Assignee)

Comment 18

5 years ago
Created attachment 773489 [details] [diff] [review]
part1.2 - fix for 887120
Attachment #773081 - Attachment is obsolete: true
Attachment #773489 - Flags: review?(jmathies)
(Assignee)

Updated

5 years ago
Attachment #773082 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 773492 [details] [diff] [review]
part1.3 - move unrelated code out of navigation section
(Assignee)

Updated

5 years ago
Attachment #773492 - Flags: review?(sfoster)
(Assignee)

Comment 20

5 years ago
Created attachment 773499 [details] [diff] [review]
part1.4 - update urlbar more intelligently, use tasks in doOpenSearch like we do in goToURI
Attachment #773499 - Flags: review?(sfoster)

Comment 21

5 years ago
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.

Updated

5 years ago
Attachment #773492 - Flags: review?(sfoster) → review+

Comment 22

5 years ago
I think part1 needs to merge to tip.

Updated

5 years ago
Attachment #773489 - Flags: review?(jmathies) → review+
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
Attachment #773476 - Flags: review?(sfoster) → review+
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
Attachment #773484 - Flags: review?(sfoster) → review+
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.
Attachment #773499 - Flags: review?(sfoster) → review+
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
(Assignee)

Comment 27

5 years ago
Created attachment 774245 [details] [diff] [review]
part1.1 - defect - browser search engines don't update

The previous patch only fixed /adding/ engines. This one now handles removing them, too.
Attachment #773484 - Attachment is obsolete: true
Attachment #774245 - Flags: review?(sfoster)
(Assignee)

Comment 28

5 years ago
Created attachment 774250 [details] [diff] [review]
part 2 - some initial tests

This is just a basic set of tests to verify functionality. More coverage to be added in followups.
Attachment #774250 - Flags: review?(jmathies)
(Assignee)

Updated

5 years ago
Blocks: 892240
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.
Attachment #774245 - Flags: review?(sfoster) → review+
(Assignee)

Comment 30

5 years ago
> 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

5 years ago
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?
(Assignee)

Comment 32

5 years ago
(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

5 years ago
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.
Attachment #774250 - Flags: review?(jmathies) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [leave open] feature=work → feature=work
(Assignee)

Comment 34

5 years ago
Created attachment 774422 [details] [diff] [review]
part1.5 - cleanup of urlbar.xml, using single-selection to simplify things

one more patch!
Attachment #774422 - Flags: review?(sfoster)
(Assignee)

Updated

5 years ago
Attachment #774422 - Attachment is patch: true
Attachment #774422 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Updated

5 years ago
Attachment #774422 - Attachment is obsolete: true
Attachment #774422 - Flags: review?(sfoster)
(Assignee)

Comment 35

5 years ago
Created attachment 774894 [details] [diff] [review]
part1.5 - cleanup of urlbar.xml
Attachment #774894 - Flags: review?(sfoster)
(Assignee)

Comment 36

5 years ago
Created attachment 774896 [details] [diff] [review]
part1.6 - use grid.xml to manage mouse/touch selection
Attachment #774896 - Flags: review?(sfoster)
(Assignee)

Comment 37

5 years ago
Created attachment 774972 [details] [diff] [review]
part1.2 - fix for 887120

Switching over to SpecialPowers function for copying text to clipboard.
Attachment #773489 - Attachment is obsolete: true
Attachment #774972 - Flags: review?(jmathies)

Updated

5 years ago
Attachment #774972 - Flags: review?(jmathies) → review+
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
Attachment #774894 - Flags: review?(sfoster) → review+
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.
Attachment #774896 - Flags: review?(sfoster) → review+
Please land on fx-team now (see newsgroup post) - this unfortunately caused conflicts with metro landings on fx-team. Thank you :-)
(Assignee)

Comment 43

5 years ago
Sorry about that. >.<
Depends on: 904119
You need to log in before you can comment on or make changes to this bug.