Closed Bug 1394207 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: General, defect)

56 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 - wontfix
firefox57 --- fixed

People

(Reporter: brian1gaff, Assigned: bgrins)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622
Component: Untriaged → General
What is the bookmark search? Location bar? Search bar on New tab page?
Flags: needinfo?(brian1gaff)
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.
Flags: needinfo?(brian1gaff)
Summary: Firefox 56 beta, Windows 7 32 bit version of ff) Blind user with nvda. When firefox is opened, focus is on bookmark search, not the web page as previously. → When firefox is opened with the sidebar open, the sidebar search box gets/steals focus, not the web page as previously
Flags: needinfo?(gijskruitbosch+bugs)
[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
Blocks: 1360282
Version: 55 Branch → 56 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Brian, do you have cycles to take a look at this?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bgrinstead)
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.
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.
I'll take a look at it
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
Comment on attachment 8904755 [details]
Bug 1394207 - Only fire the SidebarFocused event for non-startup entry points;

https://reviewboard.mozilla.org/r/176536/#review181670
Attachment #8904755 - Flags: review?(gijskruitbosch+bugs) → review+
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?
Attachment #8904756 - Flags: review?(gijskruitbosch+bugs) → review+
Looks like the first part independently landed as bug 1387356.
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
Attachment #8904756 - Flags: review+ → review?(gijskruitbosch+bugs)
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!
Attachment #8904756 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by bgrinstead@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/9435660523cb
https://hg.mozilla.org/mozilla-central/rev/5edfd440c97f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Is this worth backporting to Beta or can it ride the 57 train?
Flags: needinfo?(bgrinstead)
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.
Flags: needinfo?(bgrinstead)
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
QA Whiteboard: [bugday-20170913]
Duplicate of this bug: 1406123
This bug is still present when there is a history sidebar in Firefox 56.0 on MacOS.
(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
(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.
Duplicate of this bug: 1408004
You need to log in before you can comment on or make changes to this bug.