1256658 - ctrl+g and ctrl+shift+g shortcuts don't work for 'find again' in inspector in Windows

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Brian Grinstead [:bgrins]]

STR:

Search for something in inspector
Press ctrl+g or ctrl+shift+g

Expected:

Next/previous result is selected

Actual:

Default browser find bar is opened

Comment 1

[Brian Grinstead [:bgrins]]

Looks like an improper use of metaKey (which is the windows key, not ctrl): https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-search.js#111

Comment 2

[Patrick Brosset <:pbro>]

Triaging this as a P2. Feel free to change it if needed.

Comment 3

[arni2033]

Wasn't the whole shortcut thing "under discussion" in bug 1225459?
Also, it shouldn't block bug 835896, because it's reproducible on Firefox 44- (i.e. before 835896)

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to arni2033 from comment #3)
> Wasn't the whole shortcut thing "under discussion" in bug 1225459?

I think the discussion was resolved, and ctrl+g / ctrl+shift+g should cycle through results.  This is a particular bug where the key shortcut should be ctrl+g but it's windows+g.

You're right that ctrl+g has always been missing (even before bug 835896) but I don't mind leaving this as blocking it because that's where it was intended to be added.

Comment 5

[arni2033]

> I think the discussion was resolved
Looking at suggestions bug 1225459 comment 5 + bug 1225459 comment 6 and this bug, it doesn't seem resolved at all.
Those suggestions are quite bad actually since they only remove useful short shortcuts (Up/Down) for action "find next/previous" and add those short shortcuts to the action "move caret to the end/beginning of input" which already has many short shortcuts like Home/End and Ctrl+Up/Down.

I filed bug 1225459 not as breakage of Up/Down, but (obviously) as a regression that completely removed the possibility to switch to a previous element. That is very annoying to this day.

> bug 835896 is where it was intended to be added
So from the start you were going to use a long and inconvenient shortcut Ctrl+(Shift+)G everywhere instead of short shortcuts like Up/Down and (Shift+)Enter? That's just terrible

Notes:
- "Short shortcut" is shortcut with a small sum of distances between keys
- "Long shortcut" is shortcut with a huge sum of distances between keys

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to arni2033 from comment #5)
> > I think the discussion was resolved
> Looking at suggestions bug 1225459 comment 5 + bug 1225459 comment 6 and
> this bug, it doesn't seem resolved at all.
> Those suggestions are quite bad actually since they only remove useful short
> shortcuts (Up/Down) for action "find next/previous" and add those short
> shortcuts to the action "move caret to the end/beginning of input" which
> already has many short shortcuts like Home/End and Ctrl+Up/Down.
> 
> I filed bug 1225459 not as breakage of Up/Down, but (obviously) as a
> regression that completely removed the possibility to switch to a previous
> element. That is very annoying to this day.
> 
> > bug 835896 is where it was intended to be added
> So from the start you were going to use a long and inconvenient shortcut
> Ctrl+(Shift+)G everywhere instead of short shortcuts like Up/Down and
> (Shift+)Enter? That's just terrible

So I think overriding up/down on inputs is not a good idea since it has a well-defined native behavior for selection.  ctrl+g and ctrl+shift+g makes sense for navigating through things in the tool.  And (shift)+enter is a 'short' shortcut that's proposed to do the same as ctrl+(shift)+g - indeed that's the current behavior for inspector.

Comment 7

[arni2033]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> And (shift)+enter is a 'short' shortcut that's proposed to do the same as
> ctrl+(shift)+g - indeed that's the current behavior for inspector
I believe you wanted to say "that's the current plan for inspector"? Because (Shift+)Enter never worked in inspector, as I said in bug 1225459. At least on Firefox 28+ (32 bit) on Win7_64.
I can't say I care much about broken Up/Down if you will actually implement (Shift+)Enter.

> up/down on inputs has a well-defined native behavior for selection
Just my personal feedback (not that it matters): (1) I never used Up/Down for moving caret in one line, because that hotkey works differently in text fields (input vs textarea)
(2) I never used Ctrl+Up/Down to move caret, as it doesn't work the same way in other applications
(3) So only Home/End works everywhere as expected for action "move caret to the end/start of line"
(4) Up/Down is "overrided" for Findbar (like in GoogleChrome), and it was convenient all the time

Comment 8

[Steve Melia]

I'd like to take this case; looks like I should just replace metaKey with ctrlKey here and fix up any mochiweb tests.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #8)
> I'd like to take this case; looks like I should just replace metaKey with
> ctrlKey here and fix up any mochiweb tests.

Thanks Steve.  We need to conditionally use metaKey for mac and ctrlKey for Windows / Linux.  You can check system.constants.platform === "macosx" like so: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-hosts.js#292.  You can require("devtools/shared/system") to get that system object.

Comment 10

[Steve Melia]

Attachment: Proposed patch for feedback

Hi Brian, I need some help

- is my patch OK as the test is in addition to my previous for #1259060
- do you have any ideas of how I can write a mochiweb test for the macosx modifier?

Comment 11

[Brian Grinstead [:bgrins]]

If you can apply this change on top of the other one then you can make changes in the same place without needing to rebase.

However, in this case I'm thinking it's quite likely we'll need to proceed with something like plan 2 in https://bugzilla.mozilla.org/show_bug.cgi?id=1259060#c5, since adding these extra checks will almost certainly cause linux debug builds to be too slow.  And we could also move the shift+enter check into that new test.

For checking the platform you can import AppConstants into the test: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js#5 and then check AppConstants.platform (it's set here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#150)

Comment 12

[Steve Melia]

Attachment: Patch for review

- Refactored test for this and #1259060 into separate test
- Linted test except for max line length (?)
- Added conditional based on AppConstants for macosx CI test

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8734989 [details] [diff] [review]
Patch for review

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

Please update the commit message to to replace 'search' with 'inspector search'.  Looks great though, you can reupload with an r+ with the change below.  Here's a try push:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c32667b2cf

::: devtools/client/inspector/inspector-search.js
@@ +7,5 @@
>  const {Cu, Ci} = require("chrome");
>  
>  const promise = require("promise");
> +
> +const system = require("devtools/shared/system");

I know this was my suggestion but I realized it'd be better as loader.lazyGetter(this, "system", () => require("devtools/shared/system")) so we don't have to load the module until (if) a user does the shortcut

Comment 14

[Steve Melia]

Attachment: Updated patch with lazily loaded reference

- updated changed commit message
- changed system reference to be lazily loaded.

Great Brian! Thanks for all your help. Is there another case you could recommend for me?

Comment 15

[Steve Melia]

Comment on attachment 8734989 [details] [diff] [review]
Patch for review

Obsoleted previous patch

Comment 16

[Steve Melia]

Comment on attachment 8735132 [details] [diff] [review]
Updated patch with lazily loaded reference

Obsoleted as file missing from commit/patch - whoops

Comment 17

[Steve Melia]

Attachment: Updated patch with lazily loaded reference & all files included!

Added in the missing file (test) - sorry about that.

Comment 18

[Brian Grinstead [:bgrins]]

Try looks good, going to land this

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/678ddeb8eb84

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #14)
> Created attachment 8735132 [details] [diff] [review]
> Updated patch with lazily loaded reference
> 
> - updated changed commit message
> - changed system reference to be lazily loaded.
> 
> Great Brian! Thanks for all your help. Is there another case you could
> recommend for me?

Thank you!  Here's a bug that's important for our conversion to HTML from XUL: https://bugzilla.mozilla.org/show_bug.cgi?id=1259126.  I just added some additional detail with what I was thinking for implementation.  If you'd like to take that on then leave a comment in the bug and I'll mark you as assigned.

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/678ddeb8eb84

Comment 22

[Jens]

Steve/Brian: It works now on Windows with Ctrl+g, when the search box is focussed. However, I would have assumed that Ctrl+g works also as "Find next" when you are within the Inspector. 

STR:
1. Search HTML
2. Press Ctrl+G or Shift+Ctrl+G => works
3. Select any other element in the inspector or do something else editin the HTML, so that search box looses focus
4. Press Ctrl+G => Opens native search box

Would should have happened:
4. Press Ctrl+G => Goes to next search results

I thought that would be the benefit of Ctrl+G over just <Enter>. It also works when you are not still in the search box. Can you retest? If you agree, maybe you or Steve might want to fix?

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Jens from comment #22)
> Steve/Brian: It works now on Windows with Ctrl+g, when the search box is
> focussed. However, I would have assumed that Ctrl+g works also as "Find
> next" when you are within the Inspector. 
> 
> STR:
> 1. Search HTML
> 2. Press Ctrl+G or Shift+Ctrl+G => works
> 3. Select any other element in the inspector or do something else editin the
> HTML, so that search box looses focus
> 4. Press Ctrl+G => Opens native search box
> 
> Would should have happened:
> 4. Press Ctrl+G => Goes to next search results
> 
> I thought that would be the benefit of Ctrl+G over just <Enter>. It also
> works when you are not still in the search box. Can you retest? If you
> agree, maybe you or Steve might want to fix?

This bug was just about hoooking up the Ctrl+G shortcut when focused in the search box.  Adding it when the focus is in the markup view should be filed as a new bug.

Looks like the normal search box behavior has two variations that might be considered when implementing:
  If the previous search term isn't found it opens / focuses / selects the search term in the search box
  If the previous search term is found then it just jumps to the next result (with the search box remaining hidden / unfocused if it was to start)

Comment 24

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly  48.0a1 (2016-03-15)on Windows 7 ,64 Bit!

This bug's fix is verified with Latest Beta , Nightly and Developer Edition

 Build ID     20160714050942
 User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0


 Build ID     20160714030208
 User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0


 Build ID     20160727004019
 User Agent   Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0