Closed Bug 883390 Opened 11 years ago Closed 11 years ago

Work - NewUI - Implement new half-height autocomplete popup

Categories

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

Product:
Firefox for Metro Graveyard
Component:
App Bar
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jwilde, Assigned: jwilde)

References

Details

(Whiteboard: feature=work)

Attachments

(8 files, 15 obsolete files)

part1 - implement overlay, centralize urlbar code
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
part1.3 - move unrelated code out of navigation section
(deleted), patch
jimm
: review+
Details | Diff | Splinter Review
part1.4 - update urlbar more intelligently, use tasks in doOpenSearch like we do in goToURI
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
part1.1 - defect - browser search engines don't update
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
part 2 - some initial tests
(deleted), patch
jimm
: review+
Details | Diff | Splinter Review
part1.5 - cleanup of urlbar.xml
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
part1.6 - use grid.xml to manage mouse/touch selection
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
part1.2 - fix for 887120
(deleted), patch
jimm
: review+
Details | Diff | Splinter Review
No description provided.
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."
Whiteboard: feature=work
Blocks: 831910
Attached patch patch WIP0 (obsolete) (deleted) — — Splinter Review
Attached patch patch WIP1 (obsolete) (deleted) — — Splinter Review
Attachment #765012 - Attachment is patch: true
Attachment #764467 - Attachment is obsolete: true
Attached patch patch WIP2 (obsolete) (deleted) — — Splinter Review
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)
Attachment #765148 - Attachment is patch: true
Attachment #765012 - Attachment is obsolete: true
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+
Whiteboard: feature=work → [leave open] feature=work
Attached patch autocomplete-part1-v1 (obsolete) (deleted) — — Splinter Review
Attachment #765148 - Attachment is obsolete: true
Attachment #769118 - Flags: review?(fyan)
Attached patch patch for part 1, v1.1 (obsolete) (deleted) — — Splinter Review
dropping blank spaces
Attachment #769118 - Attachment is obsolete: true
Attachment #769118 - Flags: review?(fyan)
Attachment #769121 - Flags: review?(fyan)
Attached patch patch for part 1, v1.2 (obsolete) (deleted) — — Splinter Review
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)
Attached patch autocomplete-part1-v1.3 (obsolete) (deleted) — — Splinter Review
Fixing the hamburger button.
Attachment #769208 - Attachment is obsolete: true
Attachment #769208 - Flags: review?(fyan)
Attachment #769276 - Flags: review?(fyan)
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-
Depends on: 887120
Depends on: 891213
Attachment #769276 - Attachment is obsolete: true
Attachment #772470 - Attachment is obsolete: true
Attached patch part1 - implement overlay (obsolete) (deleted) — — Splinter Review
Attached patch part1.1 - ensure browser search engines update (obsolete) (deleted) — — Splinter Review
Attached patch part1.2 - fix for 887120 (obsolete) (deleted) — — Splinter Review
Attached patch part2 - initial tests (obsolete) (deleted) — — Splinter Review
Attachment #773082 - Attachment is patch: true
Attachment #773082 - Attachment mime type: text/x-patch → text/plain
Attachment #773077 - Attachment is obsolete: true
Attachment #773476 - Flags: review?(sfoster)
Attachment #773476 - Attachment description: part1 - implement overlay autocomplete → part1 - implement overlay, centralize urlbar code
Attachment #773079 - Attachment is obsolete: true
Attachment #773484 - Flags: review?(sfoster)
Attached patch part1.2 - fix for 887120 (obsolete) (deleted) — — Splinter Review
Attachment #773081 - Attachment is obsolete: true
Attachment #773489 - Flags: review?(jmathies)
Attachment #773082 - Attachment is obsolete: true
Attachment #773492 - Flags: review?(sfoster)
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.
Attachment #773492 - Flags: review?(sfoster) → review+
I think part1 needs to merge to tip.
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
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)
Attached patch part 2 - some initial tests (deleted) — — Splinter Review
This is just a basic set of tests to verify functionality. More coverage to be added in followups.
Attachment #774250 - Flags: review?(jmathies)
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+
> 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 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?
(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 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+
Whiteboard: [leave open] feature=work → feature=work
one more patch!
Attachment #774422 - Flags: review?(sfoster)
Attachment #774422 - Attachment is patch: true
Attachment #774422 - Attachment mime type: text/x-patch → text/plain
Attachment #774422 - Attachment is obsolete: true
Attachment #774422 - Flags: review?(sfoster)
Attached patch part1.5 - cleanup of urlbar.xml (deleted) — — Splinter Review
Attachment #774894 - Flags: review?(sfoster)
Attachment #774896 - Flags: review?(sfoster)
Attached patch part1.2 - fix for 887120 (deleted) — — Splinter Review
Switching over to SpecialPowers function for copying text to clipboard.
Attachment #773489 - Attachment is obsolete: true
Attachment #774972 - Flags: review?(jmathies)
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 :-)
Sorry about that. >.<
Depends on: 904119
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: