Closed Bug 928315 Opened 6 years ago Closed 6 years ago

Shift+Enter should search backwards in the debugger


(DevTools :: Debugger, defect)

Not set


(Not tracked)

Firefox 27


(Reporter: vporof, Assigned: jacob.jh.clark)


(Whiteboard: [good first bug][][lang=js])


(1 file, 2 obsolete files)


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 :)
Whiteboard: [good first bug][][lang=js]
Hello, I would like to take this bug as my first fix.
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: 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:
Assignee: nobody → jacob.jh.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*! Good luck.
Hi Victor,

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



$ make

Thanks for the support,
I usually build inside browser: ./mach build browser
Fantastic thanks, that's gone through successfully. 

Will continue and update on a solution shortly.
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,
Attached patch Proposed fix and tests (obsolete) — Splinter Review
The attached patch also includes the test case to ensure the fix works as expected.
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_ENTER:
  var isReturnKey = true;
  actionToPerform = e.shiftKey ? "selectPrev" : "selectNext";
Attachment #819387 - Flags: review+
Attachment #819353 - Attachment is obsolete: true
Please see revised fix
Attachment #819387 - Attachment is obsolete: true
Comment on attachment 819433 [details] [diff] [review]
Updated patch based on review feedback

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

Very good! Thank you.
Attachment #819433 - Flags: review+
Whiteboard: [good first bug][][lang=js] → [good first bug][][lang=js][fixed-in-fx-team]
Fantastic, thanks! I'll be on the look out for more bugs to zap!
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][][lang=js][fixed-in-fx-team] → [good first bug][][lang=js]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.