Closed Bug 928315 Opened 6 years ago Closed 6 years ago

Shift+Enter should search backwards in the debugger

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

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

Details

(Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js])

Attachments

(1 file, 2 obsolete files)

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 :)
Whiteboard: [good first bug][mentor=vporof@mozilla.com][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: 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
Assignee: nobody → jacob.jh.clark
Status: NEW → ASSIGNED
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.
Thank *you*! Good luck.
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.
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,
Jacob.
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_RETURN:
case e.DOM_VK_ENTER:
  var isReturnKey = true;
  actionToPerform = e.shiftKey ? "selectPrev" : "selectNext";
  break;
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+
https://hg.mozilla.org/integration/fx-team/rev/b37785254785
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js] → [good first bug][mentor=vporof@mozilla.com][lang=js][fixed-in-fx-team]
Fantastic, thanks! I'll be on the look out for more bugs to zap!
https://hg.mozilla.org/mozilla-central/rev/b37785254785
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=vporof@mozilla.com][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.