861478 - Search field should pre-fill with selected text

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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]]

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]]

(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]

Attachment: added simple fix and unit test

Comment 4

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

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]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=56b00727e7ab

Comment 6

[dorian jaminais]

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]]

(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]

Attachment: updated the test to use accelKey instead of ctrlKey

I applied both recommendation form victor.

Comment 9

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

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]

:)

Thank you very much for your review.

Comment 11

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

Pushed again to try: https://tbpl.mozilla.org/?tree=Try&rev=99f95523c250

Comment 12

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

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]]

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35a7fc610f84

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35a7fc610f84