Keyboard trap when Search Suggestions Hint is visible and focus is inside the input field.

RESOLVED FIXED in Firefox 57

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: yzen, Assigned: mozilla)

Tracking

({access})

unspecified
Firefox 57
access
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
When keyboard focus is in the address bar and the search suggestions hint bar becomes visible, the keyboard is trapped inside the input field of the address bar. There is no way to tab away from it: neither forward nor back.

Comment 1

2 years ago
Panos, how do we get this triaged/prioritized by the search / location bar folks?


Workaround: clear the location bar contents.

Yura, do you know if this is a regression?
Component: Toolbars and Customization → Location Bar
Flags: needinfo?(yzenevich)
Flags: needinfo?(past)
Whiteboard: [fxsearch]
(Reporter)

Comment 2

2 years ago
(In reply to :Gijs from comment #1)
> Panos, how do we get this triaged/prioritized by the search / location bar
> folks?
> 
> 
> Workaround: clear the location bar contents.
> 
> Yura, do you know if this is a regression?

Haven't tried on release but this issue also happens on Beta.
Flags: needinfo?(yzenevich)
just  adding [fxsearch] into the whiteboard puts this into bugs to triage.
Flags: needinfo?(past)
Duplicate of this bug: 1395243
P2 for now as there is nobody available to look into this and the hint is only displayed 4 times tops.
Priority: -- → P2
(Assignee)

Comment 6

2 years ago
Posted patch disable-tab-scrolling.patch (obsolete) — Splinter Review
The search suggestion hint opens a popup under urlbar.  The autocomplete widget takes an open popup as license to use tab presses for scrolling (see toolkit/content/widgets/autocomplete.xml, method handleKeyPress).

So disable tabscroll under the same conditions that are used to disable one-off search.  That is, when there are no results to display and a notification hint is displayed.  This is the least intrusive fix I can think of (otherwise should we change the logic in the widget, or move notifications out of the popup?).

Please look out for regressions.
Comment on attachment 8903934 [details] [diff] [review]
disable-tab-scrolling.patch

Review of attachment 8903934 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1979,4 @@
>            // Don't show the one-off buttons if we are showing onboarding and
>            // there's no result, since it would be ugly and pointless.
>            this.footer.collapsed = this._matchCount == 0;
> +          this.input.tabScrolling = false;

Shouldn't we do the same as collapsed, thus disable tab scrolling only if there are no results? If there are results, the user may expect tab to work as usual, and he can use ESC, as usual, to escape the tab trap.
(Assignee)

Comment 8

2 years ago
(In reply to Marco Bonardo [::mak] from comment #7)
> >            this.footer.collapsed = this._matchCount == 0;
> > +          this.input.tabScrolling = false;
> 
> Shouldn't we do the same as collapsed, thus disable tab scrolling only if
> there are no results? If there are results, the user may expect tab to work
> as usual, and he can use ESC, as usual, to escape the tab trap.

Doh, oh, yes of course.  As a matter of fact, tab & esc still work in the list with this patch because the onResultsAdded handler enables tabscrolling again at the right moment.  But I'll attach a new one.
(Assignee)

Comment 9

2 years ago
Attachment #8904299 - Flags: review?(mak77)
Comment on attachment 8904299 [details] [diff] [review]
disable-tab-scrolling-v2.patch

Review of attachment 8904299 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thank you!
Attachment #8904299 - Flags: review?(mak77) → review+
Attachment #8903934 - Attachment is obsolete: true
Attachment #8903934 - Flags: review?(mak77)
Not requesting a Try since the change looks pretty safe and I doubt anything is testing this edge case. Not requesting a test since, as previously said, this is a minor annoyance that may just happen 4 times per profile.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76834322c5a8
Disable tab scrolling when urlbar popup only displays a notification. r=mak
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76834322c5a8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Duplicate of this bug: 1378165
You need to log in before you can comment on or make changes to this bug.