928315 - Shift+Enter should search backwards in the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

STR:

1. Open debugger
2. Ctrl/Cmd + F
3. Type something that is found at least once in the current source

Pressing Enter jumps to the next match. I've been spoiled to expect Shift+Enter to jump backwards to the previous one. Maybe some other users are just as spoiled as well :)

Comment 1

[Jacob Clark]

Hello, I would like to take this bug as my first fix.

Comment 2

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

Hi Jacob, most definitely! Thanks for the interest.

The logic for the search keyboard accessibility is defined by the _onKeyPress function on the FilterView prototype in browser/devtools/debugger/debugger-toolbar.js

There's a switch statement there that decides whether the action to perform is selecting forward or backwards, for which DOM_VK_RETURN and DOM_VK_ENTER falls through. This shouldn't happen anymore, and depend on the e.shiftKey property.

Do you have any experience with building firefox from source? Here's a tutorial if not: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build Let me know if you have any questions about that.

Afterwards, you'll also need to test this new functionality. Add a few new assertions in one of the already existing tests (browser_dbg_search-basic-01.js comes to mind as a good candidate). This is a browser mochitest and you can run it via the `mach` command: https://developer.mozilla.org/en-US/docs/Developer_Guide/mach

Comment 3

[Jacob Clark]

Hi Victor, 

I indeed have Firefox already checked out of Mercurial and build via ./mach.

I will write my code, generate a patch and upload it to here for review shortly.

Thank-you,
Jacob.

Comment 4

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

Thank *you*! Good luck.

Comment 5

[Jacob Clark]

Hi Victor,

In order to incrementally build devtools without compiling the entire project, should I be within this directory:

/Users/jacobclark/Development/mozilla-central/obj-x86_64-apple-darwin12.5.0/toolkit/devtools

Executing

$ make

Thanks for the support,
Jacob.

Comment 6

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

I usually build inside browser: ./mach build browser

Comment 7

[Jacob Clark]

Fantastic thanks, that's gone through successfully. 

Will continue and update on a solution shortly.

Comment 8

[Jacob Clark]

Attachment: First proposed solution to the bug

Please see my first proposed solution to this bug, I have not yet completed any tests, I would first like to receive comments about the proposed solution before going any further.

Many thanks,
Jacob.

Comment 9

[Jacob Clark]

Attachment: Proposed fix and tests

The attached patch also includes the test case to ensure the fix works as expected.

Comment 10

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

Comment on attachment 819387 [details] [diff] [review]
Proposed fix and tests

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

Very good work! I only have one comment below, other than that, this is perfect. Please submit an updated patch, then I'll push to try and land it if everything is green.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +975,5 @@
> +          actionToPerform = "selectPrev";
> +          break;
> +        }else{
> +          var isReturnKey = true; // Otherwise Fall through.
> +        }

To be completely correct, isReturnKey should be true in both cases, since that's what happened, regardless of whether shift is pressed or not. I would write this as:

case e.DOM_VK_RETURN:
case e.DOM_VK_ENTER:
  var isReturnKey = true;
  actionToPerform = e.shiftKey ? "selectPrev" : "selectNext";
  break;

Comment 11

[Jacob Clark]

Attachment: Updated patch based on review feedback

Please see revised fix

Comment 12

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

Comment on attachment 819433 [details] [diff] [review]
Updated patch based on review feedback

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

Very good! Thank you.

Comment 13

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

Try was green: https://tbpl.mozilla.org/?tree=Try&rev=fc2bec97a484

Comment 14

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

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

Comment 15

[Jacob Clark]

Fantastic, thanks! I'll be on the look out for more bugs to zap!

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b37785254785