1259060 - Make shift+enter find previous result in the inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Brian Grinstead [:bgrins]]

We can make shift+enter navigate backwards in the search results as outlined here: https://bugzilla.mozilla.org/show_bug.cgi?id=1225459#c5.

This should be an easy thing to add - add `event.shiftKey` here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-search.js#110

Comment 1

[Patrick Brosset <:pbro>]

Brian, would you like to mentor this bug?

Comment 2

[Steve Melia]

I'd love to add this feature, it will be my first contribution.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #2)
> I'd love to add this feature, it will be my first contribution.

Hi Steve, sounds great!  Have you built Firefox?  If not, and you want to use Mercurial there are instruction here: https://wiki.mozilla.org/DevTools/Hacking.  Here's a guide if you want to use git: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Getting_set_up.  Feel free to hop into the #devtools channel on IRC or ask here if you have questions.

Once you have a build set up, the fix is outlined in Comment 0.  One extra thing we'll need to do is add a regression test for this, but let's start with the code change and go from there.

Comment 4

[Steve Melia]

Yes building now and looking good so far, just taking a while. For the code change i'll attach a patch to the case once i've given it a manual test.

Without wanting to jump the gun; should I be thinking of a mochiweb test as in /devtools/client/inspector/test? I was thinking of modifying browser_inspector_search-01.js around line 55.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #4)
> Yes building now and looking good so far, just taking a while. For the code
> change i'll attach a patch to the case once i've given it a manual test.
> 
> Without wanting to jump the gun; should I be thinking of a mochiweb test as
> in /devtools/client/inspector/test? I was thinking of modifying
> browser_inspector_search-01.js around line 55.

Sure, feel free to start on a test.  I think we might need a new test file for this since inspector_search_01.js is more testing the autocomplete suggestions and we don't really have one that is directly testing next/prev navigation for the frontend.  We have 2 options here:

1) We could modify https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_search-05.js to do some backwards searching.  It's kind of overloading the meaning of that test, but it'd stop us from needing to add a new one.
2) We could add a new test similar to that one but without iframes and not needing to mess with suggestions as in lines 29-40.  So it could load up doc_inspector_search.html and do a search for something like 'body > p'.  If so you could start by copy/pasting that file and naming it browser_inspector_search-selection.js.  The new file will also need to be added in alphabetical order here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/test/browser.ini#112 and then you can run it with `./mach mochitest browser_inspector_search-selection.js`

I'm not positive of the best choice I'm inclined to go with option 1 because it's easier.  The issue would be if that test gets too slow and times out, but I don't think we need to add a lot of cases here - the previous search functionality is already tested on the backend so we just need to make sure that 'shift+enter' works once.

Comment 6

[Steve Melia]

Attachment: Add shift+enter to inspector search results navigation and a corresponding integration test

Wasn't sure what flags to add.

Comment 7

[Steve Melia]

Hi Brian see the above, looking forward to hearing your feedback; went with option 1 and browser_inspector_search-05.js which worked great (no time out). Ran the mochiweb inspector test suite; 2 tests did time out on my box but they looked unrelated. (Audio)

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8734743 [details] [diff] [review]
Add shift+enter to inspector search results navigation and a corresponding integration test

Updated flag - for future reference you can set r? and then start to enter the name of the person to review and the field will autocomplete

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8734743 [details] [diff] [review]
Add shift+enter to inspector search results navigation and a corresponding integration test

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

Nice!  Works for me - I went ahead and pushed this to our test servers so we can land it once that finishes, if it passes everything (usually takes a couple of hours): https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa8821622116.

One more thing for future reference, if you add ';r=bgrins' to the end of the commit message (or replace bgrins with whoever is reviewing the code) it makes it a bit easier to check in.  Don't worry about it here though, I'll land it with that change.

Comment 10

[Brian Grinstead [:bgrins]]

Steve, if you'd like to take on another bug we have a very similar bug but having to do with ctrl+g and ctrl+shift+g navigating search results on Windows (Bug 1256658)

Comment 11

[Steve Melia]

Hi Brian; thanks again for your help.

Looks like it has passed except for unrelated "oranges" - is there anything else I need to do here?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #11)
> Hi Brian; thanks again for your help.
> 
> Looks like it has passed except for unrelated "oranges" - is there anything
> else I need to do here?

Nope, although it looks like the source trees are closed right now for maintenance: https://whistlepig.mozilla.org/en-US/detail/618/.  So we can mark the checkin-needed flag and it will land sometime later

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: bug1259060-to-check-in.patch

Same patch, just updated commit message

Comment 14

[Pulsebot]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e36e9a374f2

Comment 16

[Will Bamberg [:wbamberg]]

I've documented this here: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Searching
and here: https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#HTML_pane

Marking this dev-doc-complete, but let me know if you need anything else.