Created attachment 201273 [details] [review]
searchDisplay, viewSelector: implement keynav for Applications and Search

Review of attachment 201273 [details] [review]:

Notes from trying this out:

 * For some reason, the center element of the search display is selected when you go up/down.
 * We need to scrap the separate up/down action on the entry.
 * contactDisplay elements aren't focusable.

Created attachment 201336 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

(In reply to comment #2)
>  * contactDisplay elements aren't focusable.

Addressed on this new patch.

Created attachment 201337 [details] [review]
searchDisplay, viewSelector: add default result activation

Adds a way to highlight and activate the first search result when hitting
enter on the search entry.

--

(In reply to comment #2)
>  * We need to scrap the separate up/down action on the entry.

Yep, this patch gets rid of that.

We might want to use a different highlight style for the default result other
than "selected" though.

Created attachment 201338 [details] [review]
search: remove selection mechanism

Now that we are using standard St keyboard navigation we don't need this
specific keynav mechanism for search results.

--

And this cleans up all the now unused custom keynav code.

(In reply to comment #2)
>  * For some reason, the center element of the search display is selected when
> you go up/down.

So, this is unrelated to the issue at hand here. Now, if we agree on some behavior I might give it a try and implement it, but it's really another bug.

*** Bug 640779 has been marked as a duplicate of this bug. ***

Created attachment 202928 [details] [review]
search: remove selection mechanism

--

Rebased on master. It just had a small conflict due to the new Lang.Class
syntax.

Created attachment 203278 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

--
Rebased.

Created attachment 203279 [details] [review]
searchDisplay, viewSelector: add default result activation

--
Rebased.

Created attachment 203280 [details] [review]
search: remove selection mechanism

--
Rebased.

*** Bug 668190 has been marked as a duplicate of this bug. ***

Created attachment 206331 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

--
Added some more smarts when using Up/Down arrow keys. Rebased on master.

Created attachment 206332 [details] [review]
searchDisplay, viewSelector: add default result activation

--
Rebased.

Created attachment 206333 [details] [review]
search: remove selection mechanism

--
Rebased.

 5 files changed, 99 insertions(+), 183 deletions(-)

Look! More functionality with less code!

Trying this out again:

 * Having two different highlighting styles is confusing. I like the background one more than the outline one.
 * Pressing "up" from the first row to go back to the search entry does not work.

(In reply to comment #6)
> (In reply to comment #2)
> >  * For some reason, the center element of the search display is selected when
> > you go up/down.
> 
> So, this is unrelated to the issue at hand here. Now, if we agree on some
> behavior I might give it a try and implement it, but it's really another bug.

If we have a set of rows like:

 APPLICATIONS
   X |X| X  X  X

 RECENT ITEMS
   R  R  R  R  R

then pressing down should go to the second R. As a more general rule, we should go to the item with the least horizontal distance. I'm quite sure that StBoxLayout.navigate_focus already implements something like this. Perhaps we can port iconGrid to be based on that sort of thing (or use the new gjs gobject-inheritance to implement navigate_focus in iconGrid).

(In reply to comment #17)
>  * Having two different highlighting styles is confusing. I like the background
> one more than the outline one.

I'd like some designer input here: 

1) do we need to visually highlight the default target that activates on enter while the search entry is focused? Or could we get away with activating the first result without a visual highlight?

2) what style do we want for keynav focus? I think it should be consistent all over the shell at least.

I'll work on Jasper's remaining comments.

(In reply to comment #18)
> (In reply to comment #17)
> >  * Having two different highlighting styles is confusing. I like the background
> > one more than the outline one.
> 
> I'd like some designer input here: 
> 
> 1) do we need to visually highlight the default target that activates on enter
> while the search entry is focused?

Yes, I'd say we do. It's an important cue.

>Or could we get away with activating the
> first result without a visual highlight?
>
> 2) what style do we want for keynav focus? I think it should be consistent all
> over the shell at least.

The existing keynav style that is used for the search results is fine. We can use the same one for the application picker.

Created attachment 207307 [details] [review]
st-container: Use absolute coordinates for focus navigation

For arrow keys navigation we must use the actual widget we are coming from
instead of a parent of it which might not exist.

This requires us to use absolute coordinates to compare widgets since we no
longer have the guarantee that the widgets we are comparing are siblings.

Created attachment 207308 [details] [review]
st-container: Fix actor filtering for directional keynav

This allows us to do directional keyboard navigation when there's no actor in
the horizontal or vertical strip extending from the origin actor but there are
other actors to the sides of that strip that could still be used as targets.

Created attachment 207309 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

Created attachment 207310 [details] [review]
searchDisplay, viewSelector: add default result activation

Created attachment 207311 [details] [review]
search: remove selection mechanism

Created attachment 207312 [details] [review]
css: Change the focus style for contacts and overview-icons

--
Not sure what to say on the commit message other than "It looks better."

(In reply to comment #17)
>  * Pressing "up" from the first row to go back to the search entry does not
> work.

IMO this shouldn't really work anyway. Everywhere I looked with search entries you can't get to them using arrow keys, only by tabbing.

(In reply to comment #26)
> (In reply to comment #17)
> >  * Pressing "up" from the first row to go back to the search entry does not
> > work.
> 
> IMO this shouldn't really work anyway. Everywhere I looked with search entries
> you can't get to them using arrow keys, only by tabbing.

Then there's no easy way to the search entry from the results.

Review of attachment 207307 [details] [review]:

Yes.

Review of attachment 207308 [details] [review]:

In what case is this necessary? The icons in the search results are aligned on a grid, right?

Review of attachment 207309 [details] [review]:

::: js/ui/viewSelector.js
@@ +164,3 @@
+                                                                    coordinate: Clutter.BindCoordinate.SIZE }));
+        this._focusTrap.add_constraint(new Clutter.BindConstraint({ source: this._searchResults.actor,
+                                                                    coordinate: Clutter.BindCoordinate.POSITION }));

What are these binds doing?

@@ +567,1 @@
                 return true;

Should we be handling left/right keys here?

Review of attachment 207310 [details] [review]:

Sure.

Review of attachment 207311 [details] [review]:

I love a good code removal.

Review of attachment 207312 [details] [review]:

Sure.

(In reply to comment #27)
> > IMO this shouldn't really work anyway. Everywhere I looked with search entries
> > you can't get to them using arrow keys, only by tabbing.
> 
> Then there's no easy way to the search entry from the results.

You have 3 ways:

a) Use your pointing device and click on the entry;
b) Use Tab/Shift+Tab which will circulate on all results plus the entry;
c) Start typing alphanumeric keys or backspace and the entry will be focused and edited accordingly.

Really, this is how it would work if this was done in gtk+ with a vanilla GtkEntry and a GtkIconView to show the results for instance.

(In reply to comment #29)
> Review of attachment 207308 [details] [review]:
> 
> In what case is this necessary? The icons in the search results are aligned on
> a grid, right?

They are, but if you have:

[A] [B] [X]
[C]
[D] [E]

with X being the current focus and you press Down, nothing will happen without this patch. With it, the focus will move to C.

Review of attachment 207309 [details] [review]:

::: js/ui/viewSelector.js
@@ +164,3 @@
+                                                                    coordinate: Clutter.BindCoordinate.SIZE }));
+        this._focusTrap.add_constraint(new Clutter.BindConstraint({ source: this._searchResults.actor,
+                                                                    coordinate: Clutter.BindCoordinate.POSITION }));

They keep the dummy widget as big as the whole search results StBoxLayout which means that filter_by_position() in st-container.c won't pick it up for arrow keys navigation which is the intended effect.

@@ +567,1 @@
                 return true;

Actually, I'm not even sure Up and Down are correct here. This way you can go into the overview, Ctrl+PgDown and then Down to get the focus to the 1st app. Probably we should allow only Tab/Shift+Tab as in search since you can't do the symmetric action and use the arrow keys to move the focus to the "tabs" again.

Created attachment 207861 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

--
(In reply to comment #36)
> ::: js/ui/viewSelector.js
> @@ +164,3 @@
> +
> coordinate: Clutter.BindCoordinate.SIZE }));
> +        this._focusTrap.add_constraint(new Clutter.BindConstraint({ source:
> this._searchResults.actor,
> +
> coordinate: Clutter.BindCoordinate.POSITION }));
>
> They keep the dummy widget as big as the whole search results StBoxLayout which
> means that filter_by_position() in st-container.c won't pick it up for arrow
> keys navigation which is the intended effect.

I've changed this to use a single Clutter.BindCoordinate.ALL now.

> @@ +567,1 @@
>                  return true;
>
> Actually, I'm not even sure Up and Down are correct here. This way you can go
> into the overview, Ctrl+PgDown and then Down to get the focus to the 1st app.
> Probably we should allow only Tab/Shift+Tab as in search since you can't do the
> symmetric action and use the arrow keys to move the focus to the "tabs" again.

I've done this as well. So now you can only move from the Applications tab by
pressing Tab/Shift+Tab to move focus into the application icons and categories
selectors.

Created attachment 208275 [details] [review]
st-widget: Use absolute coordinates for focus navigation

--
Rebased

Created attachment 208276 [details] [review]
st-widget: Fix actor filtering for directional keynav

--
Rebased

Created attachment 208277 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

--
Rebased

Created attachment 208278 [details] [review]
searchDisplay, viewSelector: add default result activation

--
Rebased

Created attachment 208279 [details] [review]
search: remove selection mechanism

--
Rebased

Created attachment 208280 [details] [review]
css: Change the focus style for contacts/overview-icons to look like selected

--
Rebased

OK, now that it does no longer conflict with my local tree, I've actually tried it :-)

For the sake of shortness I'll only mention what I don't like (which is a *lot* less than what I like):

 - open search providers are no longer automatically focused
   (I'm not a big fan of those buttons and actually in favor of bug 670168,
    but I don't like sneaking in design changes - either get the designers
    to agree (Jon designed the current behavior, so he's probably the one to
    talk to) or fix it)

 - the first entry has "fake focus" and gets "real focus" after hitting
   tab - it feels a bit like doing nothing, so I think moving focus from
   the search entry to the results should jump directly to the _second_
   result

 - being able to use the arrow keys instead of tab to start navigating would
   be cool (that one's not a big deal, just NTH)

 - I'm not sure about the entry style while navigating - maybe we should fake
   ":focus" in that case? Make sure to talk to the design team ... :-)

Created attachment 208492 [details] [review]
searchDisplay, viewSelector: add default result activation

--
(In reply to comment #44)
>  - open search providers are no longer automatically focused
>    (I'm not a big fan of those buttons and actually in favor of bug 670168,
>     but I don't like sneaking in design changes - either get the designers
>     to agree (Jon designed the current behavior, so he's probably the one to
>     talk to) or fix it)

Not a fan either and I'm all for removing them at the earliest FWIW. Anyway,
this version of the patch addresses makes them behave as before.

>  - the first entry has "fake focus" and gets "real focus" after hitting
>    tab - it feels a bit like doing nothing, so I think moving focus from
>    the search entry to the results should jump directly to the _second_
>    result

Fixed here as well.

>  - being able to use the arrow keys instead of tab to start navigating would
>    be cool (that one's not a big deal, just NTH)

See comment #34.

>  - I'm not sure about the entry style while navigating - maybe we should fake
>    ":focus" in that case? Make sure to talk to the design team ... :-)

Alan has explicitly requested this in comment #19. Anyway we can just drop
that patch...

Created attachment 208493 [details] [review]
search: remove selection mechanism

--
Rebased

Created attachment 208494 [details] [review]
viewSelector: Make the search canceling behavior consistent

When canceling a search pressing Escape while the focus is on the search entry
we clear the entry, set its text to the hint and go back to the previously
selected tab. Make this the behavior also for when the focus is on search
results and not on the entry itself.

--

New patch to fix an inconsistency I noticed while working on this.

(In reply to comment #45)
> Created an attachment (id=208492) [details] [review]
> searchDisplay, viewSelector: add default result activation
> 
> --
> (In reply to comment #44)
> >  - open search providers are no longer automatically focused
> >    (I'm not a big fan of those buttons and actually in favor of bug 670168,
> >     but I don't like sneaking in design changes - either get the designers
> >     to agree (Jon designed the current behavior, so he's probably the one to
> >     talk to) or fix it)
> 
> Not a fan either and I'm all for removing them at the earliest FWIW. Anyway,
> this version of the patch addresses makes them behave as before.

Thanks.


> >  - being able to use the arrow keys instead of tab to start navigating would
> >    be cool (that one's not a big deal, just NTH)
> 
> See comment #34.

Hmm, yeah - I know I'm bikeshedding, but I'm actually annoyed that GTK+ applications don't work like shell search here :-)

(As mentioned, it's not a big deal IMO)


> >  - I'm not sure about the entry style while navigating - maybe we should fake
> >    ":focus" in that case? Make sure to talk to the design team ... :-)
> 
> Alan has explicitly requested this in comment #19. Anyway we can just drop
> that patch...

I don't see how comment #19 is in any way related? I'm talking about the search entry (not the selected result). From a user perspective, :focus means "what I type goes here" - given that we forward key presses to the entry when it is not actually focused, that meaning doesn't work out. Currently, the different :focus style suggests something like "there's an active search". With the patch, it really just has the technical meaning of "the search entry has grabbed the keyboard", which doesn't look like information that should be exposed to users ...

Created attachment 208507 [details] [review]
viewSelector: While on the search results keep the entry with the focus style

This hints the user that even though keynav focus is on the search results, if
there's character input it will update the search string.

--
(In reply to comment #48)
> > >  - being able to use the arrow keys instead of tab to start navigating would
> > >    be cool (that one's not a big deal, just NTH)
> >
> > See comment #34.
>
> Hmm, yeah - I know I'm bikeshedding, but I'm actually annoyed that GTK+
> applications don't work like shell search here :-)

Well it's also how google search works for instance. They use the arrow
up/down keys to select from the completion suggestions drop down and Tab to
either activate the first completion or otherwise move focus to other widgets
and search results.

> I don't see how comment #19 is in any way related? I'm talking about the search
> entry (not the selected result). From a user perspective, :focus means "what I
> type goes here" - given that we forward key presses to the entry when it is not
> actually focused, that meaning doesn't work out. Currently, the different
> :focus style suggests something like "there's an active search". With the
> patch, it really just has the technical meaning of "the search entry has
> grabbed the keyboard", which doesn't look like information that should be
> exposed to users ...

I misunderstood you. You're right and even though I haven't heard back from
the designers yet I went ahead and fixed it. It's a pretty simple fix and gets
us closer to as regression free as possible.

Review of attachment 208275 [details] [review]:

I'm not quite convinced by the change (e.g. arrow keys can be used to navigate to unrelated elements, as from an app icon to a category in the app view), but the designers are fine with it, so ...

Apart from the minor comment below, I can't make sense from the first sentence in the commit message ("a parent of it which might not exist" - I don't see how the widget could possibly be unstaged, the change here is that we now consider @from for arrow navigation even if it is not a (grand-)*child of @widget). The code itself looks good.

::: src/st/st-widget.c
@@ +1896,1 @@
   else /* direction is an arrow key, not tab */

The comment above the if() block looks like it should be updated

Review of attachment 208276 [details] [review]:

Sure. Minor nitpick: "Fix actor filtering" suggests that the code is not working as intended, which is not really the case - or in other words: you are not fixing the current behavior, you are changing it. Something like "Allow diagonal moves for directional keynav" maybe?

Review of attachment 208277 [details] [review]:

I don't like everything here, but for now I'd say it's OK to land with an unrelated change split out:

::: js/ui/viewSelector.js
@@ +162,3 @@
+        // ... but make it unfocusable using arrow keys keynav.
+        this._focusTrap.add_constraint(new Clutter.BindConstraint({ source: this._searchResults.actor,
+                                                                    coordinate: Clutter.BindCoordinate.ALL }));

Mmh, it's not immediately obvious how the constraint excempts focusTrap from arrow keynav - it does for our current definition of "go up/down/left/right", but that may be subject to change (hey, *someone* has a patch to change it right now ;-)

Shouldn't block the patch, but I think it's worth coming up with a cleaner approach eventually (now that we have inheritance in gjs, I suspect that using a custom widget which implements navigate_focus() by chaining up to the parent iff direction is TAB_FORWARD and returning false otherwise should work)

@@ +293,3 @@
+                this._focusTrap.can_focus = false;
+                this._searchResults.actor.navigate_focus(null, Gtk.DirectionType.TAB_BACKWARD, false);
+                this._focusTrap.can_focus = true;

The can_focus dance is a bit ugly, see last comment for a possible (future) fix

@@ +541,3 @@
             Main.overview.hide();
             return true;
+        } else if (Clutter.keysym_to_unicode(symbol) || symbol == Clutter.BackSpace) {

I'm wondering whether backspace should really start a search or whether we should only pass it to the entry when a search is active (minor related nitpick: startSearch() is a bit of a misnomer now)

@@ +544,3 @@
+            this._searchTab.startSearch(event);
+        } else if (!this._searchTab.active) {
+            if (modifiers & Clutter.ModifierType.CONTROL_MASK) {

Not sure why ctrl-pageUp/Down shouldn't work when the search has focus, but I don't care deeply. It is unrelated though, so please split this out.

Review of attachment 208280 [details] [review]:

Doesn't apply after Allan's CSS cleanup - after updating the patch, can you please ask Allan for review? Lapo has suggested to make him the "gatekeeper" of our CSS (to avoid messing it up even more), which doesn't sound like a bad idea (it also allows us to deny responsibility, yay!)

Created attachment 209271 [details] [review]
st-widget: Use absolute coordinates for arrow keys focus navigation

For arrow keys navigation we might want to be able to move from a widget which
isn't a descendant of the widget we are going to.

This requires us to use absolute coordinates to compare widgets since we no
longer have the guarantee that the widgets we are comparing are siblings.
--
(In reply to comment #50)
> I'm not quite convinced by the change (e.g. arrow keys can be used to navigate
> to unrelated elements, as from an app icon to a category in the app view), but
> the designers are fine with it, so ...

Without this change when you move focus by pressing Down while on search
result C you'll end up in X instead of E.

[A] [B] [C]
[X] [E]

BTW, this commit doesn't introduce that particular behavior of going from an
app icon to a category. That's how it works already without this.

> Apart from the minor comment below, I can't make sense from the first sentence
> in the commit message ("a parent of it which might not exist" - I don't see how
> the widget could possibly be unstaged, the change here is that we now consider
> @from for arrow navigation even if it is not a (grand-)*child of @widget). The
> code itself looks good.

Yep, I changed the commit summary and message, should be a lot clearer now.

> ::: src/st/st-widget.c
> @@ +1896,1 @@
>    else /* direction is an arrow key, not tab */
>
> The comment above the if() block looks like it should be updated

It's still relevant for Tab navigation so I just moved it inside the if
block. The comment on the else block also needed updating since we are no
longer using the widget's coordinate space.

Created attachment 209272 [details] [review]
st-widget: Allow diagonal moves for directional keynav

--
Amended the patch summary as suggested.

Review of attachment 208492 [details] [review]:

A couple of issues, most shouldn't be hard to fix - I have to say that I'm not entirely happy with the highlevel structure though. "SearchResult" really belongs to the corresponding SearchResultDisplay, so keeping track of the "default" SearchResult over all ResultDisplays in SearchResults feels like a violation of that (and yeah, having SearchResult, SearchResultDisplay and SearchResults is horribly confusing (not your fault of course)).

Maybe an overall structure like this could work?

 - in ResultDisplay:
   - setHighlightFirstResult(highlight): if set, the first visible actor will be highlighted (e.g. the ResultDisplay will keep track of the first actor if it changes)
   - activateFirstResult(): no surprises here

 - in SearchResults:
  - keep track of the default result display rather than the particular result and call setHighlightFirstResult() accordingly
  - activateDefault() and highlightDefault() forward to the corresponding methods in the "default display" (I don't like that name)

::: js/ui/searchDisplay.js
@@ +64,3 @@
     },
 
+    setSelectedStyle: function(selected) {

This change doesn't make any sense to me (copying an existing function with a different name, then remove the original function in the next patch) - personally I prefer 'setSelected' over 'setSelectedStyle'

@@ +200,3 @@
+    },
+
+    getResult: function(index) {

I really dislike this:
 - setResults(results) takes an array of strings (the "result ids")
 - getResult(index) returns a SearchResult (the "result actor")

(we already have too much stuff with too similar names in search/searchDisplay, let's try not to make it worse ...)

@@ +392,3 @@
             return;
 
+        let oldDefaultResult = this._defaultResult;

More a personal opinion, but I'd use newDefaultResult instead of oldDefaultResult, e.g. something like:

let newDefaultResult = null;

for (...) {
    newDefaultResult = meta.resultDisplay.getResult(0);
   ...
}

if (newDefaultResult != this._defaultResult) {
    if (this._defaultResult)
        this._defaultResult.setSelected(false);
    if (newDefaultResult)
        newDefaultResult.setSelected(true);
    this._defaultResult = newDefaultResult;
}


The reasoning is that setting the property to a "wrong" value (read: a result that should not be the default result) feels dangerous given that we may bail out early in the case of pending results. I suspect that there are actually cases where we fail to unset a highlight with the current patch.

@@ +401,3 @@
+            this._defaultResult = meta.resultDisplay.getResult(0);
+            if (this._defaultResult && this._defaultResult.actor.visible) {
+                this._defaultResult.setSelectedStyle(this._highlightDefault);

I think it's cleaner to do

for (...) {
   ...
   if (this._defaultResult && this._defaultResult.actor.visible)
       break; // select this one!
}

if (!this._defaultResult)
    this._defaultResult = this._searchProvidersBox.get_first_child();

if (this._defaultResult)
    this._defaultResult.setSelected(this._highlightDefault);

@@ +588,3 @@
+        if (direction == Gtk.DirectionType.TAB_FORWARD && this._defaultResult) {
+            this.actor.navigate_focus(null, direction, false);
+            this.actor.navigate_focus(global.stage.key_focus, direction, false);

A short explanation like
  // The default result appears focused, so navigate directly to the next result
would be helpful

Review of attachment 208493 [details] [review]:

Looks good.

Review of attachment 208494 [details] [review]:

One doubt, otherwise looks good

::: js/ui/viewSelector.js
@@ +191,3 @@
         global.stage.set_key_focus(null);
 
+        this._entry.text = '';

I don't understand that change

Review of attachment 208507 [details] [review]:

Looks OK.

I guess I'm old, but I like commit messages to be readable in an 80-columns terminal without horizontal scrolling ;-)

::: js/ui/viewSelector.js
@@ +203,3 @@
+        if (focus != this._entry && focus != this._text) {
+            if (this._searchResults.actor.contains(focus))
+                this._entry.add_style_pseudo_class('focus');

Code looks fine, but does not update the cursor visibility - rename to onStageFocusChanged or something ...

Review of attachment 209271 [details] [review]:

"a descendant of the widget we are going to"? That doesn't make much sense either, it's "a descendant of the widget itself" (or something) ...

Review of attachment 209272 [details] [review]:

Yup

(In reply to comment #54)
> Without this change when you move focus by pressing Down while on search
> result C you'll end up in X instead of E.
> 
> [A] [B] [C]
> [X] [E]

Right, but it does not necessarily have to be unconditional - it definitively does make sense here (though it looks a bit unpredictable whether the left or right column ends up focused when going up/down from a contact result)


> BTW, this commit doesn't introduce that particular behavior of going from an
> app icon to a category. That's how it works already without this.

Oh, did it? I can't really check due to the a11y switcher being broken in overview :(
I thought we would end up selecting the _first_ category (e.g. "switching focus group") rather than the category "closest" to the focused item.

Anyway, the designers don't complain, so I shouldn't either :-)

(In reply to comment #52)
> ::: js/ui/viewSelector.js
> @@ +162,3 @@
> +        // ... but make it unfocusable using arrow keys keynav.
> +        this._focusTrap.add_constraint(new Clutter.BindConstraint({ source:
> this._searchResults.actor,
> +                                                                   
> coordinate: Clutter.BindCoordinate.ALL }));
> 
> Mmh, it's not immediately obvious how the constraint excempts focusTrap from
> arrow keynav - it does for our current definition of "go up/down/left/right",
> but that may be subject to change (hey, *someone* has a patch to change it
> right now ;-)

I'll expand the comment.

> Shouldn't block the patch, but I think it's worth coming up with a cleaner
> approach eventually (now that we have inheritance in gjs, I suspect that using
> a custom widget which implements navigate_focus() by chaining up to the parent
> iff direction is TAB_FORWARD and returning false otherwise should work)

Agree, this is quite hackish.

> @@ +541,3 @@
>              Main.overview.hide();
>              return true;
> +        } else if (Clutter.keysym_to_unicode(symbol) || symbol ==
> Clutter.BackSpace) {
> 
> I'm wondering whether backspace should really start a search or whether we
> should only pass it to the entry when a search is active (minor related
> nitpick: startSearch() is a bit of a misnomer now)

Yeah it should only affect the entry when there's already an active search. In that case I don't think it's a misnomer since if you are editing the string you're effectively starting a new search.

> @@ +544,3 @@
> +            this._searchTab.startSearch(event);
> +        } else if (!this._searchTab.active) {
> +            if (modifiers & Clutter.ModifierType.CONTROL_MASK) {
> 
> Not sure why ctrl-pageUp/Down shouldn't work when the search has focus, but I
> don't care deeply. It is unrelated though, so please split this out.

This is what the code already did, I just moved the code around a bit to be more clear and introduce the Tab handling there. It's just that the diff output gets confusing.

(In reply to comment #56)
> A couple of issues, most shouldn't be hard to fix - I have to say that I'm not
> entirely happy with the highlevel structure though. "SearchResult" really
> belongs to the corresponding SearchResultDisplay, so keeping track of the
> "default" SearchResult over all ResultDisplays in SearchResults feels like a
> violation of that (and yeah, having SearchResult, SearchResultDisplay and
> SearchResults is horribly confusing (not your fault of course)).
> 
> Maybe an overall structure like this could work?
> 
>  - in ResultDisplay:
>    - setHighlightFirstResult(highlight): if set, the first visible actor will
> be highlighted (e.g. the ResultDisplay will keep track of the first actor if it
> changes)
>    - activateFirstResult(): no surprises here
> 
>  - in SearchResults:
>   - keep track of the default result display rather than the particular result
> and call setHighlightFirstResult() accordingly
>   - activateDefault() and highlightDefault() forward to the corresponding
> methods in the "default display" (I don't like that name)

I tried but this is even worse IMO. You end up having to track if the first result should be highlighted at 2 different levels, the SearchResults tracking which ResultDisplay and the ResultDisplay tracking which Result which is more confusing for no(?) gain. Yes it's a layering violation, but no one else deals with ResultDisplays besides SeachResults and they are in the same file anyway.

> ::: js/ui/searchDisplay.js
> @@ +64,3 @@
>      },
> 
> +    setSelectedStyle: function(selected) {
> 
> This change doesn't make any sense to me (copying an existing function with a
> different name, then remove the original function in the next patch) -
> personally I prefer 'setSelected' over 'setSelectedStyle'

It's just that it describes better what it actually *does*. Anyway, it's code churn for no measurable gain so I removed this.

> @@ +200,3 @@
> +    },
> +
> +    getResult: function(index) {
> 
> I really dislike this:
>  - setResults(results) takes an array of strings (the "result ids")
>  - getResult(index) returns a SearchResult (the "result actor")
> 
> (we already have too much stuff with too similar names in search/searchDisplay,
> let's try not to make it worse ...)

Agree, changed this to getFirstResult().

> @@ +392,3 @@
>              return;
> 
> +        let oldDefaultResult = this._defaultResult;
> 
> More a personal opinion, but I'd use newDefaultResult instead of
> oldDefaultResult, e.g. something like:
> 
> let newDefaultResult = null;
> 
> for (...) {
>     newDefaultResult = meta.resultDisplay.getResult(0);
>    ...
> }
> 
> if (newDefaultResult != this._defaultResult) {
>     if (this._defaultResult)
>         this._defaultResult.setSelected(false);
>     if (newDefaultResult)
>         newDefaultResult.setSelected(true);
>     this._defaultResult = newDefaultResult;
> }

Yep, that's better.

> @@ +588,3 @@
> +        if (direction == Gtk.DirectionType.TAB_FORWARD && this._defaultResult)
> {
> +            this.actor.navigate_focus(null, direction, false);
> +            this.actor.navigate_focus(global.stage.key_focus, direction,
> false);
> 
> A short explanation like
>   // The default result appears focused, so navigate directly to the next
> result
> would be helpful

Sure.

(In reply to comment #58)
> ::: js/ui/viewSelector.js
> @@ +191,3 @@
>          global.stage.set_key_focus(null);
> 
> +        this._entry.text = '';
> 
> I don't understand that change

Because setting the StEntry's text to the empty string causes the hint text to be displayed which is what we want here. This worked for the case of pressing Escape with focus on the entry because StEntry has code to display the hint text when its ClutterText loses focus.

(In reply to comment #59)
> I guess I'm old, but I like commit messages to be readable in an 80-columns
> terminal without horizontal scrolling ;-)

Hey, that's just whatever emacs does to break lines... Should I change some setting there?

> ::: js/ui/viewSelector.js
> @@ +203,3 @@
> +        if (focus != this._entry && focus != this._text) {
> +            if (this._searchResults.actor.contains(focus))
> +                this._entry.add_style_pseudo_class('focus');
> 
> Code looks fine, but does not update the cursor visibility - rename to
> onStageFocusChanged or something ...

Right.

(In reply to comment #60)
> "a descendant of the widget we are going to"? That doesn't make much sense
> either, it's "a descendant of the widget itself" (or something) ...

"For arrow keys navigation we might want to be able to move from a widget which isn't a descendant of the widget we are going to."

The "from" before your quote is the key and the whole point of this. Still, the wording might not be clear enough :-/

I'm attaching all the patches again so that git bz apply things in order for you.

Created attachment 209335 [details] [review]
st-widget: Use absolute coordinates for arrow keys focus navigation

For arrow keys navigation we might want to be able to move from a widget which
isn't a descendant of the widget we are going to.

This requires us to use absolute coordinates to compare widgets since we no
longer have the guarantee that the widgets we are comparing are siblings.

Created attachment 209336 [details] [review]
st-widget: Allow diagonal moves for directional keynav

This allows us to do directional keyboard navigation when there's no actor
inside the horizontal or vertical strip extending from the origin actor but
there are other actors to the sides of that strip that could still be used as
targets even if that means the focus would move diagonally.

Created attachment 209337 [details] [review]
searchDisplay, viewSelector: use St keynav in Applications and Search

Created attachment 209338 [details] [review]
searchDisplay, viewSelector: add default result activation

Adds a way to highlight and activate the first search result when pressing
enter on the search entry.

Created attachment 209339 [details] [review]
search: remove selection mechanism

Now that we are using standard St keyboard navigation we don't need this
specific keynav and selection mechanism for search results.

Created attachment 209340 [details] [review]
viewSelector: Make the search canceling behavior consistent

When canceling a search pressing Escape while the focus is on the search entry
we clear the entry, set its text to the hint and go back to the previously
selected tab. Make this the behavior also for when the focus is on search
results and not on the entry itself.

Created attachment 209341 [details] [review]
viewSelector: While on the search results keep the entry with the focus style

This hints the user that even though keynav focus is on the search results, if
there's character input it will update the search string.

(In reply to comment #63)
> Yeah it should only affect the entry when there's already an active search. In
> that case I don't think it's a misnomer since if you are editing the string
> you're effectively starting a new search.

I'd consider that an update rather than a new search, but it doesn't really
matter - leave it as-is (with the backspace issue fixed of course).


> This is what the code already did, I just moved the code around a bit to be
> more clear and introduce the Tab handling there. It's just that the diff output
> gets confusing.

Woops, you are right. Pretty embarassing actually, given that I wrote that code
myself ...


(In reply to comment #64)
> I tried but this is even worse IMO. You end up having to track if the first
> result should be highlighted at 2 different levels, the SearchResults tracking
> which ResultDisplay and the ResultDisplay tracking which Result which is more
> confusing for no(?) gain. Yes it's a layering violation, but no one else deals
> with ResultDisplays besides SeachResults and they are in the same file anyway.

Pity, but OK to leave for a future cleanup then.


> > (we already have too much stuff with too similar names in search/searchDisplay,
> > let's try not to make it worse ...)
> 
> Agree, changed this to getFirstResult().

Works for me.


(In reply to comment #65)
> Because setting the StEntry's text to the empty string causes the hint text to
> be displayed which is what we want here. This worked for the case of pressing
> Escape with focus on the entry because StEntry has code to display the hint
> text when its ClutterText loses focus.

OK, consider the patch ACN then.

Created attachment 209343 [details] [review]
css: Change the focus style for contacts/overview-icons to look like selected

(In reply to comment #66)
> (In reply to comment #59)
> > I guess I'm old, but I like commit messages to be readable in an 80-columns
> > terminal without horizontal scrolling ;-)
> 
> Hey, that's just whatever emacs does to break lines... Should I change some
> setting there?

I guess so - sorry, vim user here (don't hate me)


(In reply to comment #67)
> (In reply to comment #60)
> > "a descendant of the widget we are going to"? That doesn't make much sense
> > either, it's "a descendant of the widget itself" (or something) ...
> 
> "For arrow keys navigation we might want to be able to move from a widget which
> isn't a descendant of the widget we are going to."
> 
> The "from" before your quote is the key and the whole point of this. Still, the
> wording might not be clear enough :-/

My problem is that if a child of @widget has focus, widget.navigate_focus() will move to another child of @widget (coming from another descendant of @widget, never a descendant of "the widget we are going to"). Your phrasing assumes the case where something else has focus and navigate_focus() moves the focus to the widget (for me that's a logical side effect of the function, but not its purpose!)

(In reply to comment #78)
> My problem is that if a child of @widget has focus, widget.navigate_focus()
> will move to another child of @widget (coming from another descendant of
> @widget, never a descendant of "the widget we are going to"). Your phrasing
> assumes the case where something else has focus and navigate_focus() moves the
> focus to the widget (for me that's a logical side effect of the function, but
> not its purpose!)

I think navigate_focus() documentation should be updated actually. What do you think of adding this hunk to the patch:

diff --git a/src/st/st-widget.c b/src/st/st-widget.c
index beb85ae..511efad 100644
--- a/src/st/st-widget.c
+++ b/src/st/st-widget.c
@@ -1951,12 +1951,13 @@ st_widget_real_navigate_focus (StWidget         *widget,
  * Tries to update the keyboard focus within @widget in response to a
  * keyboard event.
  *
- * If @from is a descendant of @widget, this attempts to move the
- * keyboard focus to the next descendant of @widget (in the order
- * implied by @direction) that has the #StWidget:can-focus property
- * set. If @from is %NULL, or outside of @widget, this attempts to
- * focus either @widget itself, or its first descendant in the order
- * implied by @direction.
+ * If @from is a descendant of @widget, this attempts to move the keyboard
+ * focus to the next descendant of @widget (in the order implied by
+ * @direction) that has the #StWidget:can-focus property set. If @from is
+ * %NULL, this attempts to focus either @widget itself, or its first
+ * descendant in the order implied by @direction. If @from is outside @widget,
+ * it tries to focus the closest widget (in a straight line) if @direction is
+ * one of the directional arrows, otherwise it behaves as if @from was %NULL.
  *
  * If a container type is marked #StWidget:can-focus, the expected
  * behavior is that it will only take up a single slot on the focus

(In reply to comment #79)
> I think navigate_focus() documentation should be updated actually.

I agree.


> - * If @from is a descendant of @widget, this attempts to move the
> - * keyboard focus to the next descendant of @widget (in the order
> - * implied by @direction) that has the #StWidget:can-focus property
> - * set. If @from is %NULL, or outside of @widget, this attempts to
> - * focus either @widget itself, or its first descendant in the order
> - * implied by @direction.
> + * If @from is a descendant of @widget, this attempts to move the keyboard
> + * focus to the next descendant of @widget (in the order implied by
> + * @direction) that has the #StWidget:can-focus property set. If @from is

Minor nit: why do you change the line breaks here? It doesn't look like you actually changed any text ...


> + * descendant in the order implied by @direction. If @from is outside @widget,
> + * it tries to focus the closest widget (in a straight line) if @direction is
> + * one of the directional arrows

Would it be easier to say: "If @from is outside of @widget, it behaves as if it was
a descendant if @direction is one of the directional arrows and as if it was %NULL
otherwise."?

Review of attachment 209335 [details] [review]:

Looks good - please update the gtk-doc comment for navigate_focus() before committing (nit: horizontal scrolling with the commit message - I'm going to complain a *lot* about that ;-)

Review of attachment 209336 [details] [review]:

Looks good (horizontal scrolling!)

Review of attachment 209337 [details] [review]:

Yup

Review of attachment 209339 [details] [review]:

Yup

Review of attachment 209340 [details] [review]:

Yup (horizontal scrolling!)

Review of attachment 209338 [details] [review]:

Looks OK

::: js/ui/searchDisplay.js
@@ +393,3 @@
 
+            let firstResult = meta.resultDisplay.getFirstResult();
+            if (firstResult && firstResult.actor.visible) {

Just curious - will firstResult.actor.visible ever be false?

Review of attachment 209341 [details] [review]:

Looks good (horizontal scrolling!)

Review of attachment 209343 [details] [review]:

Looks good (horizontal scrolling!)

Created attachment 209371 [details] [review]
(In reply to comment #52)

> Shouldn't block the patch, but I think it's worth coming up with a cleaner
> approach eventually (now that we have inheritance in gjs, I suspect that using
> a custom widget which implements navigate_focus() by chaining up to the parent
> iff direction is TAB_FORWARD and returning false otherwise should work)

So this is what I had in mind:

viewSelector: Clean up focusTrap in search tab

Rather than relying on implementation details of StWidget's keyboard
navigation to "hide" the focusTrap from arrow key navigation, implement
the desired behavior explicitly in a custom widget.


Unfortunately it doesn't work properly for now, as g-i fails to correctly link the navigate_focus vfunc to the navigate_focus() method, and therefore misses the "allow-none" annotation of @from.

Created attachment 209372 [details] [review]
viewSelector: Allow to start navigating results using arrow keys

We currently require users to tab away from the search entry before
search results can be navigated using arrow keys. For convenience,
support using arrow keys directly from the entry.


(In reply to comment #45)
> (In reply to comment #44)
> >  - being able to use the arrow keys instead of tab to start navigating would
> >    be cool (that one's not a big deal, just NTH)
> 
> See comment #34.

On IRC, both Jasper and Matthias were surprised about the arrow keys not working here, so I propose to add that feature :)

I'm not entirely sure if allowing right arrow is a good idea (if you use arrow keys to go back and forth in the search string, it is annoying when "overshooting" moves the focus out of the entry). If we decide it's not, then the down arrow should probably just move the focus into the search results.

Created attachment 209373 [details] [review]
st-widget: Correct annotations for navigate_focus vfunc

Since the invoker for navigate_focus has an extra parameter, annotations
from the invoker aren't applied on the vfunc itself. Fix that by annotating
the vfunc separately.


This fixes attachment 663901

Comment on attachment 209371 [details] [review]
(In reply to comment #52)

(In reply to comment #91)
> This fixes attachment 663901

That should read attachment 209371 [details] [review].

Review of attachment 209372 [details] [review]:

Yeah, the right arrow is a bad idea. The search results are not visibly to the right (unless you're in RTL or something)

Review of attachment 209371 [details] [review]:

I'm not sure how confident I am about the stability of GObject Inheritance. We aren't using it anywhere else in production, and I'm not sure it's a thing we want to sneak in right before release. If you want to take the risk, go for it, and it will help us with GNOME 3.4.1, I guess.

::: js/ui/viewSelector.js
@@ +100,3 @@
+    _init: function() {
+        this.parent();
+        this.can_focus = true;

this.parent({ can_focus: true });

Of course, if you just remove the _init method, you can just do:

this._focusTrap = new FocusTrap({ can_focus: true });

(In reply to comment #93)
> Yeah, the right arrow is a bad idea. The search results are not visibly to the
> right (unless you're in RTL or something)

I don't think this argument is convincing - keep in mind that the first result appears focused, so using the right arrow to move to the second result makes sense to me.

(I didn't test RTL though, it is probably broken)

(In reply to comment #95)
> (In reply to comment #93)
> > Yeah, the right arrow is a bad idea. The search results are not visibly to the
> > right (unless you're in RTL or something)
> 
> I don't think this argument is convincing - keep in mind that the first result
> appears focused, so using the right arrow to move to the second result makes
> sense to me.

Eh, I'm not a big fan of the implicit focus stuff. I can't quite describe how I think it should work, but let me just ask this: if you're on the first row of results and press Up to go back to the search field, and then press Enter, what gets activated? The last selected search result or the first result?

In my opinion, we should treat selection and focus as two different things.

(In reply to comment #94)
> I'm not sure how confident I am about the stability of GObject Inheritance. We
> aren't using it anywhere else in production, and I'm not sure it's a thing we
> want to sneak in right before release. If you want to take the risk, go for it,
> and it will help us with GNOME 3.4.1, I guess.

I think it's clearer than the constraint hack, but it doesn't change any behavior or work better or faster, so there's no particular hurry in getting that in. I'm perfectly fine with leaving this out until branching.


> Of course, if you just remove the _init method, [...]

Cool, I had no idea _init() was optional.

(In reply to comment #96)
> Eh, I'm not a big fan of the implicit focus stuff. I can't quite describe how I
> think it should work, but let me just ask this: if you're on the first row of
> results and press Up to go back to the search field, and then press Enter, what
> gets activated? The last selected search result or the first result?

The one which appears selected (whichever that'll be). The question is moot though, as Up doesn't work to go back to the entry (with or without that patch).


> In my opinion, we should treat selection and focus as two different things.

I agree, but probably not to what you meant - to me, the selection is always on the results ("what gets activated when I hit enter"), while the focus is always on the entry ("where what I type goes"). Which actor has grabbed keyboard input and receives events is an implementation detail that shouldn't be exposed to users.

(I'm actually not sure that we really need to provide a way to keynav from the results to the entry - after all, just typing works)

(In reply to comment #80)
> > + * descendant in the order implied by @direction. If @from is outside @widget,
> > + * it tries to focus the closest widget (in a straight line) if @direction is
> > + * one of the directional arrows
> 
> Would it be easier to say: "If @from is outside of @widget, it behaves as if it
> was
> a descendant if @direction is one of the directional arrows and as if it was
> %NULL
> otherwise."?

Yes, updated.

> > - * If @from is a descendant of @widget, this attempts to move the
> > - * keyboard focus to the next descendant of @widget (in the order
> > - * implied by @direction) that has the #StWidget:can-focus property
> > - * set. If @from is %NULL, or outside of @widget, this attempts to
> > - * focus either @widget itself, or its first descendant in the order
> > - * implied by @direction.
> > + * If @from is a descendant of @widget, this attempts to move the keyboard
> > + * focus to the next descendant of @widget (in the order implied by
> > + * @direction) that has the #StWidget:can-focus property set. If @from is
> 
> Minor nit: why do you change the line breaks here? It doesn't look like you
> actually changed any text ...

(In reply to comment #81)
> Looks good - please update the gtk-doc comment for navigate_focus() before
> committing (nit: horizontal scrolling with the commit message - I'm going to
> complain a *lot* about that ;-)

I fixed my .emacs, shouldn't be a problem moving forward :-)

(In reply to comment #86)
> ::: js/ui/searchDisplay.js
> @@ +393,3 @@
> 
> +            let firstResult = meta.resultDisplay.getFirstResult();
> +            if (firstResult && firstResult.actor.visible) {
> 
> Just curious - will firstResult.actor.visible ever be false?

You told me on IRC that the ResultDisplay could be !visible which would make the result itself !visible but I didn't really dig much into it. Not a big deal anyway, I think.

Commited, thanks for the review Florian! Leaving the bug open for your
patches.

Review of attachment 209371 [details] [review]:

This looks really neat indeed. I can't comment on relying on GObject inheritance now though.

(In reply to comment #99)
> > +            let firstResult = meta.resultDisplay.getFirstResult();
> > +            if (firstResult && firstResult.actor.visible) {
> > 
> > Just curious - will firstResult.actor.visible ever be false?
> 
> You told me on IRC that the ResultDisplay could be !visible which would make
> the result itself !visible but I didn't really dig much into it. Not a big deal
> anyway, I think.

Woah, that is _not_ how visible works. It just indicates that the actor *should* be shown (if all its ancestors are visible as well), not that it actually *is*. Or in other words: firstResult.actor.visible will only return false if hide() has been called on the actor or the property has been set explicitly. It will still be true even if the actor is not shown due to the parent being hidden.

The "mapped" property behaves like you assume (but don't we hide the parent's parent while updating results? so it will fail here in other interesting ways), but I think checking the provider container is more explicit, e.g. replace the existing test of

if (meta.actor.visible)
  break;

with

if (!meta.actor.visible)
  continue;

(In reply to comment #102)
> (In reply to comment #99)
> > > +            let firstResult = meta.resultDisplay.getFirstResult();
> > > +            if (firstResult && firstResult.actor.visible) {
> > > 
> > > Just curious - will firstResult.actor.visible ever be false?
> > 
> > You told me on IRC that the ResultDisplay could be !visible which would make
> > the result itself !visible but I didn't really dig much into it. Not a big deal
> > anyway, I think.
> 
> Woah, that is _not_ how visible works. It just indicates that the actor
> *should* be shown (if all its ancestors are visible as well), not that it
> actually *is*. Or in other words: firstResult.actor.visible will only return
> false if hide() has been called on the actor or the property has been set
> explicitly. It will still be true even if the actor is not shown due to the
> parent being hidden.
> 
> The "mapped" property behaves like you assume (but don't we hide the parent's
> parent while updating results? so it will fail here in other interesting ways),
> but I think checking the provider container is more explicit, e.g. replace the
> existing test of
> 
> if (meta.actor.visible)
>   break;
> 
> with
> 
> if (!meta.actor.visible)
>   continue;

Ups, sorry. For those not on IRC, this is already fixed in master.

Review of attachment 209372 [details] [review]:

I'm not against this actually. I just can find arguments for and against :-)

The code looks good but doesn't seem to handle RTL and I can't test that right now for some weird reason.

(In reply to comment #104)
> I just can find arguments for and against :-)

Exactly. I'll try to get a design opinion ...


> The code looks good but doesn't seem to handle RTL

Yeah, it doesn't - I'll add that if we decide we want that behavior.

(In reply to comment #97)
> Cool, I had no idea _init() was optional.

It's standard inheritance. Your _init is the same as St.Widget._init, which is the object initializer that would be run if you did new St.Widget();, which calls g_object_newv and all that fun stuff.

Just be aware of bug #671833 - you may have to remove the initial "actor" parameter in your patch.

*** Bug 672228 has been marked as a duplicate of this bug. ***

*** Bug 672228 has been marked as a duplicate of this bug. ***

Review of attachment 209373 [details] [review]:

This has been committed.

Created attachment 210187 [details] [review]
viewSelector: Allow to start navigating results using arrow keys

Updated patch to work for RTL locales as well; I've brought it up in #gnome-design, and the designers where generally in favor of allowing arrow keys directly.

Review of attachment 210187 [details] [review]:

Generally looks good and works correctly.

Just a little problem with the open search system buttons... If you filter until there's only these buttons remaining and you hit right (or left in RTL) you'll get:

    JS ERROR: !!!   Exception was: Error: Expected type interface for Argument 'from' but got type 'undefined' (nil)
    JS ERROR: !!!     lineNumber = '0'
    JS ERROR: !!!     fileName = '"gjs_throw"'
    JS ERROR: !!!     stack = '"("Expected type interface for Argument 'from' but got type 'undefined' (nil)")@gjs_throw:0
(5)@/home/rui/Source/git.gnome.org/gnome-shell/js/ui/searchDisplay.js:463
wrapper(5)@/home/rui/Source/git.gnome.org/install/share/gjs-1.0/lang.js:204
([object _private_St_IMText],[object _private_Clutter_Event])@/home/rui/Source/git.gnome.org/gnome-shell/js/ui/viewSelector.js:316
wrapper([object _private_St_IMText],[object _private_Clutter_Event])@/home/rui/Source/git.gnome.org/install/share/gjs-1.0/lang.js:204


Which can be fixed with:

diff --git a/js/ui/searchDisplay.js b/js/ui/searchDisplay.js
index 58685de..aeb6cf7 100644
--- a/js/ui/searchDisplay.js
+++ b/js/ui/searchDisplay.js
@@ -266,6 +266,7 @@ const SearchResults = new Lang.Class({
         button.activate = Lang.bind(this, function() {
             this._openSearchSystem.activateResult(provider.id);
         });
+        button.actor = button;
 
         this._searchProvidersBox.add(button);
     },

Which looks ugly but at least the uglyness sticks together in this little block :-)

Created attachment 210191 [details] [review]
viewSelector: Allow to start navigating results using arrow keys

(In reply to comment #111)
> +        button.actor = button;
> 
>          this._searchProvidersBox.add(button);
>      },
> 
> Which looks ugly but at least the uglyness sticks together in this little block
> :-)

Ugh, yeah - one more reason to remove those buttons altogether I guess :)

Review of attachment 210191 [details] [review]:

Land it!

Comment on attachment 210191 [details] [review]
viewSelector: Allow to start navigating results using arrow keys

Attachment 210191 [details] pushed as 384c7e2 - viewSelector: Allow to start navigating results using arrow keys

Do we want to land the last FocusTrap patch here?

Created attachment 221827 [details] [review]
viewSelector: Clean up focusTrap in search results

Update to master

Review of attachment 221827 [details] [review]:

Are you sure this does the correct thing without modification?

Created attachment 221867 [details] [review]
viewSelector: Clean up focusTrap in search results

Nope, didn't re-test it :-)

Review of attachment 221867 [details] [review]:

What I meant was "are you sure this will focus the second icon and not the provider's icon"?

(In reply to comment #119)
> What I meant was "are you sure this will focus the second icon and not the
> provider's icon"?

Which provider's icon? The patch applies to master, which doesn't use provider icons yet ...

Review of attachment 221867 [details] [review]:

Oh, then sure. Sorry, I thought this was part of the search rewrite.

Attachment 221867 [details] pushed as ea14d74 - viewSelector: Clean up focusTrap in search results