Make shift+enter find previous result in the inspector

RESOLVED FIXED in Firefox 48

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: bgrins, Assigned: steve.j.melia, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-complete})

46 Branch
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [btpp-fix-later][good first bug][lang=js])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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
Priority: -- → P2
Whiteboard: [btpp-fix-later][good first bug][lang=js]
Brian, would you like to mentor this bug?
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 2

3 years ago
I'd love to add this feature, it will be my first contribution.
(Reporter)

Comment 3

3 years ago
(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.
Mentor: bgrinstead
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 4

3 years ago
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.
(Reporter)

Comment 5

3 years ago
(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.
Assignee: nobody → steve.j.melia
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
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)
(Reporter)

Comment 8

3 years ago
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
Attachment #8734743 - Flags: review+ → review?(bgrinstead)
(Reporter)

Comment 9

3 years ago
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.
Attachment #8734743 - Flags: review?(bgrinstead) → review+
(Reporter)

Comment 10

3 years ago
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)
(Assignee)

Comment 11

3 years ago
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?
(Reporter)

Comment 12

3 years ago
(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
(Reporter)

Comment 13

3 years ago
Same patch, just updated commit message
Attachment #8735153 - Flags: review+
(Reporter)

Updated

3 years ago
Keywords: checkin-needed

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e36e9a374f2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.