Closed Bug 1295458 Opened 8 years ago Closed 7 years ago

Rework key and mouse handling for the one-off search buttons

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Depends on 3 open bugs)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1295458 - Rework key and mouse handling for the one-off search buttons.
59 bytes, text/x-review-board-request
florian
: review+
phlsa
: ui-review+
Details 
I talked with Stephen, and we thought it would be a good idea to treat the one-offs as a single unit for keyboard navigation -- to treat them like another row in the list basically (kind of).

Say the textbox is focused.  As you press Tab/Down, you move the selection to the first list item and then down the list items, and then when you get to the last list item and hit Tab/Down again, you select the first one-off.  From there, you can press Left/Right to select another one-off.  When you press Tab/Down again, the selection wraps around to the top of the list.  Shift+Tab/Up work similarly but in reverse.

So we don't break people's Tab habits, but you can still Tab down to the one-offs.

Alt+Up/Down would still work as they work now.
Stephen pointed out an obvious problem with that idea: the left/right arrow keys move the caret in the text field, so we can't use them to move left/right within the one-offs.  So instead, when a one-off is selected, let's have tab skip to the results list, and let's have the up/down arrow keys move through the one-offs.
(In reply to Drew Willcoxon :adw from comment #1)
> when a one-off is selected,
> let's have tab skip to the results list, and let's have the up/down arrow
> keys move through the one-offs.

What problem is this change intended to fix?

Are you talking specifically about the locationbar (disabletab="true") case, or thinking about one-off buttons in general?
Right now we're only talking about the urlbar.  I don't think we're sure yet whether we want the same key handling in the searchbar, but it's possible.  Stephen's going to think more about it.  If we prioritize consistency, then we'll have to change the search bar handling too.

One related point though is that in both the search bar and the urlbar, Stephen doesn't want the double selection that's possible in both currently, where both a list item and a one-off can be blue -- except in the case of the power-user alt+up/down trick, where it can't be avoided.

The goal for the urlbar is that you should be able to select the one-offs from the keyboard, but with these constraints:

* in a discoverable way (i.e., not alt+up/down)
* in a way that doesn't break people's urlbar tab/arrow key habits (break it too much anyway)
* in a way where you don't have to move through all the one-offs in order to get back to the results list if you don't want
(In reply to Drew Willcoxon :adw from comment #3)

> One related point though is that in both the search bar and the urlbar,
> Stephen doesn't want the double selection that's possible in both currently,
> where both a list item and a one-off can be blue -- except in the case of
> the power-user alt+up/down trick, where it can't be avoided.

In the searchbar at least, this double selection is what makes the feature powerful when used with the keyboard.

Eg. something I would typically do:
Type a few letters of something (let's say a song title), then press down a couple times to highlight the whole song title, then press tab once or twice to select a relevant one off (eg. youtube for a song, or wikipedia).
If you make a change that removes the double selection, you force me to either type the whole query string before using a one-off, or to skip using a one off and type something like 'wiki' or 'youtube' as part of the query.

I know this double selection behavior is a bit unusual, but I can't imagine someone using both up/down and tab/shift+tab on the same UI without doing so intentionally. So I don't think the double selection is a problem in the searchbar.

> The goal for the urlbar is that you should be able to select the one-offs
> from the keyboard, but with these constraints:
> 
> * in a discoverable way (i.e., not alt+up/down)
> * in a way that doesn't break people's urlbar tab/arrow key habits (break it
> too much anyway)
> * in a way where you don't have to move through all the one-offs in order to
> get back to the results list if you don't want

I would add:
* in a way that doesn't require going through each of the 12 awesomebar items before reaching the first one-off button.
Stephen, could you please try this (OS X) build and let me know what you think?

https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-c2790c345747665c522b546a185823c6d8e79f2b/

It implements the key handling and mouseover states we talked about (this bug, bug 1295460, bug 1295463).  It only changes the urlbar's key behavior, but it applies the one-off styling to both the urlbar and searchbar.

It's rough around the edges and not production ready, especially the searchbar styling, but I think it should be enough to see how/whether everything works, especially the urlbar.

Questions/observations:

(1) How much of the key handling and styling should apply to the searchbar?

(2) The new mouseover state for the urlbar -- this is very different from how it currently behaves.  In a way it complicates the current behavior because now two items can appear "selected" at once, one gray, one blue.  It's also different from how autocomplete/listboxes behave everywhere else in Firefox.

(3) Now there can be *three* things that appear selected when you alt+up/down: a blue list item, a blue one-off, and a gray moused-over list item or one-off.

(4) With this build, if your mouse happens to be over the popup when it opens, you immediately get a gray item, which is a bit of an information overload that you didn't trigger.  We should probably wait until you move your mouse, don't you think?
Flags: needinfo?(shorlander)
(In reply to Drew Willcoxon :adw from comment #5)

> now two items can appear "selected" at once, one gray, one blue.

I had related discussions with phlsa almost 2 years ago. Unfortunately the discussion was mostly in-person. Philipp made an interactive mockup but without detailed description of the intended changes.

I wrote a slightly more detailed comment at bug 1118691 comment 2.
Stephen and I talked about this today, so I'll clear the needinfo.

We're going to try a different idea where mousing over list items and one-off buttons selects them.  In other words, mousing over an item or one-off would be no different from keying up or down to it.  That means that mousing over an item would replace the text in the textbox with whatever is appropriate for the item (a URL or search suggestion), same as if you had keyed down to it.  That's what Safari does incidentally.

We'll port this behavior to the searchbar, so we're valuing consistency between the urlbar and searchbar over consistency with how the searchbar has behaved for a long time.

When you alt+up/down to a one-off, the selected list item will turn gray, and the one-off will be selected and turn blue.  So there's at most one blue/selected thing.

We'll also port the settings gear icon to the searchbar.

I'll work on a build for all that and then we can see how we like it.
Assignee: nobody → adw
Status: NEW → ASSIGNED
(In reply to Drew Willcoxon :adw from comment #7)

> We'll also port the settings gear icon to the searchbar.

What's the reason for this change? And how is it related to this bug which is about fixing the keyboard handling?

Currently when all the one-off buttons are disabled, we no longer show the grid, but we still need a way to access the settings. I don't think showing the grid with just the gear icon at the end an empty line would look good.
You're right, it's not related to this bug, but it came up in the context of what to port to the searchbar.  I think consistency across the urlbar and searchbar is the reason, but Stephen can comment.  IMO consistency alone is a good reason, but I also think that the searchbar's current settings button could use some brevity.

You raise a good point about what would happen when the one-offs are disabled.  Maybe Stephen can comment?  IMO having the gear icon at the end of an empty line would look fine, but I might change my mind when I actually see it. :-)
(In reply to Drew Willcoxon :adw from comment #9)

> IMO having the gear icon at the end of an empty line would look fine

I think it's not so much about whether it looks fine, but more about whether it's discoverable. Using a button with a descriptive string was initially for discoverability: we didn't want people to think we were forcing a specific engine on them.
(In reply to Drew Willcoxon :adw from comment #7)
> We're going to try a different idea where mousing over list items and
> one-off buttons selects them.  In other words, mousing over an item or
> one-off would be no different from keying up or down to it.  That means that
> mousing over an item would replace the text in the textbox with whatever is
> appropriate for the item (a URL or search suggestion), same as if you had
> keyed down to it.  That's what Safari does incidentally.

Is it not this dangerous? we'll have to distinguish real mouse interactions from unwanted ones, it's easy for some users to inadvertently touch the touchpad. and that would throw away the user typed in text, that wold be infuriating.
It's also likely the cursor is in the area where the popup opens, since now it's screen wide and a bit taller.
We just had a bunch of complains for the fact sometimes we were serving the mouse hovered selection instead of the keyboard selection, and fixed a bug for that. showing the text before handling the wrong operation would not make it better.
I'll see what safari does, and test the try build, but off-hand I don't like the idea and the regression headaches it will bring.

> When you alt+up/down to a one-off, the selected list item will turn gray,
> and the one-off will be selected and turn blue.  So there's at most one
> blue/selected thing.

This is fine, it'd also be fine to have mouse hovered selection different from "real" selection like Chrome does.
also, not much time ago we had a bug where a bunch of users where typing in the urlbar and at the same time moving the mouse through the results to quickly click on the second or third result (by muscle memory), now when they move the mouse we'd override what they are typing.
Yeah, these are good points that I brought up when Stephen and I talked about it.  I think it's a design that's worth exploring at least.  There are definitely be some people it would break, and I bet that 100% of them are Firefox power users.

Your point about the popup being as wide as the window, so it's very easy to trigger this, is a good one.

We need to make it easy to recover your typed text.  Moving your mouse away from the popup should do it, for one thing.  That's what Safari does too by the way.
Flags: needinfo?(shorlander)
While looking into bug 1302472 I noticed that I can't properly fix that bug (thus I'm reverting the previous broken behavior) cause on mousemove the richlist-autocomplete sets selectedIndex.
That way it's basically impossible to distinguish a mouse selection from a keyboard selection in the controller, forcing us to do additional string comparisons to figure out if the user tried to confirm the popup entry or the input field autofill content. The treeview implementation doesn't do this.
Unfortunately if completeselectedindex is false, it's impossible to do a string comparison to understand if the user selected the entry with the mouse or the keyboard, cause the input field contents don't change.
There 2 possible fixes. What you plan to do here would basically enforce completeselectedindex, and thus would allow to do an additional string comparison.
Another alternative would be to go the Chrome way. The fix would consist in not setting selectedIndex in the mousemove handler, and rather set it in the popupclick hander (to mouseSelectedIndex), but that way we lose richlistbox selected hilight. Implementing an hover effect would solve that. At that point we could remove the various string comparisons in the controller. On the other side, this may uncover some subtle bugs, so it would be something to do at the beginning of a cycle.
Blocks: 1295460
Blocks: 1295463
Stephen, would you mind trying this build?  It implements the behavior we talked about last time and summarized in comment 7.  But only for the urlbar, not the searchbar.  It doesn't include the styling changes in bug 1295460 and bug 1295463.  Standard disclaimer, it's not perfect, etc. :-)

https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-665e409d0e3a144ef71490612bd02e801227b381/

Things to note:

* Only one thing is selected/blue in the popup at a time.

* The thing you mouse over becomes selected/blue.  If your mouse is in the popup but not over a result/button, then nothing's selected/blue, and the textbox reverts to what you typed, not what was autofilled.

* When you mouse out of the popup, the textbox is restored to what was autofilled, or if it wasn't autofilled, to what you typed.  (To do: restore the selection too.)  And the first result is selected again.

* When you mouse over a one-off, the textbox is restored to the value you typed, not what was autofilled.

* When you alt+up/down to a one-off, the selected/blue result becomes gray, and it stays gray until you change the selection some other way.

* The tab key moves down the results and then into the one-offs and then back to the results.  Once you tab to the one-offs, you can press up/down to change the selection as usual and select another one-off.

* The down/up keys move through both the results and all the one-offs.
Flags: needinfo?(shorlander)
Hi Drew, thanks for the build! This interaction feels a lot better to me. Removes the complexity and ambiguity of the previous version. It feels clear what's going to happen and removes the mixed state problem.
Flags: needinfo?(shorlander)
Please confirm that these 2 breakages are intentional, to avoid misunderstanding.

(In reply to Drew Willcoxon :adw from comment #7)
> mousing over an item would replace the text in the textbox
This will cause unwanted navigation if mouse is accidentally moved by 1px during typing. In case you
aren't fully aware: you (team) had already introduced this bug several times - bug1043584, bug1292310

(In reply to Drew Willcoxon :adw from comment #16)
> * The tab key moves down the results and then into the one-offs and then back to the results.
> 
> * The down/up keys move through both the results and all the one-offs.
Some users, including me, rely on Tab key cycling between suggestions in urlbar (1>2, 2>3, ...last>1),
and also Up key (or Shift+Tab, which is harder) to switch to the last suggestion.
The "one-off buttons" widget in its current state already forbids me to use Up key, but "improvement" you're describing will remove this functionality completely.
Note: there's also a known bug, that "one-off buttons" killed said functionality in good old searchbar (pre-Australis), and now all possible keys cycle through useless "one-off buttons" instead.

Wording:  (added, because some people may misunderstand)
1) word "useless" is appropriate, because I'm describing use case when the goal is to switch between
   suggestions, not to pick another search. "one off buttons" are completely useless in this case.
2) expression "good old searchbar" is used in terms of known regressions: new searchbar has a lot more
   regressions (undeniably), therefore is harder to use/rely on and is a worse UI.
3) expression `"improvement"` contains quotes, because as you can see from this comment, it only
   brings more trouble to end users (it just adds more ways to cycle through useless
  "one off buttons", despite there're already enough ways of activating them, i.e. removes existing
   functionality and doesn't add anything new), and therefore makes UI more unreliable.
Flags: needinfo?(adw)
(In reply to arni2033 from comment #18)
> (In reply to Drew Willcoxon :adw from comment #7)
> > mousing over an item would replace the text in the textbox
> This will cause unwanted navigation if mouse is accidentally moved by 1px
> during typing.

I question how often this really happens.  But, we should probably do something to mitigate it.  Maybe we can come up with a heuristic for intentional mouse movements.
Flags: needinfo?(adw)
This implements the mouse and key handling discussed in this bug.  I'm expanding the scope of this bug to include both since they're related.

The mouse handling is specific to the urlbar, but the key handling is implemented in both the urlbar and searchbar.  The mouse handling should probably be extended to the searchbar too so that the two bars behave the same...

In summary, the changes are:

In the urlbar:

* Moving the mouse over a list item selects it, same as if you had keyed to it.  The item's value is placed in the textbox.
* Moving the mouse over a one-off selects it, same as if you had keyed to it.  The textbox value is reverted to what you typed.
* Moving the mouse out of the popup reverts the selection to the heuristic result -- which is either what was autofilled, or if there wasn't autofill, what you typed.

In the urlbar and searchbar:

* The up/down keys move through the list items as usual.  The one-offs are treated as one unit for the up/down keys: when the last list item is selected and you press down, the first one-off is selected, and when you press down again, the selection wraps around to the first list item.
* Once the selection is in the one-offs, pressing left/right moves the selection within the one-offs.
* Alt+up/down now cause an "alt selection" in the list: the list item that was selected becomes "alt selected", and the actual selection moves through the one-offs.  In the urlbar, the alt-selected list item turns gray.  I want to do that in the searchbar too, but that would require implementing a custom tree view.
* The tab key moves between the list and the one-offs.

I'm probably forgetting some things but I think that pretty much covers the changes...
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Marco, could you please review the non-search.xml parts?  Please feel free to look at that too of course.  I described the changes in comment 21.
Attachment #8829366 - Flags: review?(mak77)
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Florian, could you please review the search.xml parts?  Feel free to look at everything else too of course.  I described the changes in comment 21.
Attachment #8829366 - Flags: review?(florian)
Still need to post a patch for tests.
Summary: Rework Tab/Up/Down key handling for the one-off search buttons → Rework key and mouse handling for the one-off search buttons
Stephen, could you please try this build and let me know what you think?  This one should completely implement all the key and mouse handling we talked about, which the previous build did not.  I just asked for review on it, so this is the "final" interaction, subject to the reviewers' and your comments of course.  Comment 21 summarizes the behavior if you need a refresher.

https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-377bac3542dd2b57cc63657fb05332c7853b3b15/
Flags: needinfo?(shorlander)
(In reply to Drew Willcoxon :adw from comment #21)
> * Moving the mouse over a list item selects it, same as if you had keyed to
> it.  The item's value is placed in the textbox.

I'm still not sure how we solved the issues we always had when we tried to do this, also considered no other browser behaves this way... Wouldn't this end up being more error-prone for users moving from other browsers to firefox? Can we put this behind a pref easily?
(note that I'm clearly biased here, cause imo this approach was a wontfix from the beginning, while we should instead have both selection and hover like Chrome, see the last paragraph of comment 14. So please take into account that, in case I sound too negative).
Safari does behave this way.
(In reply to Drew Willcoxon :adw from comment #21)
> * The tab key moves between the list and the one-offs.

I forgot that we specifically didn't want to break people's tab habits for the urlbar.  So this patch needs to be updated to add back the disabletab stuff to search.xml.
> * Moving the mouse over a one-off selects it, same as if you had keyed to it.  The textbox value is reverted to what you typed.

I understand why we are doing this, and indeed I'm one of the users who like to click on one-offs after typing a string, rather then reaching them by keyboard, and I'd be sad to search the last result I hovered.

I wonder though if this doesn't disallow searching for a suggestion. I clarify:
1. type "porcu"
2. select the "porcupine" suggestion from google
3. click on Wikipedia one-off button
ER. I read about porcupines habits
AR. we search "porcu" on wikipedia

Notice that this doesn't work even today (it could if we'd not always use the original string) but while with hover + selection we could support it (selection replaces string, hover doesn't), I'm not sure how we could support it with hover == selection.
Yeah.  We could special-case search suggestions.  That seems pretty straightforward.
(In reply to Drew Willcoxon :adw from comment #31)
> Yeah.  We could special-case search suggestions.  That seems pretty
> straightforward.

It will only work until suggestions are the last results in the popup though. If there's anything below them, it will override the text.
> > Yeah.  We could special-case search suggestions.  That seems pretty
> > straightforward.
> 
> It will only work until suggestions are the last results in the popup
> though. If there's anything below them, it will override the text.

You could just TAB to the one-offs though.

More generally, after playing with the build I have to say that it feels more intuitive than the current behavior. It also seems better than Chrome's, which suffers from the same issues in my view. Marco, apart from the search suggestions idea, which I agree is useful, I don't understand whether you have any other specific concerns apart from a potential adverse reaction to change from some users ("omgchange!").
(In reply to Panos Astithas [:past] from comment #33)
> You could just TAB to the one-offs though.

Sure, because I know about that, otherwise it'd be undiscoverable. Note that I completely agree that tab should go to one-offs, as well as that left/right should move through them. This all sounds great to me, it's the part of the patch that I like!
But I still prefer to use one-offs with the mouse and I still expect most Windows users to do that.
Actually, we have UITelemetry about how many users interact with one offs through keyboard or mouse, so we could check.

(In reply to Panos Astithas [:past] from comment #33)
> I don't understand
> whether you have any other specific concerns apart from a potential adverse
> reaction to change from some users ("omgchange!").

The omgchange reaction involves only the TAB behavior change. I have no doubt we'll have that reaction, but it's not my main concern.

Here I'm discussing the mouseover-selects change. My concerns are specifically:

1. this breaks well consolidated users habits. Especially on Windows, it's quite common to mixup keyboard and mouse interaction, even at the same time: the user is typing with left hand, and selecting a result with right hand. The most common users we gain are either Windows users or Chrome users, based on marketshare. They are not used to this new behavior.
2. I'm still not sure how we are protecting users from overwriting what they are typing, if they inadvertently touch the mouse or the touchpad (that is just under their hands while typing). This is not a subjective preference, it's an annoying bug.
3. It makes plausible search features, like comment 30, depend on undiscoverable interaction (need to TAB)
4. I don't object to changing TAB behavior, but be ready for vocal complains, also inside Mozilla. Based on bugs I saw in the last years, we have a large population using TAB rather than DOWN, and I can understand the reason, it's just under your fingers, you don't have to reach it on purpose.

In my opinion, we need to address point 2 at a minimum, cause that's not a subjective matter.
What I suggest, if possible, is to split this work into 2 parts, tracked by 2 separate dependencies:
 * part 1 implements TAB and LEFT/RIGHT changes. These are positive imo, even if I'm still sure a minor vocality will rise.
 * part 2 implements "real" selection on mouseover. Imo we need to add filtering of mouse events, to prevent accidental overwrites.

Landing these separately will also allow to better handle feedback and regression reports.
(In reply to Panos Astithas [:past] from comment #33)
> It also seems better than Chrome's, which suffers from the same issues in my view.

I disagree that it suffers from the same issues. That interaction clearly splits mouse and keyboard, allowing for interesting combinations (key to an entry and click on a one-off). In no case the browser overwrites what the user is typing with the keyboard, unless he selects something with the keyboard itself. The separation is clear and not error prone, there's no possibility to misinterpret the user action.
The only downside is that there are 2 hilighted results (dark and light), but in our approach we are still introducing an alternate selection where the element is greyed out (per comment 21).
I had a long conversation with Stephen about this behavior yesterday (so my context is coming from that - I didn't read the entire comment thread)

Where we ended up was that it would be best to completely separate the mouse hover interactions from the keyboard navigation (which is, if I understand correctly, what Marco is arguing for).
There are some imperfect edge cases in either scenario, but the ones that happen when the two modes are split seem much less common and less severe.

Essentially, the only thing that moving the mouse should do (in either Awesomebar and search bar) is to provide feedback that an item can be clicked. It should not change the input in the URL bar or the keyboard cursor. Stephens mockup shows this pretty well: https://people-mozilla.org/~shorlander/mockups-interactive/awesomebar-results/awesomeBar-results-tabOrdering-02.html

The other piece that is discussed in this bug (making the one-off buttons a single tab-target) still makes sense to do IMO.
Flags: needinfo?(shorlander)
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

https://reviewboard.mozilla.org/r/106496/#review109218

clearing review per comment 36. It would still be nice if we'd have 2 separate bugs, one for the mousehover and one for the tab handling change, if it doesn't add too much work to a bug that already got some wasted work (and I'm sorry about that).
Attachment #8829366 - Flags: review?(mak77)
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Sorry for commenting here so late.

From testing the patch (my testing was focused on the searchbar), I found several issues, and I'm not entirely sure what's side effects of the intended changes and what's bugs in the patch:

- typing in the searchbar and then pressing <tab> closes the search panel and moves the focus to the web page.

- <tab> jumps to the first one-off button only if a suggestion has been previously selected from the keyboard

- the 'Change Search Settings' button is no longer keyboard accessible. (edit: actually, it's accessible using the left/right arrows once a one-off has been selected. I assume it's due to a previous patch changing the settings button to an icon like in the url bar)

- it's not possible to select a suggestion with the keyboard and then use a one-off button on it (pressing <tab> to jump to the one-offs de-selects the suggestion and reverts to the user-typed text), unless you discover the 'alt' modifier.

- the selection of a suggestion by hovering doesn't fully selects: if I then press up or down, the selection moves from the previously keyboard-selected item rather than from the blue item previously hovered.

- I'm a bit surprised by the use of the left/right arrows to move among one-off buttons, because when we initially designed the keyboard interactions in this panel, we took as a constraint that whatever we did in the panel, given that the cursor was still in the textfield and typing still worked there, the left/right arrows should still let the user move the cursor.
I find the behavior implemented by this patch a bit more discoverable than the current one, but it's at the cost of breaking user habits and navigation in the text field, so I'm not convinced it's worth it. Expect omgchange reactions if this lands.
Attachment #8829366 - Flags: review?(florian)
Blocks: 1292630
Blocks: 1337003
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Marco, the only non-search change this really makes is to rename mouseSelectedIndex to mousedOverIndex, since now moving the mouse over an item should not select it, according to comment 36.
Attachment #8829366 - Flags: review?(mak77)
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Florian, this has the handleKeyPress and advanceSelection changes from the previous patch, and it makes the distinction between the selected button and the visually selected button even sharper by adding a new visuallyselected attribute separate from the selected attributed.

> - typing in the searchbar and then pressing <tab> closes the search panel
> and moves the focus to the web page.

Fixed

> - <tab> jumps to the first one-off button only if a suggestion has been
> previously selected from the keyboard

Fixed

> - the 'Change Search Settings' button is no longer keyboard accessible.
> (edit: actually, it's accessible using the left/right arrows once a one-off
> has been selected. I assume it's due to a previous patch changing the
> settings button to an icon like in the url bar)

I think that's how it has to work now.  The settings button is treated as part of the one-offs tab group.  It's just another one-off button basically I guess.

> - it's not possible to select a suggestion with the keyboard and then use a
> one-off button on it (pressing <tab> to jump to the one-offs de-selects the
> suggestion and reverts to the user-typed text), unless you discover the
> 'alt' modifier.

This patch restores that behavior in the search bar, but once you select a one-off, you have to press left/right to select another one-off.  Pressing tab at that point moves the focus out.
Attachment #8829366 - Flags: review?(florian)
Stephen or Philipp, could you please try the build here and let me know what you think?

https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-968c3fadff3a8d76c97e2e98ed6e4f15184adccc/
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
(In reply to Drew Willcoxon :adw from comment #41)

(First, I have to note that it's difficult for me to be neutral on this, because any change to this is almost guaranteed to break my muscle memory and annoy me as a user.)

> Florian, this has the handleKeyPress and advanceSelection changes from the
> previous patch, and it makes the distinction between the selected button and
> the visually selected button even sharper by adding a new visuallyselected
> attribute separate from the selected attributed.

I'm not sure if this is what you are referring to here, but I find the gray background on hovered one-off buttons confusing (why is the blue-selected one-off very visible, but the gray-selected item is the one that is displayed in the 'Search <name>' header?) and inconsistent (we have only one color of selection for the suggestions; when I hover a suggestion it become blue).

> > - the 'Change Search Settings' button is no longer keyboard accessible.
> > (edit: actually, it's accessible using the left/right arrows once a one-off
> > has been selected. I assume it's due to a previous patch changing the
> > settings button to an icon like in the url bar)
> 
> I think that's how it has to work now.  The settings button is treated as
> part of the one-offs tab group.  It's just another one-off button basically
> I guess.

That's fine for the awesomebar panel because the settings button has the same size and is on the same row as the one-off buttons, but on the search bar it's visually a separate item. I don't think that's an acceptable change (I would be surprised if you encounter users who think of pressing the left/right arrows to reach that settings item). Same concern applies to the open search items.

Also, pressing the up arrow from the input field should select the last item (ie. the settings button), or at least an item on the last line. With the patch applied this now selects the last suggestion.

> > - it's not possible to select a suggestion with the keyboard and then use a
> > one-off button on it (pressing <tab> to jump to the one-offs de-selects the
> > suggestion and reverts to the user-typed text), unless you discover the
> > 'alt' modifier.
> 
> This patch restores that behavior in the search bar, but once you select a
> one-off, you have to press left/right to select another one-off.  Pressing
> tab at that point moves the focus out.

That's probably usable, but will irk me personally each time I'll press tab a second time and it'll close the search panel. I'm not really sure why we need to change the existing tab behavior in the search bar. I initially thought it was for consistency with the awesomebar, but the tab key behaves differently there.

Playing a bit with the awesomebar panel, the arrow key behaviors seem to match my expectations. But I keep being surprised by the 'tab' key on the last suggestion that selects the first suggestion (I would expect that to select the first one-off button). Is there a good reason for the tab key to not behave like the down arrow in the awesomebar panel?
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

(removing the review flag for now, as I don't think it makes sense for me to have a detailed look at the code until there's general agreement about the behavior we want.)
Attachment #8829366 - Flags: review?(florian)
Thanks Florian, your pointing out the places where behavior is weird or inconsistent is very helpful.

> why is the blue-selected one-off very visible, but the gray-selected item
> is the one that is displayed in the 'Search <name>' header?

So do you think the header should continue to show the blue-selected name?  I don't think I agree, since the name shows what you'll search with when you click, which seems like a good thing.  But I don't feel strongly about it.  Or should the blue selection disappear while you're mousing over another one-off?

This is one of those details that comes up during implementation that Stephen or Philipp should address probably.

> we have only one color of selection for the suggestions; when I hover a
> suggestion it become blue.

Yeah... the patch doesn't touch the tree autocomplete implementation, only the listbox.  Ideally it would make the tree work the same as the listbox and probably should before it lands.  Would that be OK with you?

> That's fine for the awesomebar panel because the settings button has the
> same size and is on the same row as the one-off buttons, but on the search
> bar it's visually a separate item. I don't think that's an acceptable change
> (I would be surprised if you encounter users who think of pressing the
> left/right arrows to reach that settings item). Same concern applies to the
> open search items.

Fair point, let's make these tabbable too then.  (This is another detail that UX would ideally address, but I'm OK with making these tabbable without their input.)

> Also, pressing the up arrow from the input field should select the last item
> (ie. the settings button), or at least an item on the last line. With the
> patch applied this now selects the last suggestion.

Good point, thanks for catching that.  It behaves right in the urlbar (or at least closer to right) but not the search bar.

> That's probably usable, but will irk me personally each time I'll press tab
> a second time and it'll close the search panel. I'm not really sure why we
> need to change the existing tab behavior in the search bar. I initially
> thought it was for consistency with the awesomebar, but the tab key behaves
> differently there.

We can't really change the tab behavior in the urlbar since people complained last time we tried (hence the disabletab attribute), and earlier in this bug we decided to make the search bar consistent with the urlbar, so I guess that means the search bar needs the same tab behavior.

> Playing a bit with the awesomebar panel, the arrow key behaviors seem to
> match my expectations. But I keep being surprised by the 'tab' key on the
> last suggestion that selects the first suggestion (I would expect that to
> select the first one-off button). Is there a good reason for the tab key to
> not behave like the down arrow in the awesomebar panel?

No, I think you're right that it should tab down to the one-offs.
(In reply to Drew Willcoxon :adw from comment #45)
> We can't really change the tab behavior in the urlbar since people
> complained last time we tried (hence the disabletab attribute), and earlier
> in this bug we decided to make the search bar consistent with the urlbar, so
> I guess that means the search bar needs the same tab behavior.

Comment 36 seems to point out we *want* to change the tab behavior, so that it goes to one-offs? Also because alt+down is even more undiscoverable and alt+tab makes things even more confusing being used by the OS :(
If feasible, it would be great to put the tab behavior behind a pref, that would allow us to toggle between "tab goes to oneoffs" to "tab is the same as down, alt+down goes to oneoffs", and would give annoyed users a path outside of this. And clearly searchbar and locationbar should do the same. Yes, unfortunately that means we should have tests for both behaviors :(
(In reply to Drew Willcoxon :adw from comment #45)
> Thanks Florian, your pointing out the places where behavior is weird or
> inconsistent is very helpful.
> 
> > why is the blue-selected one-off very visible, but the gray-selected item
> > is the one that is displayed in the 'Search <name>' header?
> 
> So do you think the header should continue to show the blue-selected name?

No, I'm more thinking we should either support the gray thing on the autocomplete tree (like you proposed in your next reply), or not do the gray thing at all. (also: it's possible that the nuance of gray that is used now is a bit too light; that may contribute to the feeling).
 
> Or should the blue selection disappear while you're mousing over another
> one-off?

Maybe... but then why aren't we just making the hovered one-off blue?

> > Also, pressing the up arrow from the input field should select the last item
> > (ie. the settings button), or at least an item on the last line. With the
> > patch applied this now selects the last suggestion.
> 
> Good point, thanks for catching that.  It behaves right in the urlbar (or at
> least closer to right) but not the search bar.

From my point of view, the main difference between the awesomebar and the searchbar panels is that in the awesomebar we expect the one-off buttons to fit in a single line for most users, whereas in the search panel we expect most users to have 2 or 3 rows of one-offs.

If we wanted to have a behavior that felt really natural, the up/down keys should move to the higher/lower row of one-off. I'm not convinced that's worth the implementation effort though.

> > That's probably usable, but will irk me personally each time I'll press tab
> > a second time and it'll close the search panel. I'm not really sure why we
> > need to change the existing tab behavior in the search bar. I initially
> > thought it was for consistency with the awesomebar, but the tab key behaves
> > differently there.
> 
> We can't really change the tab behavior in the urlbar since people
> complained last time we tried (hence the disabletab attribute),

We had in the searchbpanel the same tab behavior that we currently have in the urlbar, and we changed it so that the one-off buttons would become keyboard accessible (and actually easy enough to use with the keyboard that it would be nicer to use these buttons than to type 'wiki' in the query string). People complained. That's not surprising whenever we change keyboard interactions.

> and earlier
> in this bug we decided to make the search bar consistent with the urlbar, so
> I guess that means the search bar needs the same tab behavior.

Or the other way, as Marco says in comment 46.
Drew, I felt the comment Marco and I left were mostly pointing out issues on details, but not really proposing a path forward, so I briefly discussed this with him today.

I'm trying to summarize here (Marco will correct me if I missed something) :
- <tab> seems to be the part where it's difficult to agree on what the most desirable behavior is.
- Marco and I seem to think the behavior that would make the most sense would be to implement in the awesomebar the tab behavior we have in the searchbar. But such a change is likely to cause user outcry, so we would probably have to make it controlled by a pref (and we could also add a pref for the tab behavior in the searchbar while we are at it).
- The hover behavior, and the arrow key behaviors, especially in the awesomebar panel, seem to be the most important pieces in this bug, so maybe we should focus on these parts here, and move handling of the tab key to its own bug, so that discussion related to tab doesn't block the rest of this work.
- We may want to get some data about the tab thing before making a final decision about it. Maybe getting some telemetry to know how much tab is currently used in the urlbar; or maybe a Shield study (if so, making the tab behavior pref-controlled would help). We doubt we'll ever have enough time to actually run that study though. So deciding to just leave the tab behavior unchanged is also an option.

We think the way we provided feedback in this bug must have been frustrating, and we are both sorry about that.
(In reply to Florian Quèze [:florian] [:flo] from comment #48)
> We think the way we provided feedback in this bug must have been
> frustrating, and we are both sorry about that.

No, not at all, thanks for all your feedback so far.  To be honest I think all the problems discussed in this bug and elsewhere about the one-offs point to the fact that they are a complicated piece of design and UI, maybe overly so.
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Clearing per previous discussion. The plan discussed with Florian should allow at least to unblock part of this, and make it less frustrating.
Attachment #8829366 - Flags: review?(mak77)
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
This one feels pretty good I think.  I don't see any obvious problems but maybe I'm overlooking something.

It adds the gray hover state to the searchbar by extending autocomplete-treebody.  autocomplete-treebody selects the moused-over row, so the new binding in search.xml prevents that behavior.

Like one of the previous patches, this also gets rid of the idea of "visually selected" and replaces it with a single method that updates the mouseover state in the one-offs binding and :hover CSS selectors.  I think it's a necessary change to avoid confusion now that mouseover != selected in any way.

Finally I think all the key handling is correct.  It doesn't touch tab handling, as we've talked about.  It treats compact vs. non-compact a little differently.  For non-compact, the settings button is treated as a row of its own, so the up/down arrow keys move to it and the engine buttons separately, and within the engine buttons, the left/right arrow keys move only within the engine buttons.  I agree with you Florian that it would be nice to treat each row of buttons as an up/down arrow key target, but as you say, that's harder, so this doesn't do that.

Still needs tests, but I'd like to get feedback or initial review on this first.
Florian, I refreshed my tree again between this patch and the previous one, so Review Board's interdiff won't be useful.  Sorry about that.
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

I'm going to give feedback based on trying the patch, I haven't reviewed the code. Overall I like this.

Here are a few issues I noticed:
- selecting a one-off button using the keyboard doesn't show the "Search with <engine name>" label. (applies both to the searchbar and urlbar)
- Hovering a keyboard-selected one-off button turns the blue selection background into a light gray hover background; this feels like hover-deselects. I think we should keep the blue background in that case. (applies both to the searchbar and urlbar)
- The left/right arrow keys on the "Change Search Settings" button of the searchbar select a one-off button (surprisingly the left arrow doesn't select the last one-off button). I think that should move the cursor within the textfield.
- when there are open search items, pressing the down arrow while a one-off button is selected should select the first open search item, not the settings button.
- like for the search settings button, I don't think left/right arrow keys on the open search items should select a one-off.
Attachment #8829366 - Flags: review?(florian) → feedback+
Any thoughts on why macOS says the trybuild is damaged and can't be opened? https://cl.ly/22321c3L3S0L
(In reply to Stephen Horlander [:shorlander] from comment #56)
> Any thoughts on why macOS says the trybuild is damaged and can't be opened?
> https://cl.ly/22321c3L3S0L

Do you happen to be using a try build for the first time on a new Mac? If so, you should look at the gatekeeper settings in the OS' system settings.
Other than what Florian said, I can describe what I did to successfully run the try build:

1. Download https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-f5c6fe41c7d1c9d144332bdb4bfc80b089a53df6/try-macosx64/firefox-55.0a1.en-US.mac.dmg

2. Run it from Terminal: /Volumes/Nightly/Nightly.app/Contents/MacOS/firefox-bin -no-remote -foreground -P tmp
Ah, thanks! It is a new Mac. They apparently removed the "Anywhere" option from Security if anyone else runs into this: https://cl.ly/3l2n2F3X1Q0W
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Florian, the interdiff between this and the previous patch should be OK this time.  This addresses your comment 55.

I'm asking for feedback on this so that you can hopefully give a final OK on the interactions, and then I'll work on tests and post another patch for review.  But if the interactions are OK, please also feel free to actually review the code if you'd like, instead of waiting for the next patch.  It's in a final, reviewable state, it just doesn't include tests yet.
Attachment #8829366 - Flags: review?(florian) → feedback?(florian)
Thanks Drew! I just looked over the try build with Florian and Marco.
The build looks great and I think we are 98% there.

The one thing we should change I think is the behavior of the up/down arrows in the one-off buttons. Pressing up/down should also cycle through the one-off buttons (instead of skipping that section):
- Up should do the same as left currently does (previous one-off)
- Down should do the same as right currently does (next one-off)

Left and right can keep the same behavior as in your latest build.

There are two reasons for that:
1) It is a slightly unexpected mode switch when cycling through results and the navigation direction suddenly switches from vertical to horizontal
2) It is more consistent with the current behavior of our search bar
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Perfect!
Attachment #8829366 - Flags: ui-review+
There's already a slight touch of bitrot caused by bug 1348574.
(In reply to Drew Willcoxon :adw from comment #64)

Thanks for the updated patch.

I tested it and found only one very tiny issue:
STRs:
1. Type something in the searchbar, the text in the middle of the panel says "Search for <query> with:"
2. Press <tab>, the text now says "Search <name of first one off engine>"
3. Hover the second one-off button, the text now says "Search <name of second one off engine>"
4. Move the mouse away.

Expected result:
The string should be back to step 2: "Search <name of first one off engine>"

Actual result:
The string is back to what it was at step 1: "Search for <query> with:"


I haven't done a review of the code yet, but am hoping to do it soon.
Nice catch.
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

https://reviewboard.mozilla.org/r/106496/#review128876

Sorry for the delay here :-(. Needing to reset the text field when pressing "down" on the last suggestion is the reason for the r-, the rest is edge-cases that could be follow-ups.

::: browser/components/search/content/search.xml:1880
(Diff revision 6)
>  
>          @param aForward
>                 If true, the index is incremented, and if false, the index is
>                 decremented.
> +        @param aIncludeNonEngineButtons
> +               If true, non-engine buttons are included.

Could you please rephrase this comment to use different words than the words used in the parameter name? More specifically, it would be nice if the comment explained (maybe with examples) what "non-engine buttons" means. Is it the open search engines? Is it the settings button? Is it the dummy buttons?

::: browser/components/search/content/search.xml:1968
(Diff revision 6)
> +            return true;
> +          }
>  
> -          // Tab cycles through the one-offs and moves the focus out at the end.
> -          // But only if non-Shift modifiers aren't also pressed, to avoid
> -          // clobbering other shortcuts.
> +          // Handle the Tab key, but only if non-Shift modifiers aren't also
> +          // pressed to avoid clobbering other shortcuts (like the Alt+Tab
> +          // browser tab switcher).

With your patch applied, alt+tab does nothing, but alt+shift+tab moves the selection up in the bottom part of the search panel, and then moves the focus to the location bar when pressing alt+shift+tab again after the first one off button got selected. This seems inconsistent/unintended.

::: browser/components/search/content/search.xml:1983
(Diff revision 6)
> -                  this.advanceSelection(false, true, true);
> -                  stopEvent = true;
> -                }
> +          }
> -              } else {
> -                let firstButtonSelected =
> -                  this.selectedButton &&
> +
> +          if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_UP) {
> +            if (event.altKey) {

Another one in the edge-case category: if the settings button is selected and I press alt+up, the one-off button before last gets selected; I would expect the last one-off button to get selected.

::: browser/components/search/content/search.xml:2027
(Diff revision 6)
> -                if (!allowEmptySelection) {
> +              return false;
> -                  this.popup.selectedIndex = -1;
> -                  stopEvent = true;
> -                }
> +            }
> -                if (this.textbox && typeof(textboxUserValue) == "string") {
> -                  this.textbox.value = textboxUserValue;
> +            if (this.popup.selectedIndex == numListItems - 1) {
> +              // Moving down from the last item in the list to the buttons.

In this case we need to reset the text in the textfield, otherwise using the down arrow to get to the first one off button always searches the text of the last suggestion.

Note: pressing tab or alt+down shouldn't reset the text field (that works correctly in the current patch, but let's not regress it :-)).

::: browser/components/search/content/search.xml:2035
(Diff revision 6)
> -                // accessibility events.
> -                this.advanceSelection(true, true, true);
> -              } else {
> -                let buttons = this.getSelectableButtons(true);
> +              let buttons = this.getSelectableButtons(true);
> -                let lastButtonSelected =
> -                  this.selectedButton &&
> +              if (this.selectedButtonIndex == buttons.length - 1) {
> +                // Moving down from the buttons back up to the top of the list.

Rather than selecting the first suggestion, this used to clear the selection, and the user would have to press the down arrow again to get the first suggestion selected.

With the current patch, if the user presses down once, the first suggestion gets selected, and then there's no way to clear the selection (pressing "up" to undo the "down" will select the settings button, which seems a bit unexpected).

I'm not sure if this needs fixing or not, but I figured I would mention it just in case this wasn't intentional.

In the location bar it is fine because we automatically select the first item, which is always the user-typed string, rather than a suggestion.

::: browser/themes/osx/searchbar.css:220
(Diff revision 6)
> +.addengine-item:hover {
> +  background-color: hsla(0, 0%, 0%, 0.06);
> +  color: inherit;
> +}
> +
> +.searchbar-engine-one-off-item[selected]:not(.dummy) {

We never set the "selected" attribute on dummy buttons, do we? Or is this needed to make the selector more specific than ".searchbar-engine-one-off-item:not(.dummy):hover" ? If so, I think I would prefer adding a :not([selected]) there.
Attachment #8829366 - Flags: review?(florian) → review-
(Oh, looks like you posted your review while I was posting this new patch.  I'll paste the comment I was typing up for it anyway.  It might be out of date based on your comment.)

Working on tests made me realize there were a couple of broken things:

* Your typed value should be restored once you key down into the one-offs.

* In the search bar, keying all the way down should result in no selection in either the one-offs or the results list (and then when you key down again, the selection should wrap back around to the first result).  Similar for keying up.

Which means I needed to restore the allowEmptySelection and textboxUserValue parameters to the key-handling method.  Can't get rid of those after all.  (Could get rid of textboxUserValue if there were a nsIAutoCompleteController.revertText() method though.  That method already exists internally...)
(In reply to Florian Quèze [:florian] [:flo] from comment #71)
> Needing to reset the text field when pressing "down" on the last
> suggestion is the reason for the r-

That should be fixed now.
(In reply to Florian Quèze [:florian] [:flo] from comment #71)
> Could you please rephrase this comment to use different words than the words
> used in the parameter name?

Done

> With your patch applied, alt+tab does nothing, but alt+shift+tab moves the
> selection up in the bottom part of the search panel, and then moves the
> focus to the location bar when pressing alt+shift+tab again after the first
> one off button got selected. This seems inconsistent/unintended.

Apparently when you press Shift-Option-Tab on macOS, event.altKey is false, which is why this was happening.  We were ending up in the tab-handling part of that method.  I played around with it and found that AltGraph remains true, so I changed this to use event.getModifierState() and added a check for AltGraph.

> Another one in the edge-case category: if the settings button is selected
> and I press alt+up, the one-off button before last gets selected; I would
> expect the last one-off button to get selected.

I think the right thing to do here is nothing at all (so I guess I disagree with what you were expecting), so that's what this latest patch does.

> In this case we need to reset the text in the textfield, otherwise using the
> down arrow to get to the first one off button always searches the text of
> the last suggestion.

Fixed by the previous patch

> Rather than selecting the first suggestion, this used to clear the
> selection, and the user would have to press the down arrow again to get the
> first suggestion selected.

Fixed by the previous patch

> ::: browser/themes/osx/searchbar.css:220
> (Diff revision 6)
> > +.addengine-item:hover {
> > +  background-color: hsla(0, 0%, 0%, 0.06);
> > +  color: inherit;
> > +}
> > +
> > +.searchbar-engine-one-off-item[selected]:not(.dummy) {
> 
> We never set the "selected" attribute on dummy buttons, do we? Or is this
> needed to make the selector more specific than
> ".searchbar-engine-one-off-item:not(.dummy):hover" ? If so, I think I would
> prefer adding a :not([selected]) there.

I might not be following you, but that's not possible because then when you hover over dummy buttons, they get the hover styling, which of course isn't right.  So I left this part the way I had it.

(Still working on tests BTW.)
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

https://reviewboard.mozilla.org/r/106496/#review128892

::: browser/components/search/content/search.xml:2011
(Diff revisions 7 - 8)
>            if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_UP) {
>              if (event.altKey) {
> -              // Keep a secondary "alt" selection in the list, and move the
> -              // selection up within the buttons.
> +              // If a result in the list is selected, keep it as a secondary
> +              // "alt" selection and move the selection up within the buttons.
> +              // Otherwise do nothing.
> +              if (this.popup.selectedIndex >= 0) {

Except this isn't right either, sigh... I've fixed this locally to be:

if (!this.selectedButton || this.selectedButton.engine) {
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

Clearing this until I get all tests working.
Attachment #8829366 - Flags: review?(florian)
All right, all tests pass locally with this.
try looks good too.
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

https://reviewboard.mozilla.org/r/106496/#review129512

(almost r+) Are you still working on more tests? (eg. to cover the keyboard behaviors in the awesomebar panel)

(In reply to Drew Willcoxon :adw from comment #76)

> > With your patch applied, alt+tab does nothing, but alt+shift+tab moves the
> > selection up in the bottom part of the search panel, and then moves the
> > focus to the location bar when pressing alt+shift+tab again after the first
> > one off button got selected. This seems inconsistent/unintended.
> 
> Apparently when you press Shift-Option-Tab on macOS, event.altKey is false,
> which is why this was happening.  We were ending up in the tab-handling part
> of that method.  I played around with it and found that AltGraph remains
> true, so I changed this to use event.getModifierState() and added a check
> for AltGraph.

Actually, I now noticed that this problem also existed without your patch; sorry.
I'm ok with taking the hack, but it sounds like we should file a bug for the underlying issue.

> 
> > Another one in the edge-case category: if the settings button is selected
> > and I press alt+up, the one-off button before last gets selected; I would
> > expect the last one-off button to get selected.
> 
> I think the right thing to do here is nothing at all (so I guess I disagree
> with what you were expecting), so that's what this latest patch does.

Why do you think that?

My understanding here is that alt+up/down should cycle through the one-off buttons. It does it when a suggestion is selected, or when there's no selection. So I don't think the settings buttong being selected should affect the behavior of this key binding.


> > ::: browser/themes/osx/searchbar.css:220
> > (Diff revision 6)
> > > +.addengine-item:hover {
> > > +  background-color: hsla(0, 0%, 0%, 0.06);
> > > +  color: inherit;
> > > +}
> > > +
> > > +.searchbar-engine-one-off-item[selected]:not(.dummy) {
> > 
> > We never set the "selected" attribute on dummy buttons, do we? Or is this
> > needed to make the selector more specific than
> > ".searchbar-engine-one-off-item:not(.dummy):hover" ? If so, I think I would
> > prefer adding a :not([selected]) there.
> 
> I might not be following you, but that's not possible because then when you
> hover over dummy buttons, they get the hover styling, which of course isn't
> right.  So I left this part the way I had it.

You currently have these selectors:

.searchbar-engine-one-off-item:not(.dummy):hover
and
.searchbar-engine-one-off-item[selected]:not(.dummy)

I was proposing changing them to:

.searchbar-engine-one-off-item:not([selected]):not(.dummy):hover
and
.searchbar-engine-one-off-item[selected]

::: browser/components/search/content/search.xml:1994
(Diff revisions 6 - 9)
>            // Handle the Tab key, but only if non-Shift modifiers aren't also
>            // pressed to avoid clobbering other shortcuts (like the Alt+Tab
>            // browser tab switcher).
>            if (event.keyCode == KeyEvent.DOM_VK_TAB &&
> -              !event.altKey &&
> -              !event.ctrlKey &&
> +              !event.getModifierState("Alt") &&
> +              !event.getModifierState("AltGraph") &&

I think this needs a comment explaining what you said about it in comment 76. Also, is this a platform bug we should file?

::: browser/components/search/test/browser_searchbar_keyboard_navigation.js
(Diff revision 9)
>    is(textbox.selectedButton, oneOffs[0],
>       "the first one-off button should be selected");
> -  is(searchPopup.selectedIndex, 0, "first suggestion should still be selected");
> +  is(searchPopup.selectedIndex, -1, "no suggestion should be selected");
>  
> -  // After pressing down, the second suggestion should be selected,
> +  // After pressing down, the second one-off should be selected.
> -  // and the first one-off still selected.

After this, should we press right and check that the 3rd one-off is selected, then left and check the second one-off is selected again?
Attachment #8829366 - Flags: review?(florian) → review-
(In reply to Florian Quèze [:florian] [:flo] from comment #82)
> (almost r+) Are you still working on more tests? (eg. to cover the keyboard
> behaviors in the awesomebar panel)

Wasn't planning on it to be honest...
(In reply to Florian Quèze [:florian] [:flo] from comment #82)
> (almost r+) Are you still working on more tests? (eg. to cover the keyboard
> behaviors in the awesomebar panel)

I remembered there's already a test for the urlbar one-offs, so this adds some left and right key checks to it.

> My understanding here is that alt+up/down should cycle through the one-off
> buttons. It does it when a suggestion is selected, or when there's no
> selection. So I don't think the settings buttong being selected should
> affect the behavior of this key binding.

Done

> You currently have these selectors:
> 
> .searchbar-engine-one-off-item:not(.dummy):hover
> and
> .searchbar-engine-one-off-item[selected]:not(.dummy)
> 
> I was proposing changing them to:
> 
> .searchbar-engine-one-off-item:not([selected]):not(.dummy):hover
> and
> .searchbar-engine-one-off-item[selected]

Done

> I think this needs a comment explaining what you said about it in comment 76.

Done

> After this, should we press right and check that the 3rd one-off is
> selected, then left and check the second one-off is selected again?

Done
Comment on attachment 8829366 [details]
Bug 1295458 - Rework key and mouse handling for the one-off search buttons.

https://reviewboard.mozilla.org/r/106496/#review131320

Thanks!
Attachment #8829366 - Flags: review?(florian) → review+
Rebased on current tree.  One last try push before landing.
Try looks good, styling/themes on all three platforms looks good.  Landing!
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb0c3ac25762
Rework key and mouse handling for the one-off search buttons. r=florian
https://hg.mozilla.org/mozilla-central/rev/bb0c3ac25762
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1356641
Depends on: 1357533
Depends on: 1360170
Depends on: 1360183
Depends on: 1360188
Depends on: 1360208
Drew, is there any equivalent of Alt+Up/Down keys on Ubuntu that could get me to navigate only through the one off buttons? (this key combination is properly working on Windows 10 and Mac OS X 10.12 but not on Ubuntu 16.04).
Flags: needinfo?(adw)
It looks like you have to press Shift-Alt, not just Alt, on Linux.  But looking at the code, I don't know why that should be the case.  I'll file a new bug to look into it.
Flags: needinfo?(adw)
See Also: → 1363570
Depends on: 1366625
Blocks: 1290424
Depends on: 1371331
Depends on: 1392582
Depends on: 1389907
No longer depends on: 1366625
Regressions: 1366625
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: