Closed Bug 978861 Opened 7 years ago Closed 6 years ago

The find bar no longer initializes the search string to the selected text in the document

Categories

(Toolkit :: Find Toolbar, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 + verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed

People

(Reporter: ehsan, Assigned: mikedeboer)

References

Details

(Keywords: regression, Whiteboard: p=0 s=it-32c-31a-30b.1 [qa!])

Attachments

(1 file, 6 obsolete files)

STR:

1. Load a web page, and select some text on it.
2. Press Cmd+F.  The search string is no longer prefilled from the selection.
WAG. Something in Bug 326743 regressed normal selection prefill
What is your setting for: accessibility.typeaheadfind.prefillwithselection
Ehsan, if you have `accessibility.typeaheadfind.prefillwithselection` set to true, this is indeed a regression from bug 326743. One that is easy to fix, but still...
accessibility.typeaheadfind.prefillwithselection is true, which is the default:

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#501

Nominating this for tracking.
OK, this is what's going on here:

If the global find clipboard is empty - you never searched for anything in a native app before during the lifetime of your OS session - the find bar will take the current selection from the page, if there is one.

After that, the (global) find clipboard data is set to the string you searched for.

In other words, `accessibility.typeaheadfind.prefillwithselection` has become less prevalent in order for the find bar to integrate better with the OS.

There's a definite trade-off here: either we turn off the (new) global clipboard, i.e. backout bug 326743, or we keep it as is and accept the lesser useful pref/ non-standard find bar behavior on OSX.

(IMHO, but that's kinda obvious, we should maintain the status quo. We're a native app on OSX and we should adhere to their basic UX, which is ultimately most beneficial for our users.)
Is there no way to only use the OSX global clipboard only if there is no selected text in the browser? Basically:
> if(accessibility.typeaheadfind.prefillwithselection && browserHasSelectedText)
>   use browserSelectedText;
> else
>   use globalClipboard;
I honestly thought that was what bug 326743 did (or was supposed to do) already, from http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1046, which is pretty much made irrelevant by http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1125 now that I look at it.

Why was that onFindFieldFocus handler added? Is this how OSX native apps behave normally in this situation?
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: p=0
(In reply to comment #4)
> OK, this is what's going on here:
> 
> If the global find clipboard is empty - you never searched for anything in a
> native app before during the lifetime of your OS session - the find bar will
> take the current selection from the page, if there is one.

You mean the first time you search after rebooting?

> After that, the (global) find clipboard data is set to the string you searched
> for.
> 
> In other words, `accessibility.typeaheadfind.prefillwithselection` has become
> less prevalent in order for the find bar to integrate better with the OS.
> 
> There's a definite trade-off here: either we turn off the (new) global
> clipboard, i.e. backout bug 326743, or we keep it as is and accept the lesser
> useful pref/ non-standard find bar behavior on OSX.
> 
> (IMHO, but that's kinda obvious, we should maintain the status quo. We're a
> native app on OSX and we should adhere to their basic UX, which is ultimately
> most beneficial for our users.)

That is not very obvious to me.

Firstly, I'm not even sure what clipboard this is talking about.  I just tried to search for something in Chrome and then switched to Firefox and Cmd+F prefills the find bar with that text.  But when I select something on a page in Firefox, and copy it to the normal clipboard (i.e., Cmd+C) and then open the find bar again, then the findbar is filled with what the regular clipboard contains.  But Chrome still knows about the previous thing I searched for.  I'm not really sure what the status quo is any more.

I do get the argument of making the default behavior match the native OS X behavior.  But if we want to do that, I think the correct way would be to flip the prefillwithselection pref to false on OSX (because we're not prefilling the find bar with the selection!), and disable the usage of that global clipboard for the findbar if someone manually sets that pref to true.  That way, people like me who are negatively affected by this change can toggle the default pref and set it back to true, and others will get this new shiny OS X behavior.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Summary: The find bar no longer initializes the search string to the selected text in the document → [UX] The find bar no longer initializes the search string to the selected text in the document
Whiteboard: p=0 → [ux] p=0
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> I do get the argument of making the default behavior match the native OS X
> behavior.  But if we want to do that, I think the correct way would be to
> flip the prefillwithselection pref to false on OSX (because we're not
> prefilling the find bar with the selection!), and disable the usage of that
> global clipboard for the findbar if someone manually sets that pref to true.
> That way, people like me who are negatively affected by this change can
> toggle the default pref and set it back to true, and others will get this
> new shiny OS X behavior.

I can completely get behind this! I can implement this quite easily... taking this bug.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
No longer blocks: fxdesktopbacklog
Summary: [UX] The find bar no longer initializes the search string to the selected text in the document → The find bar no longer initializes the search string to the selected text in the document
Whiteboard: [ux] p=0 → p=0 s=it-32c-31a-30b.1 [qa?]
Ehsan, what do you think of this? It should work for you methinks...
Attachment #8416490 - Flags: review?(ehsan)
Comment on attachment 8416490 [details] [diff] [review]
Patch v1: disable prefillWithSelection on OSX by default, fix its semantics when enabled

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

Yes, that is exactly what I had in mind.  Thanks!
Attachment #8416490 - Flags: review?(ehsan) → review+
Well, then let's push it to trunk, shall we?

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

Thanks, Ehsan!

I'll request uplift approval when it's been merged to m-c.
And backed out again in https://hg.mozilla.org/integration/fx-team/rev/f4782d7db879.

I need to update a test, apparently.
Updated unit-test. It's actually nicer now, since we can opt-in to the global find-clipboard on OSX.

Carrying over r=ehsan.

Accompanying try push: https://tbpl.mozilla.org/?tree=Try&rev=607d8b6c5fd4. Waiting for it to green out.
Attachment #8416490 - Attachment is obsolete: true
Attachment #8417929 - Flags: review+
You forgot to unset the pref in the test, which changes the environment in which all of the following tests run in.  Can you please fix that?
Flags: needinfo?(mdeboer)
Ah, apologies. I'll attach a patch soon. Are you able to review that quickly?
Flags: needinfo?(mdeboer)
Attached patch patch 2: follow-up (obsolete) — Splinter Review
Attachment #8418023 - Flags: review?(ehsan)
Attached patch Patch 2: follow-up (obsolete) — Splinter Review
now without trailing garbage.
Attachment #8418023 - Attachment is obsolete: true
Attachment #8418023 - Flags: review?(ehsan)
Attachment #8418024 - Flags: review?(ehsan)
Backed out in https://hg.mozilla.org/integration/fx-team/rev/aa15867d381b.
This is not my finest moment. *sigh*
Attachment #8417929 - Attachment is obsolete: true
Attachment #8418024 - Attachment is obsolete: true
Attachment #8418024 - Flags: review?(ehsan)
Attachment #8418068 - Flags: review?(ehsan)
Attachment #8418068 - Attachment is obsolete: true
Attachment #8418068 - Flags: review?(ehsan)
Attachment #8418070 - Flags: review?(ehsan)
Try run accompanying patch v1.2: https://tbpl.mozilla.org/?tree=Try&rev=78d8a0a900ab
Comment on attachment 8418070 [details] [diff] [review]
Patch v1.2: disable prefillWithSelection on OSX by default, fix its semantics when enabled

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

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +64,5 @@
>      function ok(condition, message) {
>        window.opener.wrappedJSObject.SimpleTest.ok(condition, message);
>      }
>      function finish() {
> +      prefsvc.setBoolPref(kPrefillPref, gOldPrefillValue);

You want clearUserPref here, so that you don't need to remember the old value.
Attachment #8418070 - Flags: review?(ehsan) → review+
Or just use SimpleTest.pushPrefEnv.
Went with Ehsan's suggestion to use `SpecialPowers.pushPrefEnv`. (cue Borat voice) It's niiize, I like!

Carrying over r=ehsan.
Attachment #8418070 - Attachment is obsolete: true
Attachment #8418116 - Flags: review+
Whiteboard: p=0 s=it-32c-31a-30b.1 [qa?] → p=0 s=it-32c-31a-30b.1 [qa+]
https://hg.mozilla.org/mozilla-central/rev/ecd6d1b3670c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8418116 [details] [diff] [review]
Patch v1.3: disable prefillWithSelection on OSX by default, fix its semantics when enabled

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 326743
User impact if declined: user is not able to opt-out of the find clipboard implementation and the find bar textbox won't be pre-filled with the text of a selection, when text in the document is selected.
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor.
String or IDL/UUID changes made by this patch: n/a.
Attachment #8418116 - Flags: approval-mozilla-beta?
Attachment #8418116 - Flags: approval-mozilla-aurora?
QA Contact: cornel.ionce
Attachment #8418116 - Flags: approval-mozilla-beta?
Attachment #8418116 - Flags: approval-mozilla-beta+
Attachment #8418116 - Flags: approval-mozilla-aurora?
Attachment #8418116 - Flags: approval-mozilla-aurora+
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:32.0) Gecko/20100101 Firefox/32.0

"accessibility.typeaheadfind.prefillwithselection" is now disabled by default on Mac OS X using Latest Nightly, build ID: 20140507030202.

When this pref is enabled, the find bar textbox will be pre-filled with the selected text.

Leaving [qa+] until I can confirm this fix on Fx 30 and Fx 31.
Status: RESOLVED → VERIFIED
Verified as fixed on:
- Fx 30 beta 3, build ID: 20140508121358
- Fx 31 Aurora, build ID: 20140508004006.
Whiteboard: p=0 s=it-32c-31a-30b.1 [qa+] → p=0 s=it-32c-31a-30b.1 [qa!]
You need to log in before you can comment on or make changes to this bug.