The accessibility.typeaheadfind.prefillwithselection pref doesn't work with e10s

VERIFIED FIXED in mozilla38

Status

()

VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: mrbkap)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 18 obsolete attachments)

34.47 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
IOW, the find bar's content is not prefilled with the current selection when opening the find bar.  Note that this pref is the default on Linux and Windows.

Updated

4 years ago
tracking-e10s: ? → +

Comment 1

4 years ago
Could this be the same issue as bug 1053045?
Flags: needinfo?(ehsan)
(Reporter)

Comment 2

4 years ago
(In reply to atifrea from comment #1)
> Could this be the same issue as bug 1053045?

Yes, but I think my bug report is a bit more precise, so I'll dupe the other way around.
Flags: needinfo?(ehsan)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1053045

Updated

4 years ago
Assignee: nobody → atifrea

Updated

4 years ago
Blocks: 1029143
No longer blocks: 1029143
Created attachment 8539422 [details] [diff] [review]
switch to using a set for the listeners

This is just a small patch to use a JS feature that we can use.
Created attachment 8539423 [details] [diff] [review]
Implement prefillwithselection for e10s.

This actually implements the pref.
Assignee: atifrea → mrbkap
Comment on attachment 8539422 [details] [diff] [review]
switch to using a set for the listeners

Mike, I think you're the best reviewer for this, right?
Attachment #8539422 - Flags: review?(mdeboer)
Attachment #8539423 - Flags: review?(mdeboer)
Comment on attachment 8539422 [details] [diff] [review]
switch to using a set for the listeners

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

Yes, using a Set looks better!

If you make the changes I outlined below, r=me. (They're simple enough that we don't need to go through another cycle, methinks)

::: toolkit/modules/RemoteFinder.jsm
@@ +22,5 @@
>  }
>  
>  RemoteFinder.prototype = {
>    addResultListener: function (aListener) {
> +    this._listeners.add(aListener);

Please make sure you only add one unique listener, to prevent race conditions:
```js
if (!this._listeners.has(aListener)) {
  this._listeners.add(aListener);
}
```

@@ +27,4 @@
>    },
>  
>    removeResultListener: function (aListener) {
> +    this._listeners.remove(aListener);

Whoops, this should be: `this._listeners.delete(aListener);`

To be perfectly honest, I usually have https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set open when working with newer primitives, same with (Weak)Map for example.
Attachment #8539422 - Flags: review?(mdeboer) → review+
Comment on attachment 8539423 [details] [diff] [review]
Implement prefillwithselection for e10s.

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

Thanks for working on this!

::: toolkit/content/widgets/findbar.xml
@@ +1068,5 @@
>              userWantsPrefill =
>                prefsvc.getBoolPref("accessibility.typeaheadfind.prefillwithselection");
>  
> +          // If userWantsPrefill is false but prefillWithSelection is true,
> +          // then we might need to check the selection keyboard. Call

I *think* you mean s/keyboard/clipboard/ ?

@@ +1074,4 @@
>            if (this.prefillWithSelection && userWantsPrefill)
> +            this.browser.finder.getInitialSelection();
> +          else if (this.prefillWithSelection)
> +            this.onInitialSelection("");

Can you rewrite this to:

```js
if (this.prefillWithSelection && userWantsPrefill) {
  this.browser.finder.getInitialSelection();
  return;
}

this.onInitialSelection()'
```
?

@@ +1078,3 @@
>  
>            this._findField.select();
>            this._findField.focus();

...in which case you can remove these two lines.

@@ +1224,5 @@
> +        <body><![CDATA[
> +          // Be sure not to overwrite anything the user might have typed.
> +          if (this._findField.value)
> +            return;
> +

Please remove these four lines above; we have too little context to make this assumption here.

::: toolkit/modules/Finder.jsm
@@ +201,5 @@
>      }
>    }),
>  
> +  getInitialSelection: function() {
> +    new Promise((resolve) => { resolve(); }).then(() => {

What are you trying to do here? I *think* you added this while iteratively developing this patch...

@@ +202,5 @@
>    }),
>  
> +  getInitialSelection: function() {
> +    new Promise((resolve) => { resolve(); }).then(() => {
> +      let initialSelection = doGetInitialSelection();

This will throw, so you probably want to make this `this.doGetInitialSelection()`

@@ +211,5 @@
> +      }
> +    });
> +  },
> +
> +  doGetInitialSelection: function() {

I'd like to suggest renaming this to `getActiveSelectionText`... What do you think? If you agree, don't forget to update RemoteFinder.jsm appropriately ;)

@@ +222,5 @@
> +    let selText;
> +
> +    if (focusedElement instanceof Ci.nsIDOMNSEditableElement &&
> +        focusedElement.editor)
> +    {

nit: I know you pasted this over, but can you change the bracing style while we're here?

@@ +223,5 @@
> +
> +    if (focusedElement instanceof Ci.nsIDOMNSEditableElement &&
> +        focusedElement.editor)
> +    {
> +      // The user may have a selection in an input or textarea

nit: missing dot.

@@ +228,5 @@
> +      selText = focusedElement.editor.selectionController
> +        .getSelection(Components.interfaces.nsISelectionController.SELECTION_NORMAL)
> +        .toString();
> +    }
> +    else {

nit: bracing style, like above.

@@ +229,5 @@
> +        .getSelection(Components.interfaces.nsISelectionController.SELECTION_NORMAL)
> +        .toString();
> +    }
> +    else {
> +      // Look for any selected text on the actual page

nit: missing dot.

@@ +236,5 @@
> +
> +    if (!selText)
> +      return "";
> +
> +    // Process our text to get rid of unwanted characters

nit: missing dot.

@@ +237,5 @@
> +    if (!selText)
> +      return "";
> +
> +    // Process our text to get rid of unwanted characters
> +    const selectionMaxLen = 150;

Can you move this const to the top of this file, right below `kHighlightIterationSizeMax` and name it `kSelectionMaxLen`?

@@ +241,5 @@
> +    const selectionMaxLen = 150;
> +    if (selText.length > selectionMaxLen) {
> +      let pattern = new RegExp("^(?:\\s*.){0," + selectionMaxLen + "}");
> +      pattern.test(selText);
> +      selText = RegExp.lastMatch;

Well, this gives us a perfect opportunity to re-evaluate the correctness of this code... This _must_ be the one of the most verbose methods I've ever seen to do `substr(0, selectionMaxLen)`! :-)

On top of that, it's completely redundant! (See below)

@@ +246,5 @@
> +    }
> +    return selText.replace(/^\s+/, "")
> +                  .replace(/\s+$/, "")
> +                  .replace(/\s+/g, " ")
> +                  .substr(0, selectionMaxLen);

Can you change this to 
```js
return selText.trim().replace(/\s+/g, " ").substr(0, kSelectionMaxLen);
```
?
Attachment #8539423 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Please make sure you only add one unique listener, to prevent race
> conditions:

This was intentional. Sets already guarantee that their elements are unique.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #9)
> This was intentional. Sets already guarantee that their elements are unique.

So true! My apologies for asserting wrong.
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Please remove these four lines above; we have too little context to make
> this assumption here.

I'm a little worried about the content process being very slow to respond, the user starting to type, and us overwriting the user's initial search. If that becomes a problem, we can deal with it then.

> > +    new Promise((resolve) => { resolve(); }).then(() => {
> 
> What are you trying to do here? I *think* you added this while iteratively
> developing this patch...

This was meant to make this execute asynchronously. I've fixed it.

I'll attach the two updated patches.
Created attachment 8540211 [details] [diff] [review]
First patch, updated
Attachment #8539422 - Attachment is obsolete: true
Attachment #8540211 - Flags: review+
Created attachment 8540212 [details] [diff] [review]
Second patch, updated
Attachment #8539423 - Attachment is obsolete: true
Attachment #8540212 - Flags: review?(mdeboer)
Comment on attachment 8540212 [details] [diff] [review]
Second patch, updated

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

Awesome, this looks good to me!

However, I'm afraid you're not done here yet :( The Cmd+E hotkey on OSX sets the current value of the find clipboard to the currently selected text.
The way this is implemented currently, is that the browser <key> handler calls `onFindSelectionCommand` on the findbar, which in turn calls into `setSearchStringToSelection` in Finder.jsm. RemoteFinder.jsm currently doesn't have this function implemented, so this doesn't get proxied to the actual Finder.jsm implementation in the child process.
I think it's worth adding a part 3 patch that establishes this proxying to make this a full-circle implementation.

Thanks!

::: toolkit/content/widgets/findbar.xml
@@ +1068,5 @@
>              userWantsPrefill =
>                prefsvc.getBoolPref("accessibility.typeaheadfind.prefillwithselection");
>  
> +          // If userWantsPrefill is false but prefillWithSelection is true,
> +          // then we might need to check the selection clipboard. Call

ITYM 'find clipboard' (on OSX).

@@ +1069,5 @@
>                prefsvc.getBoolPref("accessibility.typeaheadfind.prefillwithselection");
>  
> +          // If userWantsPrefill is false but prefillWithSelection is true,
> +          // then we might need to check the selection clipboard. Call
> +          // onInitialSelection to do so.

Perhaps it's good to move this comment just above the call to `this.onInitalSelection()`?

::: toolkit/modules/Finder.jsm
@@ +225,5 @@
> +    if (focusedElement instanceof Ci.nsIDOMNSEditableElement &&
> +        focusedElement.editor) {
> +      // The user may have a selection in an input or textarea.
> +      selText = focusedElement.editor.selectionController
> +        .getSelection(Components.interfaces.nsISelectionController.SELECTION_NORMAL)

nit: Ci
Attachment #8540212 - Flags: review?(mdeboer) → review+
Created attachment 8542703 [details] [diff] [review]
Add support for cmd+e to set the find clipboard on OSX on e10s.

This patch actually fixes a bug, where cmd+e currently doesn't see selections in text inputs. This is a little trickier than it should be because I had to avoid focusing the find bar for cmd+e.
Attachment #8542703 - Flags: review?(mdeboer)
Created attachment 8542705 [details] [diff] [review]
Fix getting clipboardSearchString on RemoteFinder.

I was able to avoid the round-trip (and therefore making this
asynchronous) by using the loadgroup off of the browser. I had to do
some acrobatics to share the implementation between the two files.
Attachment #8542705 - Flags: review?(mdeboer)
Comment on attachment 8542705 [details] [diff] [review]
Fix getting clipboardSearchString on RemoteFinder.

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

Nice! This seems quite essential to me ;)

::: toolkit/modules/RemoteFinder.jsm
@@ +14,5 @@
>  
> +XPCOMUtils.defineLazyGetter(this, "GetClipboardSearchString",
> +    () => {
> +      return Cu.import("resource://gre/modules/Finder.jsm", {}).GetClipboardSearchString;
> +    });

nit: you can omit the curlies and 'return' statement in the function declaration here.
Attachment #8542705 - Flags: review?(mdeboer) → review+
Hi Blake - sorry for the delay, I was out sick - how do I apply the two latest patches? on top of the previous two?
Flags: needinfo?(mrbkap)
OS: Mac OS X → All
Hardware: x86 → All
If you use git, you can find my branch at <https://github.com/mrbkap/gecko-dev/tree/1055508-findbar-e10s>. If not, I'll attach rebased versions of all of the patches now.
Flags: needinfo?(mrbkap)
Created attachment 8545453 [details] [diff] [review]
patch 1
Attachment #8540211 - Attachment is obsolete: true
Attachment #8540212 - Attachment is obsolete: true
Attachment #8542703 - Attachment is obsolete: true
Attachment #8542705 - Attachment is obsolete: true
Attachment #8542703 - Flags: review?(mdeboer)
Attachment #8545453 - Flags: review+
Created attachment 8545455 [details] [diff] [review]
Patch 3 - add support for cmd+e
Attachment #8545455 - Flags: review?(mdeboer)
Created attachment 8545458 [details] [diff] [review]
Patch 4 - Fix getting clipboardSearchString
Attachment #8545458 - Flags: review+
Comment on attachment 8545455 [details] [diff] [review]
Patch 3 - add support for cmd+e

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

Well, this all looks rather awesome, sir! Congrats! :)

Who knew that this would turn out to be such a substantial amount of work!
Attachment #8545455 - Flags: review?(mdeboer) → review+
I'm pushing this to try for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d39dca8d059

Meanwhile I'll fully test this on Windows and Linux.
Status: NEW → ASSIGNED
Blake, did you see the TBPL results? There are a couple of test failures that shows that your patches regress a few things. Do you have time to fix those?
I have fixes for most of the oranges and just debugged the last ones. I should have patches tomorrow to fix them.
Created attachment 8549273 [details] [diff] [review]
Fix a test by focusing the findField when the findbar opens.

This actually fixes a few tests that assume that the findbar is synchronously focused when the findbar opens.
Attachment #8549273 - Flags: review?(mdeboer)
Created attachment 8549274 [details] [diff] [review]
Add a promise for findbar initialization

Most of the test failures are due to findbar initialization being async.  It's hard to program to that though, because there's no good notification of it being done. This adds a way for code to wait until the findbar is initialized.
Attachment #8549274 - Flags: review?(mdeboer)
Created attachment 8549275 [details] [diff] [review]
Fix a mochitest failure by using the new promise.
Attachment #8549275 - Flags: review?(mdeboer)
Created attachment 8549276 [details] [diff] [review]
Get rid of the uncaught rejection

This is kind of a free patch. There's an "uncaught promise exception" in this test, so I debugged it a bit. The problem is that we end up trying to get innerWidth on a window that doesn't have a docshell (it's about:addons). I don't know why it doesn't have a docshell, but the test passes with this and I don't think this makes anything worse than it is now.
Attachment #8549276 - Flags: review?(mdeboer)
Created attachment 8549278 [details] [diff] [review]
Fix test_findbar.xul

This was the hardest of the tests to fix because it has the most direct dependencies on the implementation of the findbar. For my own ease, I broke opening the find bar into its own function so that I could use the Task async stuff to drive running the actual test for a few things.

One tricky bit is that there's a test that wants to muck with the getInitialSelection function. I moved that off of the findbar and onto the Finder in one of the first few patches, so I had to update the test to do the right thing (it's also async and needs to call the callback function).
Attachment #8549278 - Flags: review?(mdeboer)
Created attachment 8549802 [details] [diff] [review]
Add a promise for findbar initialization

This should still be reviewed first. My try run found a couple of bugs in this patch the first time around (notably the bug on OSX, where we'd clear this._startFindPromise before returning it and a use of Promise without being sure it'd actually be defined).
Attachment #8549274 - Attachment is obsolete: true
Attachment #8549274 - Flags: review?(mdeboer)
Attachment #8549802 - Flags: review?(mdeboer)
Comment on attachment 8549273 [details] [diff] [review]
Fix a test by focusing the findField when the findbar opens.

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

LGTM
Attachment #8549273 - Flags: review?(mdeboer) → review+
Comment on attachment 8549275 [details] [diff] [review]
Fix a mochitest failure by using the new promise.

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

LGTM

::: browser/base/content/test/general/browser.ini
@@ +217,5 @@
>  skip-if = e10s
>  [browser_bug565667.js]
>  run-if = toolkit == "cocoa"
>  [browser_bug567306.js]
> +skip-if = e10s # Bug XXX - Needs some massaging to run in e10s

nit: s/massaging/messaging/
Attachment #8549275 - Flags: review?(mdeboer) → review+
Comment on attachment 8549276 [details] [diff] [review]
Get rid of the uncaught rejection

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

Nice catch!
Attachment #8549276 - Flags: review?(mdeboer) → review+
Comment on attachment 8549278 [details] [diff] [review]
Fix test_findbar.xul

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

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +92,5 @@
>      let onPageShow = Task.async(function* () {
>        testNormalFind();
>        gFindBar.close();
>        ok(gFindBar.hidden, "Failed to close findbar after testNormalFind");
> +      yield openFindBar();

I *think* the convention is more or less to use `Findbar`, not `FindBar`. Can you make this small nitty change?

@@ +104,5 @@
>        ok(gFindBar.hidden, "Failed to close findbar after testQuickFindText");
>        testFindWithHighlight();
>        gFindBar.close();
>        ok(gFindBar.hidden, "Failed to close findbar after testFindWithHighlight");
> +      let findbarSelection = testFindbarSelection();

You can change this to `yield Task.spawn(testFindbarSelection)`, which will in effect do the same as the loop you introduce below.

@@ -124,2 @@
>        function checkFindbarState(aTestName, aExpSelection) {
> -        document.getElementById("cmd_find").doCommand();

<3 that you're changing this to the `openFindbar()` pattern!

@@ +288,5 @@
>  
>      function testAutoCaseSensitivityUI() {
>        var matchCaseCheckbox = gFindBar.getElement("find-case-sensitive");
>        var matchCaseLabel = gFindBar.getElement("match-case-status");
> +

nit: no newline necessary I think...

@@ +346,5 @@
>      function testFindWithHighlight() {
>        //clearFocus();
>        gFindBar._findField.value = "";
>  
> +      let oldGetInitialSelection = gFindBar.browser.finder.getInitialSelection;

Can you document why you're doing this for future reference?
Attachment #8549278 - Flags: review?(mdeboer) → review+
Comment on attachment 8549802 [details] [diff] [review]
Add a promise for findbar initialization

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

r- due to promise usage type, but expect instant-re-review when you upload a new patch ;-)

::: toolkit/content/widgets/findbar.xml
@@ +1044,5 @@
>        <!--
> +        - For tests that need to know when the find bar is finished
> +        - initializing, we store a promise to notify on.
> +        -->
> +      <field name="_startFindPromise">null</field>

The convention in findbar.xml kinda is that fields and property definition blocks are grouped together in the implementation block. Can you move this one there too?

@@ +1070,5 @@
>                                 --this._flashFindBar);
>            }
>  
> +          let { Promise } = Components.utils.import("resource://gre/modules/Promise.jsm", {});
> +          this._startFindPromise = Promise.defer();

`Promise.defer` is deprecated and `new Promise()` is the desired replacement. Can you change that here?

Read all about it at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Attachment #8549802 - Flags: review?(mdeboer) → review-
Once we're done here, which I expect is very soon, you might want to consider folding your patches a bit.
I'm probably going to fold everything into a single patch in order to land.
Created attachment 8550426 [details] [diff] [review]
Add a promise for findbar initialization
Attachment #8549802 - Attachment is obsolete: true
Attachment #8550426 - Flags: review?(mdeboer)
Created attachment 8550427 [details] [diff] [review]
Fix test_findbar.xul
Attachment #8549278 - Attachment is obsolete: true
Attachment #8550427 - Flags: review+
Comment on attachment 8550426 [details] [diff] [review]
Add a promise for findbar initialization

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

::: toolkit/content/widgets/findbar.xml
@@ +1071,5 @@
>            }
>  
> +          let {PromiseUtils} =
> +            Components.utils.import("resource://gre/modules/PromiseUtils.jsm", {});
> +          this._startFindPromise = PromiseUtils.defer();

nit: 'technically', this is `_startFindDeferred`
Attachment #8550426 - Flags: review?(mdeboer) → review+
Created attachment 8550584 [details] [diff] [review]
For checkin

There's a green try run pointed to by comment 45.
Attachment #8545453 - Attachment is obsolete: true
Attachment #8545454 - Attachment is obsolete: true
Attachment #8545455 - Attachment is obsolete: true
Attachment #8545458 - Attachment is obsolete: true
Attachment #8549273 - Attachment is obsolete: true
Attachment #8549275 - Attachment is obsolete: true
Attachment #8549276 - Attachment is obsolete: true
Attachment #8550426 - Attachment is obsolete: true
Attachment #8550427 - Attachment is obsolete: true
Attachment #8550584 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/230d46f387f2
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/230d46f387f2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38

Comment 50

4 years ago
Reproduced with Nightly 34.0a1 on Windows 7 x64.
Verified as fixed on Nightly 40.0a1 (Build ID: 20150331030204) under Windows 7 x64, Ubuntu 14.04 x32 and Mac OS X 10.9.5: the selection is present in Find bar when using Ctrl+F (Win and Linux) or Cmd+E (OSX) with e10s enabled.
Status: RESOLVED → VERIFIED
Depends on: 1198465
Depends on: 1358728
You need to log in before you can comment on or make changes to this bug.