1394207 - When firefox is opened with the sidebar open, the sidebar search box gets/steals focus, not the web page as previously

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Brian Gaff]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622

Comment 1

[YF (Yang)]

What is the bookmark search? Location bar? Search bar on New tab page?

Comment 2

[:Gijs (out for now; he/him)]

The only bookmarks search I can think this would apply to is the one in the sidebar, and indeed if the sidebar is open that is what happens for me.

Reporter: if you don't actually use the sidebar, you can hide it again by using ctrl-b (or activating the button that should be labeled "close sidebar" and in the tab hierarchy from that search box).

We should figure out if/when/why this changed. Even if not a regression, I don't think the sidebar should steal focus on startup. It makes sense that it takes focus when opened explicitly, but not on startup / window open.

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]:

I can reproduce the problem on Beta56 Windows10.

Steps To Reproduce:
1. Set data:text/html,<div style="height:2000px">foo foo foo</div> when Firefox starts in options.
2. Open Bookmarks Sidebar
3. Re launch browser

4. Try to scroll page with keyboard

Actual Results:
After step3, caret is in the Sidebar.
Un able to scroll the page.

Expected Results:
Able to scroll the page

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8cbbafc26747fd8c92f065e3883896f5e34da52a&tochange=114fb8a5f56cdff35784193d3143039aa43b48c8

Regressed by: Bug 1360282

Comment 4

[:Gijs (out for now; he/him)]

Brian, do you have cycles to take a look at this?

Comment 5

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

I wouldn't hold back 56 release for this issue and we are nearing the end of the cycle, so better if I can keep the "tracked" bugs minimal. I'm happy to take a patch for 56 though if you come up with one.

Comment 6

[Brian Grinstead [:bgrins]]

For context, we discussed this event in https://bugzilla.mozilla.org/show_bug.cgi?id=1360282#c10. There's a code comment indicating the expected behavior is to not fire SidebarFocused on startup but after unifying the show sequence from all entry points we must have regressed that.

From https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sidebar.js#252L:
   * Fire a "SidebarFocused" event on the sidebar's |window| to give the sidebar
   * a chance to adjust focus as needed. An additional event is needed, because
   * we don't want to focus the sidebar when it's opened on startup or in a new
   * window, only when the user opens the sidebar.

Comment 7

[Brian Grinstead [:bgrins]]

I'll take a look at it

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1394207 - Only fire the SidebarFocused event for non-startup entry points;

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

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1394207 - Export a function to show the sidebar without firing focus for session restore;

If the sidebar is being opened during startup (either at delayedLoad or when being
adopted from another window), we don't want sidebar panels to steal focus from the page

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

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8904755 [details]
Bug 1394207 - Only fire the SidebarFocused event for non-startup entry points;

https://reviewboard.mozilla.org/r/176536/#review181670

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8904756 [details]
Bug 1394207 - Export a function to show the sidebar without firing focus for session restore;

https://reviewboard.mozilla.org/r/176538/#review181672

Sorry for not thinking this through fully in the first review. :-(

::: browser/base/content/browser-sidebar.js:333
(Diff revision 1)
>      return this._show(commandID).then(() => {
>        if (triggerNode) {
>          updateToggleControlLabel(triggerNode);
>        }
> +
> +      this._fireFocusedEvent();

I think this will now also fire when session store re-shows a sidebar ( https://dxr.mozilla.org/mozilla-central/rev/3ecda4678c49ca255c38b1697142b9118cdd27e7/browser/components/sessionstore/SessionStore.jsm#4047 ). Is it difficult to fix that here, too?

Comment 12

[:Gijs (out for now; he/him)]

Looks like the first part independently landed as bug 1387356.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8904755 [details]
Bug 1394207 - Only fire the SidebarFocused event for non-startup entry points;

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

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8904756 [details]
Bug 1394207 - Export a function to show the sidebar without firing focus for session restore;

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

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8904756 [details]
Bug 1394207 - Export a function to show the sidebar without firing focus for session restore;

Resetting review since this is a new patch

Comment 16

[:Gijs (out for now; he/him)]

Comment on attachment 8904756 [details]
Bug 1394207 - Export a function to show the sidebar without firing focus for session restore;

https://reviewboard.mozilla.org/r/176538/#review181884

Nice, thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9435660523cb
Only fire the SidebarFocused event for non-startup entry points;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5edfd440c97f
Export a function to show the sidebar without firing focus for session restore;r=Gijs

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9435660523cb
https://hg.mozilla.org/mozilla-central/rev/5edfd440c97f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this worth backporting to Beta or can it ride the 57 train?

Comment 20

[Brian Grinstead [:bgrins]]

I think this can ride the train. We aren't surfacing the sidebar button in the primary UI until photon ships in 57. And also this doesn't apply cleanly to Beta - it depends on Bug 1387356 which removed some deprecated functions that were there for addon support which I assume we don't want to take away in 56.

Comment 21

[Azmina]

I have reproduced this bug with Nightly 57.0a1 (2017-08-24) on Windows 8.1, 64 Bit!

This bug's fix is verified on Latest Nightly 57.0a1.

Build ID : 20170918100059
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 23

[carl]

This bug is still present when there is a history sidebar in Firefox 56.0 on MacOS.

Comment 24

[carl]

(In reply to carl from comment #23)
> This bug is still present when there is a history sidebar in Firefox 56.0 on
> MacOS.
It never had this behavior until 56.0

Comment 25

[:Gijs (out for now; he/him)]

(In reply to carl from comment #24)
> (In reply to carl from comment #23)
> > This bug is still present when there is a history sidebar in Firefox 56.0 on
> > MacOS.
> It never had this behavior until 56.0

This bug is only fixed in 57. It's expected that you're seeing this in 56, but it's too late to fix this within 56 because it's already released, and this issue is not severe enough to warrant a specific dot-release. If it's very annoying given your usage pattern, you could consider switching to 57 beta. Otherwise, this will be fixed when 57 reaches the release channel in a few weeks' time.