Closed Bug 1022464 Opened 7 years ago Closed 5 years ago

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

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

(firefox46 verified, firefox47 verified, firefox48 verified, fennec+)

VERIFIED FIXED
Firefox 46
Tracking Status
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified
fennec + ---

People

(Reporter: rnewman, Assigned: mcomella)

References

Details

Attachments

(3 files)

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.
Sounds like a long overdue follow-up to bug 823230.
Blocks: 823230
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.
tracking-fennec: --- → ?
I'll try writing a patch for this. This could fit in well with the recent search suggestion/history work we've been doing.
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
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).
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
Assignee: margaret.leibovic → nobody
NI-ing liuche here since we worked on this in mozlando (moving over from bug 1230587)
Flags: needinfo?(liuche)
Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mfinkle
Attachment #8698137 - Flags: review?(mark.finkle)
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 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.
Flags: needinfo?(liuche)
Attachment #8698137 - Flags: review?(mark.finkle)
(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.
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 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/
Attachment #8698137 - Attachment description: MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mfinkle → MozReview Request: Bug 1022464 - Returning to URL bar containing search terms doesn't re-enter search mode. r=mcomella
Attachment #8698137 - Flags: review?(michael.l.comella)
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.)
This doesn't fully work, but is this what you had in mind? This is the same transparency color that we use elsewhere.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #8698301 - Flags: feedback?(alam)
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 :)
Attachment #8698301 - Flags: feedback?(alam) → feedback+
Attachment #8698137 - Flags: review?(michael.l.comella)
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!
Let's land this behind a Nightly flag to see how it feels in practice, without committing to shipping it.
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 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/
Attachment #8698137 - Flags: review?(michael.l.comella)
Attachment #8698137 - Flags: review?(michael.l.comella)
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?
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.
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.
Flags: needinfo?(michael.l.comella)
(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?
Assignee: liuche → michael.l.comella
Flags: needinfo?(michael.l.comella) → needinfo?(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.
Flags: needinfo?(margaret.leibovic) → needinfo?(mark.finkle)
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).
Manually clearing NI flag. Thanks, MozReview!
Flags: needinfo?(mark.finkle)
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.
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.
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
I landed the changes that worked locally, but NI liuche to double-check my changes and ensure they make sense.
Flags: needinfo?(liuche)
https://hg.mozilla.org/mozilla-central/rev/69197595f283
https://hg.mozilla.org/mozilla-central/rev/33a02b110c34
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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!
Flags: needinfo?(liuche)
Depends on: 1240549
Depends on: 1244734
Depends on: 1257628
QA Contact: 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).
Status: RESOLVED → VERIFIED
Depends on: 1263580
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.