785883 - Pressing up/down/tab while filtering scripts should have some expected behavior

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

up: previous match
down/tab: next match
esc already cancels the search operation, nothing to do here.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Victor Porof [:vporof][:vp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a5140deb9ca1

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 655609 [details] [diff] [review]
v1

+    if (--this._currentlyFocusedMatch < 0) {
+      this._currentlyFocusedMatch = matches.length - 1;
+    }

could comment that you're looping here. Maybe that's obvious to smart people.

+    if (e.keyCode === e.DOM_VK_DOWN ||
+        e.keyCode === e.DOM_VK_TAB ||
+        e.keyCode === e.DOM_VK_RETURN ||
+        e.keyCode === e.DOM_VK_ENTER) {

not sure we want to capture TAB here. Worried that we'd be breaking accessibility on the toolbar with that.

Also, UP/DOWN move the caret to the beginning and end of line respectively.

What about RETURN or SHIFT+RETURN for this?

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Comment on attachment 655609 [details] [diff] [review]
> v1
> 
> +    if (--this._currentlyFocusedMatch < 0) {
> +      this._currentlyFocusedMatch = matches.length - 1;
> +    }
> 
> could comment that you're looping here. Maybe that's obvious to smart people.
> 

Ok.

> +    if (e.keyCode === e.DOM_VK_DOWN ||
> +        e.keyCode === e.DOM_VK_TAB ||
> +        e.keyCode === e.DOM_VK_RETURN ||
> +        e.keyCode === e.DOM_VK_ENTER) {
> 
> not sure we want to capture TAB here. Worried that we'd be breaking
> accessibility on the toolbar with that.
> 

That's exactly what msucan was expecting to work. Maybe it's an instinctive thing to do. I'll cc him here.

> Also, UP/DOWN move the caret to the beginning and end of line respectively.
> 

Also alt+left and alt+right. 

> What about RETURN or SHIFT+RETURN for this?

Dunno. Mihai?

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #3)

Wrt accessibility issues: we could capture these events only when needed (after a search was made, during a global search etc.)

Comment 6

[Mihai Sucan [:msucan]]

In the typical case, maybe we should leave out handling of tab/up/down. The global search, with the list of matches, seemed more like the case where tab should move the focus to the results list, or that up/down allow me to go through the matches. Thoughts?

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> In the typical case, maybe we should leave out handling of tab/up/down. The
> global search, with the list of matches, seemed more like the case where tab
> should move the focus to the results list, or that up/down allow me to go
> through the matches. Thoughts?

I'd like to have consistency between the search operations.
I can't think of a use case where I wouldn't want to jump back while doing a token search inside a single script (# op.). In fact, we don't have any UI for it, so that's actually even a more important case where shortcuts are necessary.

What do you think about comment #5? I feel like it's the perfect tradeoff.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > In the typical case, maybe we should leave out handling of tab/up/down. The
> > global search, with the list of matches, seemed more like the case where tab
> > should move the focus to the results list, or that up/down allow me to go
> > through the matches. Thoughts?
> 
> I'd like to have consistency between the search operations.
> I can't think of a use case where I wouldn't want to jump back while doing a
> token search inside a single script (# op.). In fact, we don't have any UI
> for it, so that's actually even a more important case where shortcuts are
> necessary.
> 
> What do you think about comment #5? I feel like it's the perfect tradeoff.

That sounds like a sensible thing to do - allow Tab to change focus when the user is not performing any search.

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: v1.1

Made the changes.
Great idea (c) Mihai:

[3:17 PM] <victorporof> well, as far as accessibility goes, the only issue imho is that tab should allow focus switching
[3:18 PM] <victorporof> and we can easily do that by making tab work when not searching and the textbox is empty
[3:19 PM] <msucan> victorporof: or, even better, check if text changed since focus
[3:19 PM] <msucan> so the user can tab through elements even if the textbox is not empty
[3:19 PM] <victorporof> indeed

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 656408 [details] [diff] [review]
v1.1

Rob's leaving!

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 656408 [details] [diff] [review]
v1.1

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

Not sure what are the right keybindings for this very important functionality. Up/Down are slightly more palatable, since one is not expected to type long sentences in the find box very often, and there are also workarounds, like Alt-Left/Right. Tab more is problematic though. One would expect to be able to leave the textbox by hitting Tab, even after having typed a search query and navigated through the results. Return/Shift-Return sounds good, if one can think about Return in the first place (maybe add them in the tooltip hint?). How about Ctrl/Cmd-(Shift-)G for going to the next (previous) match, like the Find bar and our editor search?

Also, I think it would make sense to have these keybindings work when filtering scripts only (without '!'): there is no visual feedback in this case, so keybindings would allow one to navigate the matches without reaching for the mouse.

I also came across a bug:
1) Visit http://harthur.github.com/wwcode/
2) Type !search and press RETURN (or whatever) to visit the matches.
3) Observe that the jquery match isn't visited and if you open it and click it with the mouse you get: "JavaScript error: chrome://browser/content/debugger-view.js, line 717: match is null"

::: browser/devtools/debugger/debugger-view.js
@@ +1362,5 @@
>            DebuggerView.GlobalSearch.scheduleSearch();
>          } else {
> +          DebuggerView.GlobalSearch[action === 1
> +            ? "focusNextMatch"
> +            : "focusPrevMatch"]();

Ouch, this brings back Perl memories! Can't we do something less likely to win the Obfuscated JavaScript Contest?

@@ +1368,5 @@
>          return;
>        }
>  
>        let editor = DebuggerView.editor;
> +      let offset = editor[action === 1 ? "findNext" : "findPrevious"](true);

I'm starting to get dizzy...

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 656408 [details] [diff] [review]
> v1.1
> 
> Review of attachment 656408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not sure what are the right keybindings for this very important
> functionality. Up/Down are slightly more palatable, since one is not
> expected to type long sentences in the find box very often, and there are
> also workarounds, like Alt-Left/Right. 

Yes, I also think it's a good idea to keep UP/DOWN, since these are instinctively the ones people try out.

> Tab more is problematic though. One
> would expect to be able to leave the textbox by hitting Tab, even after
> having typed a search query and navigated through the results.

That may be true, I'm not entirely convinced. But since Rob also shares this opinion, I'll surrender :)

> Return/Shift-Return sounds good, if one can think about Return in the first
> place (maybe add them in the tooltip hint?). How about Ctrl/Cmd-(Shift-)G
> for going to the next (previous) match, like the Find bar and our editor
> search?

Let me ask you this: if we have UP/DOWN, then we can't we simply remove TAB alltogether?

I only added TAB because it's another instinctive thing to do while dealing with a lot of searches (at least it's what msucan did, and I think it was a very valid thing to try).

> 
> Also, I think it would make sense to have these keybindings work when
> filtering scripts only (without '!'): there is no visual feedback in this
> case, so keybindings would allow one to navigate the matches without
> reaching for the mouse.
> 

I wouldn't be too sure about the "only" part in your statement, but it's definitely a good point. How about a followup for enabling these keys for scripts filtering?

> I also came across a bug:
> 1) Visit http://harthur.github.com/wwcode/
> 2) Type !search and press RETURN (or whatever) to visit the matches.
> 3) Observe that the jquery match isn't visited and if you open it and click
> it with the mouse you get: "JavaScript error:
> chrome://browser/content/debugger-view.js, line 717: match is null"
> 

Huh, interesting.
[later...] I think I know why this is happening. Happens when the result is on very very (!) long lines, after GLOBAL_SEARCH_LINE_MAX_SIZE characters. Filed bug 789027.

> ::: browser/devtools/debugger/debugger-view.js
> @@ +1362,5 @@
> >            DebuggerView.GlobalSearch.scheduleSearch();
> >          } else {
> > +          DebuggerView.GlobalSearch[action === 1
> > +            ? "focusNextMatch"
> > +            : "focusPrevMatch"]();
> 
> Ouch, this brings back Perl memories! Can't we do something less likely to
> win the Obfuscated JavaScript Contest?
> 

Lol perl :)
Ok, fiiinee...

> @@ +1368,5 @@
> >          return;
> >        }
> >  
> >        let editor = DebuggerView.editor;
> > +      let offset = editor[action === 1 ? "findNext" : "findPrevious"](true);
> 
> I'm starting to get dizzy...

Embrace javascript!

Comment 13

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #12)
> (In reply to Panos Astithas [:past] from comment #11)
> 
> > Return/Shift-Return sounds good, if one can think about Return in the first
> > place (maybe add them in the tooltip hint?). How about Ctrl/Cmd-(Shift-)G
> > for going to the next (previous) match, like the Find bar and our editor
> > search?
> 

Also filed bug 789029 for this (it's a very good idea).

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: v1.2

Addressed comments.

Comment 15

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/750ece4db918

Comment 16

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/750ece4db918