Closed Bug 1425454 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 60
Iteration:
60.2 - Feb 12
Tracking Status
firefox58 --- wontfix
firefox59 + verified
firefox60 --- verified

People

(Reporter: past, Assigned: Mardak)

References

Details

(Whiteboard: [fxsearch] [AS60MVP])

Attachments

(4 files, 1 obsolete file)

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.
Priority: -- → P2
Iteration: --- → 60.1 - Jan 29
Iteration: 60.1 - Jan 29 → 60.2 - Feb 12
Whiteboard: [fxsearch] → [fxsearch] [AS60MVP]
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…
Assignee: nobody → edilee
Component: Activity Streams: Newtab → Tours
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attached video v1 fix video (obsolete) —
Attached video v1 fix video
Attachment #8946538 - Attachment is obsolete: true
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)
Component: Tours → Activity Streams: Newtab
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!
Attachment #8946547 - Flags: review?(gijskruitbosch+bugs) → review+
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
Pushed by edilee@gmail.com:
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
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.
Flags: needinfo?(past)
https://hg.mozilla.org/mozilla-central/rev/fcd48c850369
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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.
Flags: needinfo?(past)
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
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?
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.
Flags: qe-verify?
Flags: needinfo?(ciprian.muresan)
FYI, EARLY_BETA_OR_EARLIER already got unset prior to this week's b7 build.
@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?
Flags: needinfo?(tgrabowski)
Flags: needinfo?(sphilp)
Flags: needinfo?(ciprian.muresan)
(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.
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.
Flags: needinfo?(sphilp)
[Tracking Requested - why for this release]: Improve the first run experience with a small tweak. Verified in comment 19.
Status: RESOLVED → VERIFIED
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(tgrabowski)
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
Attachment #8946547 - Flags: approval-mozilla-beta?
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.
Attachment #8946547 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: qe-verify+
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.