Last Comment Bug 861478 - Search field should pre-fill with selected text
: Search field should pre-fill with selected text
Status: RESOLVED FIXED
[good first bug][mentor=vporof@mozill...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: Firefox 25
Assigned To: dorian jaminais
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-12 23:45 PDT by J. Ryan Stinnett [:jryans] (use ni?)
Modified: 2013-06-26 13:43 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
added simple fix and unit test (2.47 KB, patch)
2013-06-23 07:06 PDT, dorian jaminais
vporof: feedback+
Details | Diff | Splinter Review
updated the test to use accelKey instead of ctrlKey (2.14 KB, patch)
2013-06-24 11:02 PDT, dorian jaminais
vporof: review+
Details | Diff | Splinter Review

Description J. Ryan Stinnett [:jryans] (use ni?) 2013-04-12 23:45:20 PDT
If there is text selected in the source view of the debugger and then a search keyboard shortcut is pressed, the search field should start out with the selected text already filled in the field.

This is similar to what currently happens with the browser's find in page function.
Comment 1 Abhishek Potnis [:avp] 2013-05-24 19:18:21 PDT
Hi Victor,

I am interested in working on this bug. Can you please help me get started with it?

Thanks.
Comment 2 Victor Porof [:vporof][:vp] 2013-05-25 03:13:42 PDT
(In reply to Abhishek Potnis [:abhishekp] from comment #1)

Hi, thanks for the interest. You can get the currently selected source editor text via getEditorSelection in debugger-view.js. With that information, you can modify the _doSearch function in the FilterView object from debugger-toolbar.js, which is called whenever a search key is pressed.

You'll also need to add some testing for this, and modifying browser_dbg_scripts-searching-01.js with a few more assertions should be enough.
Comment 3 dorian jaminais 2013-06-23 07:06:52 PDT
Created attachment 766443 [details] [diff] [review]
added simple fix and unit test
Comment 4 Victor Porof [:vporof][:vp] 2013-06-23 08:42:57 PDT
Comment on attachment 766443 [details] [diff] [review]
added simple fix and unit test

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

Yeah, this looks good! Thank you Dorian. r+ with a green try run. Do you have try access? If not I can push for you.

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-01.js
@@ +266,5 @@
>      is(gSources.visibleItems.length, 1,
>        "Not all the scripts are shown after the search. (20)");
>      isnot(gSources.widget.getAttribute("label"), noMatchingSources,
>        "The scripts container should not display a notice that matches are found.");
> +    

Nit: some extra whitespace here.
Comment 5 Victor Porof [:vporof][:vp] 2013-06-23 09:13:32 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=56b00727e7ab
Comment 6 dorian jaminais 2013-06-23 09:44:38 PDT
Thank you Victor for reviewing my patch so quickly.

I don't have try access but have seen that you pushed it.
Comment 7 Victor Porof [:vporof][:vp] 2013-06-23 11:28:36 PDT
(In reply to dorian jaminais from comment #6)
> Thank you Victor for reviewing my patch so quickly.
> 
> I don't have try access but have seen that you pushed it.

Test seems to be failing on OS X. This is because you'll need to use accelKey instead of ctrlKey when synthesizing the event in the test. OS X uses cmd+F instead of ctrl+F for searching.

Oh, and while you're at it, please use "is" instead of "ok" for testing the searchbox value.
Comment 8 dorian jaminais 2013-06-24 11:02:45 PDT
Created attachment 766776 [details] [diff] [review]
updated the test to use accelKey instead of ctrlKey

I applied both recommendation form victor.
Comment 9 Victor Porof [:vporof][:vp] 2013-06-24 11:03:59 PDT
Comment on attachment 766776 [details] [diff] [review]
updated the test to use accelKey instead of ctrlKey

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

<3
Comment 10 dorian jaminais 2013-06-24 11:05:35 PDT
:)

Thank you very much for your review.
Comment 11 Victor Porof [:vporof][:vp] 2013-06-24 11:06:31 PDT
Pushed again to try: https://tbpl.mozilla.org/?tree=Try&rev=99f95523c250
Comment 12 Victor Porof [:vporof][:vp] 2013-06-25 05:36:23 PDT
There's a timeout on OS X 10.6 debug, but it happens exactly at a sweet spot just before the test finishes. requestingLongerTimeout should be enough, I'll add it just before landing.
Comment 13 Victor Porof [:vporof][:vp] 2013-06-26 05:09:31 PDT
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35a7fc610f84
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-06-26 13:43:20 PDT
https://hg.mozilla.org/mozilla-central/rev/35a7fc610f84

Note You need to log in before you can comment on or make changes to this bug.