1425454 - The onboarding flow should end up focusing the awesomebar instead of the search box in the page

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

This is a consistency issue across our search access points. Both new tab and new window pages focus the awesomebar and this is the only exception to the rule. UX and product suggest that we fix this interaction as well.

Comment 1

[Ed Lee :Mardak]

I believe this bug is referring to the welcome animation that ends up loading activity stream. Looking into the js on https://www.mozilla.org/en-US/firefox/58.0/firstrun/

It seems to load about:home via

```
var redirectDest = "about:home";
var onVerificationComplete = function() {
  document.getElementById("white-overlay").addEventListener("transitionend", function() {
    window.location.href = redirectDest
 }, false)
};
skipbutton.onclick = onVerificationComplete;
```

Gijs added a Mozilla.UITour.showNewTab in bug 1322726, but the page isn't calling that, and even if it did, the focus would just stay in the content browser.

https://searchfox.org/mozilla-central/source/browser/components/uitour/UITour.jsm#1497-1498

This would get the event to navigate and focus, but I'm not sure if that behavior can change, nor how to get the page updated to use it?

```diff
diff --git a/browser/components/uitour/UITour.jsm b/browser/components/uitour/UITour.jsm
index 2ad1d434b874..e7a69762958b 100644
--- a/browser/components/uitour/UITour.jsm
+++ b/browser/components/uitour/UITour.jsm
@@ -1497,4 +1497,5 @@ this.UITour = {
   showNewTab(aWindow, aBrowser) {
     aWindow.openLinkIn("about:newtab", "current", {targetBrowser: aBrowser});
+    this.getTarget(aWindow, "urlbar").then(target => target.node.focus());
   },
 
```

A quick hack with the above change and doing `document.getElementById("white-overlay").addEventListener("transitionend", () => Mozilla.UITour.showNewTab())` from the web console then clicking [Skip this step] seems to show it works…

Comment 2

[:Gijs (he/him)]

Not sure what the question is... the suggested change to use showNewTab() and focusing the location bar in that code seems OK to me. Though we should technically check inside .then() that focus hasn't moved, the selected tab hasn't changed, etc. (so as not to steal focus from the user) . On the other hand, given the once-in-a-Firefox-profile-lifetime case that this runs, and that `getTarget` really shouldn't be slow, I wonder if it's worth added complexity to deal with focus stealing...

I can talk about the about:home vs. about:newtab thing privately if you would like. For this bug, I would just say we should move to using about:newtab if/when possible.

Comment 3

[Ed Lee :Mardak]

Attachment: v1 fix video

Comment 4

[Ed Lee :Mardak]

Attachment: v1 fix video

Comment 5

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/bedrock/pull/5421

Comment 6

[Ed Lee :Mardak]

Attachment: Bug 1425454 - The onboarding flow should end up focusing the awesomebar instead of the search box in the page.

Review commit: https://reviewboard.mozilla.org/r/216566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216566/

Comment 7

[Ed Lee :Mardak]

Here's try running uitour tests 20x:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8c260618b8e02025d7e24834747dc26299cec97

And I pushed with --skip-lint:

browser/components/uitour/UITour.jsm
    93  warning  explictly  ==> explicitly  (codespell)
  1057  warning  explictly  ==> explicitly  (codespell)
  1058  warning  explictly  ==> explicitly  (codespell)

✖ 3 problems (0 errors, 3 warnings)
abort: Lint run failed (if you really need to you can --skip-lint)

Comment 8

[:Gijs (he/him)]

Comment on attachment 8946547 [details]
Bug 1425454 - The onboarding flow should end up focusing the awesomebar instead of the search box in the page.

https://reviewboard.mozilla.org/r/216566/#review222296

LGTM!

Comment 9

[Ed Lee :Mardak]

I'll land the m-c change first as per irc:

agibson> Ok, well I’ll review your PR as soon as I can get to it
agibson> We’ll hold off merging until we can test the patch in m-c when it lands

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcd48c850369
The onboarding flow should end up focusing the awesomebar instead of the search box in the page. r=Gijs

Comment 11

[Ed Lee :Mardak]

past, where/when should this be fixed? agibson just noted in the PR to not update the website until the UITour change reaches Firefox 58. If we just wait for the m-c patch to reach Release 60, that'll be ~May.

On the other hand, bug 1322726 added `UITour.showNewTab` to Firefox 51, and its behavior is to just navigate to about:newtab (without focusing the address bar), which is effectively the same current behavior of navigating to about:home since Firefox 57 as the two pages show the same content now.

I believe if we merge the bedrock PR now, there won't be any feature regression, and once the patch from this bug makes it to Release, it'll actually get the address bar focused.

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/fcd48c850369

Comment 13

[Ed Lee :Mardak]

Attachment: v2 fix video 56 vs 58 vs 60

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

I don't think this is particularly urgent to fix, but at the same time, if it's effectively the same behavior for new users, it makes sense to update the website. The patch looks very safe for uplift, too.

Comment 15

[[github robot]]

Commit pushed to master at https://github.com/mozilla/bedrock

https://github.com/mozilla/bedrock/commit/50b56bfbc07010a55e3a7257f877ea2151403822
Fix Bug 1425454 - The onboarding flow should end up focusing the awesomebar instead of the search box in the page

Comment 16

[Ed Lee :Mardak]

I see if I set browser.uitour.testingOrigins;https://www-dev.allizom.org and visit https://www-dev.allizom.org/en-US/firefox/60.0/firstrun/ it seems to do the right thing and going to the 56.0 url, it does the old thing (no address bar focus).

Not sure who usually does onboarding flow QA, but I'll try asking around… probably when the changes hit mozilla.org?

Comment 17

[Ed Lee :Mardak]

I see the firstrun changes live now: https://www.mozilla.org/en-US/firefox/60.0/firstrun/

Ciprian, is there someone who usually covers mozilla.org website changes that should also verify? We would like to uplift the mozilla-central changes to beta 59 before Tuesday's EARLY_BETA cutoff.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

FYI, EARLY_BETA_OR_EARLIER already got unset prior to this week's b7 build.

Comment 19

[Ciprian Muresan [:cmuresan], Ecosystem QA]

@Ed I'm not sure who owns or covers mozilla.org changes. I can however confirm that the address bar is focused after I finish the onboarding flow on Nightly 60 using Windows 10, Mac 10.13 and Arch Linux, I'm not sure if that's enough though.

@Tom, @Stuart, do either of you know if anyone owns mozilla.org testing?

Comment 20

[Alex Gibson [:agibson]]

(In reply to Ciprian Muresan [:cmuresan], Desktop Engineering QA, :steve from comment #19)
> @Tom, @Stuart, do either of you know if anyone owns mozilla.org testing?

The Mozilla.org team own their own QA for testing changes on www.mozilla.org. We've already verified with :Mardak that these changes are working as intended & monitored the deployment. If there's anything additional you need outside of this process (i.e. on the Firefox side) then that's probably outside of our scope.

Comment 21

[Stuart Philp :sphilp]

If there is more needed outside of what :agibson mentioned, the old WebQA team now reports through :kthiessen and they would be able to help.

Comment 22

[Ed Lee :Mardak]

[Tracking Requested - why for this release]: Improve the first run experience with a small tweak. Verified in comment 19.

Comment 23

[Ed Lee :Mardak]

Comment on attachment 8946547 [details]
Bug 1425454 - The onboarding flow should end up focusing the awesomebar instead of the search box in the page.

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream being on by default in 57 (with the decision in bug 1395961 to have focus be in the address bar when opening Activity Stream)
[User impact if declined]: Inconsistent experience when opening a new window (focusing address bar) vs firstrun redirect to Activity Stream (keeping focus in page)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes comment 19
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: One line change to UITour functionality not used by Firefox code
[String changes made/needed]: None

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8946547 [details]
Bug 1425454 - The onboarding flow should end up focusing the awesomebar instead of the search box in the page.

Fix for UX consistency, verified in Nightly, let's uplift this for beta 10.

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c3d1a48781b

Comment 26

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  58.0.2 on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 59.0b10 on Win 8.1 x64, Mac OS X 10.13.2 and Ubuntu 14.04 x64.