Closed Bug 1153474 Opened 9 years ago Closed 9 years ago

CMD + G in style editor does not find next if you haven't hit enter first

Categories

(DevTools :: Source Editor, defect, P2)

x86
macOS
defect

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: shobson, Assigned: zer0)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 5 obsolete files)

Steps to reproduce:

1) go to new website
2) open style editor
3) place cursor in style editor
4) CMD + F to bring up search dialogue
5) type something 
6) CMD + G to find next occurance
7) watch Firefox search the webpage for whatever you search a webpage for last instead of the style editor

Occasionally it fails to search either place.
Also a problem when resuming searches for the same term (search, type something CMD + F, CMD + G).
(search, type something in the editor, CMD + F, CMD + G).
Priority: -- → P2
Whiteboard: [devedition-40]
Component: Developer Tools → Developer Tools: Source Editor
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Assignee: nobody → zer0
Had a discussion with Matteo about this.  It happens when the keypress happens on the search input itself.  In that case the normal CodeMirror events aren't firing and the event is bubbling up to the browser level.  There should be a way to catch this case in editor.js, stop propagation on the event, and perform the expected command.
Status: NEW → ASSIGNED
Attached patch cmd-g.patch (obsolete) — Splinter Review
As mentioned to Brian, this behavior is not limited to Style Editor, but affect all the components that uses Code Mirror's search – so we have the same bug in Scratchpad; not in Debugger 'cause we use our UI for the search input.

I dig the code of Code Mirror to find an elegant way to solve that, but the fact that we're using the Code Mirror UI for the search is a bit problematic; 'cause all the key bindings we can set in CM are considered only when the editor area has the focus, not when the focus in the dialog – e.g. search.

So a quick solution is intercept the key event on DOM level, and then mimic the proper behavior depends by the key modifiers. It's a bit hacky, but Brian points to me that we already have something like that for the `contextmenu` so I think it will be OK, at least for the time being.

Few considerations:
- As said, this fix is not related to style editor per sé, it fixes also scratchpad
- The unit test we have for search in the sourceeditor, doesn't rely on keystrokes, but invokes directly the search functions; plus I didn't find any test for the `contextmenu` hack: so my question is, should we test this, and in case where would be the best place to do so?
- the code just check if we're inside an <INPUT> of CodeMirror. It's probably better if use a class or data-* attribute to identify the search one. I noticed also the editor has also input for "replace", but I wasn't able to display them: do we use such functionality?
Attachment #8605171 - Flags: review?(bgrinstead)
Comment on attachment 8605171 [details] [diff] [review]
cmd-g.patch

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

This works well, but we need a test for it.  browser_editor_goto_line.js is probably the closest example to what we will need here (in that it also opens up an input and tests it)

::: browser/devtools/sourceeditor/editor.js
@@ +298,5 @@
>            popup = el.ownerDocument.getElementById(this.config.contextMenu);
>          popup.openPopupAtScreen(ev.screenX, ev.screenY, true);
>        }, false);
>  
> +      // Intercept the find again keystroke on CodeMirror, to perform the

Don't we also need to intercept the 'find' key and prevent default so that it doesn't open the default find bar?

@@ +302,5 @@
> +      // Intercept the find again keystroke on CodeMirror, to perform the
> +      // search even if the focus is still on search dialog's input instead of
> +      // the editor.
> +
> +      let findAgainKey = L10N.GetStringFromName("findAgain.commandkey");

Can you change the string name to 'findNext' so it will match the command name?

@@ +307,5 @@
> +      let accel = Services.appinfo.OS === "Darwin" ? "metaKey" : "ctrlKey";
> +
> +      cm.getWrapperElement().addEventListener("keydown", (ev) => {
> +        let isFindAgainKey = ev[accel] && ev.key.toUpperCase() === findAgainKey;
> +        let isInInput = ev.originalTarget.tagName === "INPUT";

There are other inputs we need to watch out for (like 'jump to line' via cmd + j).  So we will need another way to target this input to make sure it's the right one.  This could be done possibly by just adding an attribute to it from search.js - just make sure to also update the README for updating that file

@@ +309,5 @@
> +      cm.getWrapperElement().addEventListener("keydown", (ev) => {
> +        let isFindAgainKey = ev[accel] && ev.key.toUpperCase() === findAgainKey;
> +        let isInInput = ev.originalTarget.tagName === "INPUT";
> +
> +        if (isInInput && isFindAgainKey) {

Nit: maybe just early return when it's not this case to limit the amount of nesting in this function

@@ +314,5 @@
> +          ev.preventDefault();
> +
> +          let query = ev.originalTarget.value;
> +
> +          if (!cm.state.search || cm.state.search.query !== query) {

I don't understand exactly why we need this, but even the most updated versions of the CM search plugin seem to use this same interface so it's probably OK-ish.  Would be nice if we could expose the getSearchCursor function from search.js so we don't need to duplicate the properties here.  I guess we could attach it at the bottom of that file as CodeMirror.getSearchState = getSearchState.  I'm not really sure which is a less-bad approach, so I'll leave that up to you
Attachment #8605171 - Flags: review?(bgrinstead) → feedback+
(In reply to Brian Grinstead [:bgrins] from comment #5)

> Don't we also need to intercept the 'find' key and prevent default so that
> it doesn't open the default find bar?

I'm going to add it, thanks for the catch!

> > +      let findAgainKey = L10N.GetStringFromName("findAgain.commandkey");
> 
> Can you change the string name to 'findNext' so it will match the command
> name?

I choose `findAgain` to avoid misleading, 'cause this code is used for both `findNext` and `findPrev` – we also use it as command's name for the browser.

> There are other inputs we need to watch out for (like 'jump to line' via cmd
> + j).  So we will need another way to target this input to make sure it's
> the right one.

I can base this fix on bug 1159001, so I can use `type="search"` to target specifically this input.


> > +          if (!cm.state.search || cm.state.search.query !== query) {
> 
> I don't understand exactly why we need this, but even the most updated
> versions of the CM search plugin seem to use this same interface so it's
> probably OK-ish.

Basically, we want to search what is in the search input – even if we didn't press enter yet – so if there isn't a search's state, or the text we want to search doesn't match with the current search state, we need to create a new one.
Depends on: 1159001
Attached patch cmd-g v2 (obsolete) — Splinter Review
It also contains the unit test for bug 1159001 (it's built on top of that patch)
Attachment #8605171 - Attachment is obsolete: true
Attachment #8617948 - Flags: review?(bgrinstead)
Comment on attachment 8617948 [details] [diff] [review]
cmd-g v2

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

Applied and played with it, nice improvements.  See comments below

::: browser/devtools/sourceeditor/editor.js
@@ +313,5 @@
> +
> +        if (!isSearchInput || !ev[accel]) return;
> +
> +        if (key === findKey) {
> +          ev.preventDefault();

I think if find is pressed again we should select the contents of the input (and the test should add an assertion for this)

::: browser/devtools/sourceeditor/test/browser_editor_find_again.js
@@ +3,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +

Please add a comment here about what this test is doing

@@ +4,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +//Cu.import("resource://gre/modules/Services.jsm");

Commented out line here should be remove

@@ +51,5 @@
> +  ok(selectionStart === 0 && selectionEnd === value.length,
> +    "The text inside the search box is selected when re-opened");
> +}
> +
> +function test() {

Can you convert this test to use add_task and yields wherever possible?  There's not a ton of examples using this in the sourceeditor folder, but here is one: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_script_injection.js
Attachment #8617948 - Flags: review?(bgrinstead) → feedback+
Attached patch cmd-g v3 (obsolete) — Splinter Review
Attachment #8617948 - Attachment is obsolete: true
Attachment #8623362 - Flags: review?(bgrinstead)
Comment on attachment 8623362 [details] [diff] [review]
cmd-g v3

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

Nice, much better!

Cancelling the review because I see something odd in splinter:

'Modified Binary File: xpcom/tests/unit/data/SmallApp.app/Contents/MacOS/SmallApp'

Can you double check that any changes to this file are removed from the patch?

::: browser/devtools/sourceeditor/test/browser_editor_find_again.js
@@ +62,5 @@
> +
> +}
> +
> +add_task(function*() {
> +  yield runTest();

Nit: may as well move the contents of runTest into this add_task block and delete the runTest function

add_task(function*() {
  let { ed, win } = yield setup();

  ...etc
});
Attachment #8623362 - Flags: review?(bgrinstead)
Attached patch cmd-g v4 (obsolete) — Splinter Review
I'm not sure why I have this issue, every time, when I compile firefox on OS X.. I also do a complete checkout. I'll try to investigate more. Anyway, here the updated patch. I did use the `runTest` function before because looking at the unit test I thought what a sort of code style; now is fixed.
Attachment #8628475 - Flags: review?(bgrinstead)
Attachment #8623362 - Attachment is obsolete: true
Attachment #8628475 - Attachment description: cmd-g.patch → cmd-g v4
Attachment #8628475 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
Please provide a Try link for this.
Keywords: checkin-needed
While trying this out, I noticed a similar thing involving search/replace.
To reproduce:

shift-ctrl-f in the style editor
This brings up the code mirror search&replace and switches the focus to it.
Now type ctrl-g.

This brings up the firefox search bar.
I don't know what it should do, maybe nothing, but this was suprising.
(In reply to Tom Tromey :tromey from comment #13)

> While trying this out, I noticed a similar thing involving search/replace.

As said in the meeting yesterday, thank you for point me out that! It has been fixed, but I was waiting for the unit test to make it working again before apply a new patch.

> I don't know what it should do, maybe nothing, but this was suprising.

So, currently what I've done is doing nothing in case of find / find again keys in the "replace" dialog box – so, no Firefox's search UI is opened – and re-select the text in "replace" dialog box if the replace shortcut keys is pressed again (like we decided to do for "find").

I think it's reasonable, in case we could improve that in a separate bug.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0efc2bb555f6

(waiting the result before update the patch on the bug, just in case)
Attached patch cmd-g v5 (obsolete) — Splinter Review
Brian, I'm requesting a new round of review 'cause there are sensible changes here.
Also, with all the versions I made for fixing the unit testing on linux, it could be that I've some left overs.

At the end, I decided to do not use `cursorActivity` event in testing, 'cause I got some intermittent failures.
Attachment #8628475 - Attachment is obsolete: true
Attachment #8634134 - Flags: review?(bgrinstead)
Comment on attachment 8634134 [details] [diff] [review]
cmd-g v5

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

::: browser/devtools/sourceeditor/editor.js
@@ +313,5 @@
> +      cm.getWrapperElement().addEventListener("keydown", (ev) => {
> +        let key = ev.key.toUpperCase();
> +        let node = ev.originalTarget;
> +        let isInput = node.tagName === "INPUT";
> +        let isSearch = isInput && node.type === "search";

I liked the old name better - isSearchInput.  isSearch sounds like it depends on the key event not the node

@@ +314,5 @@
> +        let key = ev.key.toUpperCase();
> +        let node = ev.originalTarget;
> +        let isInput = node.tagName === "INPUT";
> +        let isSearch = isInput && node.type === "search";
> +        let isDialog = isInput &&

This could probably use a comment above it to the effect that the 'replace' box is an instance of an input in a dialog.  Also, the variable should probably called isDialogInput or similar, since the target itself isn't actually a dialog

::: browser/devtools/sourceeditor/test/browser_editor_find_again.js
@@ +79,5 @@
> +
> +  input = edDoc.querySelector("input[type=search]");
> +  ok(input, "find command key opens the search box");
> +
> +  // On linux, getting immediately the selection's range here fails, returning

Maybe wrap up the big comment + the dispatchEventAsync + the waitFor call into a function called waitForFocus(input) or similar so we can reuse it in both test cases to save a bit of code and make the tests easier to read?
Attachment #8634134 - Flags: review?(bgrinstead) → review+
Attached patch cmd-g v6Splinter Review
Attachment #8634134 - Attachment is obsolete: true
Attachment #8634601 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14fe6f0e072a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.