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

RESOLVED FIXED in Firefox 48

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
11 months ago

People

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

Tracking

46 Branch
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

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

Comment 1

3 years ago
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
Triaging this as a P2. Feel free to change it if needed.
Priority: -- → P2
Whiteboard: [btpp-fix-later]

Comment 3

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

Comment 4

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

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

Comment 6

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

3 years ago
(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
See Also: → 1258963
Assignee

Comment 8

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

Comment 9

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

Updated

3 years ago
Mentor: bgrinstead
Assignee

Comment 10

3 years ago
Posted patch Proposed patch for feedback (obsolete) — Splinter Review
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?
Attachment #8734791 - Flags: feedback?(bgrinstead)
Reporter

Comment 11

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

Comment 12

3 years ago
Posted patch Patch for review (obsolete) — Splinter 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
Attachment #8734791 - Attachment is obsolete: true
Attachment #8734791 - Flags: feedback?(bgrinstead)
Attachment #8734989 - Flags: review?(bgrinstead)
Reporter

Comment 13

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

Comment 14

3 years ago
- 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?
Attachment #8735132 - Flags: review+
Assignee

Comment 15

3 years ago
Comment on attachment 8734989 [details] [diff] [review]
Patch for review

Obsoleted previous patch
Attachment #8734989 - Attachment is obsolete: true
Assignee

Comment 16

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

Obsoleted as file missing from commit/patch - whoops
Attachment #8735132 - Attachment is obsolete: true
Assignee

Comment 17

3 years ago
Added in the missing file (test) - sorry about that.
Attachment #8735133 - Flags: review+
Reporter

Comment 18

3 years ago
Try looks good, going to land this
Reporter

Comment 20

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

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

Updated

3 years ago
See Also: → 1263104

Comment 22

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

Comment 23

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

Updated

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