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

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: mikedeboer)

Tracking

({regression})

unspecified
mozilla32
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox30+ verified, firefox31 verified, firefox32 verified, b2g-v1.4 fixed)

Details

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

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

5 years ago
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.

Comment 1

5 years ago
WAG. Something in Bug 326743 regressed normal selection prefill
What is your setting for: accessibility.typeaheadfind.prefillwithselection
Assignee

Comment 2

5 years ago
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...
Reporter

Comment 3

5 years ago
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.
Assignee

Comment 4

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

Comment 6

5 years ago
(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
Assignee

Comment 7

5 years ago
(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?]
Assignee

Comment 8

5 years ago
Ehsan, what do you think of this? It should work for you methinks...
Attachment #8416490 - Flags: review?(ehsan)
Reporter

Comment 9

5 years ago
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+
Assignee

Comment 10

5 years ago
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.
Assignee

Comment 11

5 years ago
And backed out again in https://hg.mozilla.org/integration/fx-team/rev/f4782d7db879.

I need to update a test, apparently.
Assignee

Comment 12

5 years ago
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+
Reporter

Comment 14

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

Comment 15

5 years ago
Ah, apologies. I'll attach a patch soon. Are you able to review that quickly?
Flags: needinfo?(mdeboer)
Assignee

Comment 16

5 years ago
Posted patch patch 2: follow-up (obsolete) — Splinter Review
Attachment #8418023 - Flags: review?(ehsan)
Assignee

Comment 17

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

Comment 18

5 years ago
Backed out in https://hg.mozilla.org/integration/fx-team/rev/aa15867d381b.
This is not my finest moment. *sigh*
Assignee

Comment 19

5 years ago
Attachment #8417929 - Attachment is obsolete: true
Attachment #8418024 - Attachment is obsolete: true
Attachment #8418024 - Flags: review?(ehsan)
Attachment #8418068 - Flags: review?(ehsan)
Assignee

Comment 20

5 years ago
Attachment #8418068 - Attachment is obsolete: true
Attachment #8418068 - Flags: review?(ehsan)
Attachment #8418070 - Flags: review?(ehsan)
Assignee

Comment 21

5 years ago
Try run accompanying patch v1.2: https://tbpl.mozilla.org/?tree=Try&rev=78d8a0a900ab
Reporter

Comment 22

5 years ago
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+
Reporter

Comment 23

5 years ago
Or just use SimpleTest.pushPrefEnv.
Assignee

Comment 24

5 years ago
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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee

Comment 27

5 years ago
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.