Closed Bug 1385374 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: yzen, Assigned: mozilla)

References

Details

(Keywords: access, Whiteboard: [fxsearch])

Attachments

(1 file, 1 obsolete file)

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.
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]
(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)
P2 for now as there is nobody available to look into this and the hint is only displayed 4 times tops.
Priority: -- → P2
Attached 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.
(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.
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.