Closed Bug 1104142 Opened 5 years ago Closed 5 years ago

Shift-tab doesn't go back to location bar if there is text in the search field

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox34 --- wontfix
firefox35 + verified
firefox36 + verified
firefox37 --- verified

People

(Reporter: Gijs, Assigned: florian)

References

(Depends on 1 open bug)

Details

(Keywords: papercut, ux-consistency)

Attachments

(1 file, 1 obsolete file)

STR:

1. open Firefox beta on a new profile
2. focus location bar
3. hit tab -> focus goes to the search bar
4. hit shift-tab -> focus goes back to the url bar
5. hit tab again to focus the search bar
6. type something
7. hit shift-tab

ER:
back in the url bar

AR:
wikipedia is focused

This breaks my muscle memory and is inconsistent.
Philipp, this is something we marked as wontfix in our initial triage, but I don't think Gijs is the only one annoyed by this. Could you please give this some thoughts next year?
Flags: needinfo?(philipp)
Duplicate of this bug: 1107266
Bug 1107266 mentions a use case I wouldn't have expected for <tab>ing out of the searchbox:

(john ruskin from bug 1107266 comment #0)

> This disrupts using the shiftTab + Tab to back out and re-enter and select
> all, to start typing over prior entry (or to erase and start over)
I also don't really understand why this is so onerous to fix. People can hit <tab> directly to jump to the one-off search items, instead of shift-tab. I don't see any good reason for <shift-tab> to go to the last item in the list of one-off providers. If we want to provide shortcuts for them, we should find some other way of doing that. The current behaviour breaks basic accessibility guarantees (where you navigate forward with tab and backwards with shift-tab, and so a tab+shift-tab sequence should get you back to where you started) and I can't imagine the current behaviour is per-spec - I assumed the wontfix for 34, but tracking for 35/36 meant that it was too late/risky to fix this for 34, not that we were never going to fix it.

I don't think this needs ux-feedback; I think it just needs a patch that bails out of the tab-handling keyhandler when modifiers (including shift) are pressed. Shouldn't be hard, either.

Florian, does this make sense, or am I missing something?
Flags: needinfo?(florian)
Attached patch Possible patch (obsolete) — Splinter Review
I think this does something that Gijs will like better than the current behavior.

This does a couple things that diverge from behaviors Philipp had specifically specified:
- Pressing <tab> when the last one-off item is highlighted will close the panel; focus moves to the next focusable element (ie. the web content area).
- Pressing shift+<tab> when no one-off item is highlighted will close the panel; focus moves to the previous element (ie. the location bar).
- When the panel is closed due to pressing (shift+)<tab>, the searchbox's text field value isn't reverted to what the user typed. The text field value will only be different if the user used the up/down arrows before pressing <tab>, so this is really an edge case.

I personally like the behavior with this patch better than one implemented in 34, but if we want to take this, I would like someone from UX to ui-review.
Flags: needinfo?(florian)
Attachment #8538509 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8538509 [details] [diff] [review]
Possible patch

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

I will look at fixing the third thing you noted.

Stephen, can you ux-review+ the first two results of this patch?

::: browser/components/search/content/search.xml
@@ +1007,5 @@
>                // If no selection, select the first or last one-off button.
>                if (aEvent.shiftKey) {
> +                // If shift+tab is pressed when there was no selected one-off,
> +                // let the focus escape the panel.
> +                return;

Remove the else and braces after this (no else after return)
Attachment #8538509 - Flags: ui-review?(shorlander)
Attachment #8538509 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8538509 - Flags: feedback+
The use case suggested by me in Bug 1107266 has more to do with the behavior brought on by the new search UI (and does not involve the location bar as a problem), and tabbing out & back from the search text box.  

From the search Box: 
   Tabbing out (to the drop down engine dialog) and back in with a Shift-Tab -does- result in a selected search field

versus

   Shift-Tab out and returning with a Tab  (that is backward into the location bar, and returning forward to the search) does -not- 'select' the text in the search box.   This is the essence of my report, now -- a new behavior with the new engine stuff, that came with v34.  I'm not sure that Florian's patch description solves this problem, or makes it more cumbersome, or doesn't address it at all.



Oddly, I can repeat my reported shift tab backing into the engine area, and getting stuck there, but there is something which results in the above instead, a use distinction which perplexes me.   I've tried using a mouse to land in the search box, in lieu of a tab move, and all sorts of other things, and I can't figure out why in some situations i back into the search engines, and loop, and why sometimes I back into the Location bar.

In any case, the original bug 1107266 issues should be addressed here, in this fix:  non-selecting the search field when re-entering, and getting stuck in a loop and backs over the search and loops back to the engines.
Attached patch Patch v2Splinter Review
The third point I mentioned in comment 5 can in my opinion really be handled in a follow-up.
Assignee: nobody → florian
Attachment #8538509 - Attachment is obsolete: true
Attachment #8538509 - Flags: ui-review?(shorlander)
Attachment #8538703 - Flags: review+
Comment on attachment 8538703 [details] [diff] [review]
Patch v2

Still need that UI-review though... Stephen?
Attachment #8538703 - Flags: ui-review?(shorlander)
Comment on attachment 8538703 [details] [diff] [review]
Patch v2

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

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Created attachment 8538509 [details] [diff] [review]
> Possible patch
> 
> I think this does something that Gijs will like better than the current
> behavior.
> 
> This does a couple things that diverge from behaviors Philipp had
> specifically specified:
> - Pressing <tab> when the last one-off item is highlighted will close the
> panel; focus moves to the next focusable element (ie. the web content area).
> - Pressing shift+<tab> when no one-off item is highlighted will close the
> panel; focus moves to the previous element (ie. the location bar).
> - When the panel is closed due to pressing (shift+)<tab>, the searchbox's
> text field value isn't reverted to what the user typed. The text field value
> will only be different if the user used the up/down arrows before pressing
> <tab>, so this is really an edge case.
> 
> I personally like the behavior with this patch better than one implemented
> in 34, but if we want to take this, I would like someone from UX to
> ui-review.

Makes sense.
Attachment #8538703 - Flags: ui-review?(shorlander) → ui-review+
https://hg.mozilla.org/integration/fx-team/rev/d41e8fde7910
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
OS: Windows 8.1 → All
Hardware: x86_64 → All
Status: NEW → ASSIGNED
Iteration: --- → 37.2
https://hg.mozilla.org/mozilla-central/rev/d41e8fde7910
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8538703 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: fixes annoying regression caused by bug 1088660 in 34 for keyboard users.
[User impact if declined]: no way to shift+tab out of the new search UI.
[Describe test coverage new/current, TBPL]: will be verified by QA.
[Risks and why]: simple patch.
[String/UUID change made/needed]: none.
Attachment #8538703 - Flags: approval-mozilla-beta?
Attachment #8538703 - Flags: approval-mozilla-aurora?
Hi Florin, could you please ensure this gets the appropriate QA? Thanks!
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: petruta.rasa
Attachment #8538703 - Flags: approval-mozilla-beta?
Attachment #8538703 - Flags: approval-mozilla-beta+
Attachment #8538703 - Flags: approval-mozilla-aurora?
Attachment #8538703 - Flags: approval-mozilla-aurora+
I've verified this on Windows 7 x64, Mac OS X 10.9.5 and Ubuntu 14.04 x64, using Firefox 35 Beta 6 (BuildID=20141222200458), latest Firefox 36 Aurora (BuildID=20141223004006), and latest Firefox 37 Nightly (BuildID=20141223030207). Scenario from comment #0 now works as expected (dropdown is also closed as anticipated) and also other scenarios like selecting a suggestion or further tabbing do not affect Shift-Tab.
Duplicate of this bug: 1108594
Depends on: 1124747
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Created attachment 8538703 [details] [diff] [review]
> Patch v2
> 
> The third point I mentioned in comment 5 can in my opinion really be handled
> in a follow-up.

Filed bug 1124747.
You need to log in before you can comment on or make changes to this bug.