837396 - Work - Show the Firefox Start page when the user taps/clicks the URL bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. Load a web page in Metro Firefox.
2. Display the toolbars, and tap/click in the location bar.
3. Type something.  The autocomplete popup should appear.
4. Delete what you typed.

Expected: The autocomplete popup disappears and you see the web page again.
Actual:   The autocomplete popup disappears and you see the Firefox start page.

This happens because StartUI.filter calls StartUI.show, but StartUI.unfilter fails to call StartUI.hide, so it leaves the start-container expanded.

We can avoid this problem in a familiar-looking way.  This is split out of the work I was doing on bug 836969.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: 1/3: Rename bcast_changedViewState to bcast_windowState

This is just setting the stage.  I want to use a single broadcaster for several attributes that all affect the layout of the main UI, so I'm renaming it from bcast_changedViewState to bcast_windowState.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: 2/2: Broadcast the "filtering" and "startpage" states

This separates the state of the startUI into two broadcasted attributes, one for the start page ("startpage") and one for the autocomplete popup ("filtering").

When either of these attributes is set, then we should show the start-container and hide the browser.  When filtering is set, then we show the autocomplete popup and hide the start page.

StartUI.show/hide now act only on the start page; they are no longer overloaded to affect the autocomplete popup.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: 2/2: Broadcast the "filtering" and "startpage" states (v2)

I had removed the visible="true" from #tray in browser.xul, because on startup it would be out of sync with the ContextUI._visible variable, leaving the tray in an un-dismissable state.

However, this caused the tray to animate in when the start page was displayed on startup, which was janky and distracting.  It's better to keep the tray visible="true" on startup but make sure ContextUI has the right initial state.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: 2/2: Broadcast the "filtering" and "startpage" states (v3)

Fixed a minor layout bug, and simplified the patch a bit (you can ignore comment 3 now; I just dropped the changes related to that).  Sorry for the churn.

Comment 5

[Allison Naaktgeboren :ally]

rhoh, I can't reproduce the original behavior. Pulling new tree.

Comment 6

[Matt Brubeck (:mbrubeck)]

Actually, based on item (4.2) from attachment 709174 [details] (bug 831899), maybe we *should* show the start page in this situation, and the bug is that we *don't* show it when you first tap in the URL bar.

Comment 7

[Matt Brubeck (:mbrubeck)]

Changing the bug summary to

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: 2/3: Broadcast the "filtering" and "startpage" states (v4)

...match the requirement from story bug 831899.  (Sorry, submitted too soon.)

Minor change to this patch to support this change.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Show the start page when the URL bar is focused

This brings us into compliance with the story as currently spec'ed:

* Show the start page automatically when the user taps the URL bar (BrowserUI._editURI).

* Hide it when the user presses escape while typing in the URL bar (BrowserUI.handleEscape) or navigates to a page (BrowserUI.showContent).

We want to make sure to hide the start page *immediately* when the user chooses a page to navigate to (rather than wait for the new page to load, which leads to a weird delay where the start page is still there for a second after the autocomplete popup goes away).  So we pass the URL that's *about* to load from goToURI to showContent to StartUI, so it can show/hide immediately based on the new page URL (rather than the current one).

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Show the start page when the URL bar is focused (v2)

Restore a line I accidentally changed, and add another comment.

Comment 11

[Matt Brubeck (:mbrubeck)]

Note: The latest patches are built on top of the one from bug 838222, so if you want to test at home you will need to import that patch first.

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 709375 [details] [diff] [review]
1/3: Rename bcast_changedViewState to bcast_windowState

looks good

Comment 13

[Allison Naaktgeboren :ally]

Comment on attachment 709387 [details] [diff] [review]
2/2: Broadcast the "filtering" and "startpage" states (v3)

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

- nit: you'll need to update the commit as this is 2/3 :)

- I'm uneasy about adding filtering & tabsonly to the broadcaster. One one hand, it does make for less code, on the other, I worry about leaky abstractions and overloading coming back to bite us. So, I'd like to hear your thinking on the topic a bit more (either vidyo, irc, or here is fine. Whichever is best for you)

- I'm also generally concerned about tests/testing for this, as we're 'sneaking' a bit of a refactor that could regress things into this bug. That said, I'm not sure a good automated one is possible or within scope right now.

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 710227 [details] [diff] [review]
2/3: Broadcast the "filtering" and "startpage" states (v4)

bah, commented on old patch. sorry about that

Comment 15

[Allison Naaktgeboren :ally]

Comment on attachment 710242 [details] [diff] [review]
3/3: Show the start page when the URL bar is focused (v2)

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

this part might be reasonable to write a test for :)

r+* - pending discussion on part 2

Comment 16

[Allison Naaktgeboren :ally]

Comment on attachment 710227 [details] [diff] [review]
2/3: Broadcast the "filtering" and "startpage" states (v4)

talked offline.

Comment 17

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/projects/elm/rev/5a538865f0ef
http://hg.mozilla.org/projects/elm/rev/e6004e812395
http://hg.mozilla.org/projects/elm/rev/d27d4944ea92

I'll work on some tests tomorrow.