Closed Bug 1055508 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
e10s + ---

People

(Reporter: ehsan.akhgari, Assigned: mrbkap)

References

Details

Attachments

(1 file, 18 obsolete files)

For checkin
34.47 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
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.
Could this be the same issue as bug 1053045?
Flags: needinfo?(ehsan)
(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)
Assignee: nobody → atifrea
Blocks: e10sa11y2
No longer blocks: e10sa11y2
This is just a small patch to use a JS feature that we can use.
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.
Attached patch First patch, updated (obsolete) — — Splinter Review
Attachment #8539422 - Attachment is obsolete: true
Attachment #8540211 - Flags: review+
Attached patch Second patch, updated (obsolete) — — Splinter Review
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+
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)
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)
Attached patch patch 1 (obsolete) — — Splinter Review
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+
Attached patch Patch 2 (obsolete) — — Splinter Review
Attachment #8545454 - Flags: review+
Attached patch Patch 3 - add support for cmd+e (obsolete) — — Splinter Review
Attachment #8545455 - Flags: review?(mdeboer)
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
Flags: qe-verify+
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.
This actually fixes a few tests that assume that the findbar is synchronously focused when the findbar opens.
Attachment #8549273 - Flags: review?(mdeboer)
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)
Attachment #8549275 - Flags: review?(mdeboer)
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)
Attached patch Fix test_findbar.xul (obsolete) — — Splinter Review
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)
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.
Attachment #8549802 - Attachment is obsolete: true
Attachment #8550426 - Flags: review?(mdeboer)
Attached patch Fix test_findbar.xul (obsolete) — — Splinter Review
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+
Attached patch For checkin — — Splinter Review
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/mozilla-central/rev/230d46f387f2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
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.

Attachment

General

Creator:
Created:
Updated:
Size: