1022464 - Returning to URL bar containing search terms doesn't re-enter search mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Richard Newman [:rnewman]]

Type search terms. Hit enter. Search page loads. 

Tap url bar. Search terms, not url, are pre filled. That's correct. 

Expected: visible are suggestions and engines.

Actual: history is visible until you type another character.

Comment 1

[:Margaret Leibovic]

Sounds like a long overdue follow-up to bug 823230.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Given we were looking at search recently this is something that I run into often. It is annoying to need to edit the search term to bring up the search engine view.

Comment 3

[:Margaret Leibovic]

I'll try writing a patch for this. This could fit in well with the recent search suggestion/history work we've been doing.

Comment 4

[:Margaret Leibovic]

I wrote a patch for this, but this brings up a tricky UX concern...

If we automatically enter the search suggestion UI as soon as the user hits the urlbar, there's no way for them to get back to their home panels, other than opening a new tab (or navigating to a URL that isn't a search term, which makes this even more weird).

I do agree that it feels nicer to get back to editing your current term, but maybe this is a sign that we should just always immediately enter the awesomescreen UI when the user taps on the urlbar, and only show the home panels when they create a new tab. Basically, this would make the UX a lot more like desktop (and like Chrome).

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #4)
> I wrote a patch for this, but this brings up a tricky UX concern...
> 
> If we automatically enter the search suggestion UI as soon as the user hits
> the urlbar, there's no way for them to get back to their home panels, other
> than opening a new tab (or navigating to a URL that isn't a search term,
> which makes this even more weird).
> 
> I do agree that it feels nicer to get back to editing your current term, but
> maybe this is a sign that we should just always immediately enter the
> awesomescreen UI when the user taps on the urlbar, and only show the home
> panels when they create a new tab. Basically, this would make the UX a lot
> more like desktop (and like Chrome).

This is something I've been wanting to spend more time on for a while now. As someone coming over from chrome or another browser, this interaction (a very high visibility one) is quite startling - because it's different.

We made a note to chat about this in person this week.

To summarize, we're talking about triggering the awesomescreen here (while user entered search terms are in the awesomebar) and NOT about:home.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

NI-ing liuche here since we worked on this in mozlando (moving over from bug 1230587)

Comment 8

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mfinkle

Comment 9

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/27829/#review24987

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2158
(Diff revision 1)
> +                showBrowserSearch();

I don't like this inconsistency. If we're going to go down this route, I think we should always show the BrowserSearch UI instead of the home panels.

I also think we should file a separate bug to do the search URL reverse-parsing as a fallback for the case when we don't have a saved userRequested term (e.g. session restore, "next" search results page).

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8698137 [details]
MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

Going to play around with some transparency in the interstitial space really quick.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #9)
> https://reviewboard.mozilla.org/r/27829/#review24987
> 
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2158
> (Diff revision 1)
> > +                showBrowserSearch();
> 
> I don't like this inconsistency. If we're going to go down this route, I
> think we should always show the BrowserSearch UI instead of the home panels.

I don't think it's as clear-cut as that. Telemetry tells us that around 33% of Top Sites/History/Bookmark URLs are loaded from the Awesomescreen entry point. This means that a non-trivial amount of people are using the Home Panels from the Awesomescreen.

Moving into BrowserSearch when we have search terms is a more conservative approach for now.

> I also think we should file a separate bug to do the search URL
> reverse-parsing as a fallback for the case when we don't have a saved
> userRequested term (e.g. session restore, "next" search results page).

It would be nice if the reverse-parsing approach became the only approach and we could drop userRequested, which was never a great solution.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hm, I can see how this might seem like an inconsistency at first. But if we break down the experience, it actually makes more sense I think. 

For example, let's consider the use case when a user is on any give web page. This could be a site like "mozilla.org", or it could be a search results page. For the latter, we already try to help the user by just showing the search terms rather than a long and confusing URL. This presumably makes it easier for them to revise their search.

But I think the key context that we forgot is that the user is being shown the Awesome screen (with suggestions, frecency, and quick search) every step of the way while they're typing until they "submit" the search. To remove this but keep the search terms in the URL bar seems disjointed and it's further disconnected when we juxtapose our Top Sites/ Home panels UI in the same view.

This proposed experience puts our user into Home panels w/keyboard up still _OR_ into our Awesomescreen (exactly the way it was before they "submitted" their search). This gives them the ability to pick up exactly where they were before they got to the results page.

Otherwise (with the current experience), they have to tap the URL bar, un-select the search terms, move the cursor to the end (or where they want to type), then move their hands back down to the keyboard and hit space bar/ another character, then submit a revised search.

tl;dr

if "coffee vancouver" is in URL bar > tap URL bar > see awesomescreen
if browsing URLis in URL bar > tap URL bar > see Home panels

This creates a clearer separation I think. It's also something that's more evident in practice so maybe a build would exemplify this better. :)

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8698137 [details]
MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27829/diff/1-2/

Comment 14

[Chenxia Liu [:liuche]]

Talked to mcomella, and fixed this patch to preserve our no-overdraw conventions. However, this gets pretty messy because we've got a lot of different paths for showing/hiding each of webcontent, homepager, and awesomescreen, and we do the transitions at each step. (Filed bug 1232508 to handle these mutually exclusive states globally, instead of at each transition.)

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Screenshot: transparent spacer

This doesn't fully work, but is this what you had in mind? This is the same transparency color that we use elsewhere.

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8698301 [details]
Screenshot: transparent spacer

Awesome!

This is in the direction of what I was thinking. Essentially making this more "modal" in nature. But, we'd need the web content to also come through in that dark grey section.

But, I don't think we should block on this. Let's file that under our polish meta :)

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8698137 [details]
MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

https://reviewboard.mozilla.org/r/27829/#review25289

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2157
(Diff revision 2)
> +                url = userSearchTerm;

nit: You could simplify this by only having one call to telemetry, e.g.

`final String telemetryExtra;`
`if (...isEmpty...) {`
  `url = ...`
  `telemetryExtra = ...`
` } ...`
` }`
` Telemetry.sendUIEvent(...`

über-nit: `url =` should be above/below telemetry in both branches for consistency.

Sorry. :P

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2200
(Diff revision 2)
> +        final boolean isUserSearchTerm = url.equals(selectedTab.getURL()) ? false : true;

nit: Assign the return value of `String.equals` directly to the variable, without the ternary operator.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2200
(Diff revision 2)
> +        final boolean isUserSearchTerm = url.equals(selectedTab.getURL()) ? false : true;

I don't think `url.equals(selectedTab.getURL())` dictates a user-entered search – in theory, anything can be passed into `enterEditingMode(String)`. How about `Tab.getUserRequested`? We should probably make sure it's not empty first (as in `enterEditingMode()`). It's a little sketch that we checke against user requested in `enterEditingMode()` and again here but I'm unclear how to fix it.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2208
(Diff revision 2)
> +        hideWebContentOnPropertyAnimationEnd(animator);

There's a similar, unlanded solution in bug 1223586, expected with the animationEnd – I'm haven't looked deep enough into this to be sure what's more appropriate here but we should probably make sure we don't call it twice.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2400
(Diff revision 2)
> +            hideWebContent();

Should we just do this in `showHomePager`?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 2)
> -        hideWebContentOnPropertyAnimationEnd(animator);

Why do we want to remove this? Don't we want to hide the web content after the animation ends?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2618
(Diff revision 2)
> +    private void showBrowserSearch(PropertyAnimator animator) {

nit: -> `showBrowserSearchAfterAnimation` or similar

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2680
(Diff revision 2)
>          mHomePagerContainer.setVisibility(View.VISIBLE);

`showHomePager` (which you added) calls `mHomePagerContainer.setVisibility` so you can remove this line.

Seems to do the job but first a few technicality questions!

Comment 18

[:Margaret Leibovic]

Let's land this behind a Nightly flag to see how it feels in practice, without committing to shipping it.

Comment 19

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/27829/#review25289

> There's a similar, unlanded solution in bug 1223586, expected with the animationEnd – I'm haven't looked deep enough into this to be sure what's more appropriate here but we should probably make sure we don't call it twice.

Hm, that's a good point - I moved the hideWebContentOnAnimationEnd to be added at the same time as the hideBrowserSearchAfterAnimation, so it won't get called twice.

> Should we just do this in `showHomePager`?

Yeah, that makes sense - this means I also have to add it to showBrowserSearch, but that makes sense. This code really should just be in a state machine form, alas :/

> Why do we want to remove this? Don't we want to hide the web content after the animation ends?

I removed this because it's only called in one place, but in hindsight it does make sense to keep it here and just add it to the showSearch code.

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8698137 [details]
MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27829/diff/2-3/

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8698137 [details]
MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

https://reviewboard.mozilla.org/r/27829/#review25925

Looks mostly correct but I'd like answers to a few of these questions before landing.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2167
(Diff revision 3)
> -            url = (TextUtils.isEmpty(userRequested) ? tab.getURL() : userRequested);
> +            String telemetryMsg;

nit: `final String`

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2176
(Diff revision 3)
> +            Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.ACTIONBAR, telemetryMsg);

Are we sure we want `Event.SEARCH` here? We may be loading a url here (though I see we have that in the extra `"urlbar-url"`).

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2214
(Diff revision 3)
> +            hideWebContentOnPropertyAnimationEnd(animator);

Why is this necessary? showBrowserSearch calls `hideWebContent` and it's set up to be called at the end of the animation.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2696
(Diff revision 3)
> -        mHomePagerContainer.setVisibility(View.VISIBLE);
> +        showHomePager(Tabs.getInstance().getSelectedTab().getMostRecentHomePanel());

Why did you make this change? I imagine it's "more correct" though the method might written with the assumptions that it's being called from web content.

It's also not symmetric – `showBrowserSearch` calls `mHomePagerContainer.setVisibility`, not `hideHomePager`.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2699
(Diff revision 3)
> +        mHideWebContentOnAnimationEnd = false;

Why is this necessary?

Comment 22

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/27829/#review25957

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2176
(Diff revision 3)
> +            Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.ACTIONBAR, telemetryMsg);

I agree. I don't think we want Event.SEARCH here, since we go through here when entering edit mode, not when actually conducting a search.

Comment 23

[:Margaret Leibovic]

What's the worst that could happen if we land this (with the telemetry probe comment addressed) and let it bake on Nightly a bit before Chenxia comes back and answers these questions?

I suppose the changes to `hideBrowserSearch` might be a bit worrisome, since they're not behind a Nightly only flag. But I want to gt this change in front of Nightly users (aka us) to see what they think.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Margaret Leibovic from comment #23)
> What's the worst that could happen if we land this (with the telemetry probe
> comment addressed) and let it bake on Nightly a bit before Chenxia comes
> back and answers these questions?

Looking at my comments, I think it'd be fine to land if I undo those complex added bits in hide/showing BrowserSearch and address the telemetry comment – I can take care of finishing this.

Margaret, what event should the telemetry probe use?

Comment 25

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #24)
> (In reply to :Margaret Leibovic from comment #23)
> > What's the worst that could happen if we land this (with the telemetry probe
> > comment addressed) and let it bake on Nightly a bit before Chenxia comes
> > back and answers these questions?
> 
> Looking at my comments, I think it'd be fine to land if I undo those complex
> added bits in hide/showing BrowserSearch and address the telemetry comment –
> I can take care of finishing this.
> 
> Margaret, what event should the telemetry probe use?

How about (Event.SHOW, Method.ACTIONBAR, "editmode-userEntered"/"editmode-empty"/"editmode-url")?

https://gecko.readthedocs.org/en/latest/mobile/android/fennec/uitelemetry.html#id1

NI to mfinkle for a sanity check.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

https://reviewboard.mozilla.org/r/27829/#review26177

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2176
(Diff revision 3)
> +            Telemetry.sendUIEvent(TelemetryContract.Event.SEARCH, TelemetryContract.Method.ACTIONBAR, telemetryMsg);

Yes. Event.SHOW is much better. I would also suggest "urlbar-userentered" (no camel case).

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Manually clearing NI flag. Thanks, MozReview!

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/27829/#review25925

> Are we sure we want `Event.SEARCH` here? We may be loading a url here (though I see we have that in the extra `"urlbar-url"`).

I went with Finkle's suggestion of `urlbar-*`.

> Why is this necessary? showBrowserSearch calls `hideWebContent` and it's set up to be called at the end of the animation.

Removed `hideWebContentOnPropertyAnimationEnd` call.

> Why did you make this change? I imagine it's "more correct" though the method might written with the assumptions that it's being called from web content.
> 
> It's also not symmetric – `showBrowserSearch` calls `mHomePagerContainer.setVisibility`, not `hideHomePager`.

Upon testing, it appears this method is necessary – I didn't make any changes here.

> Why is this necessary?

Removed this.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: review: Make changes for review comments while liuche is out

There is one review comment questioning a change that, upon testing, seems to
be necessary upon testing - I didn't make any changes for that change but I
still don't entirely understand why it's necessary.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/69197595f2838a89abb1e969801242797737a038
Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/33a02b110c346b863830f22dc61b7727c4c1e102
Bug 1022464 - review: Make changes for review comments while liuche is out. r=me

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I landed the changes that worked locally, but NI liuche to double-check my changes and ensure they make sense.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69197595f283
https://hg.mozilla.org/mozilla-central/rev/33a02b110c34

Comment 33

[Chenxia Liu [:liuche]]

These changes are fine - the hideWebContent changes were to try to handle the race condition referenced by the comment for mHideWebContentOnAnimation, but you're right that we call hideWebContent anyways so they were unnecessary. I'll keep an eye out for the race condition happening (though the web/homepager/search state code is pretty fragile and extremely complex) :/

Thanks for pushing this through, mcomella!

Comment 34

[Flaviu Cos]

Verified as fixed in builds:
- 46 Beta 6;
- 47.0a2 2016-03-29;
- 48.0a1 2016-03-29.
Device: Nexus 5 (Android 6.0.1).