Closed
Bug 978861
Opened 10 years ago
Closed 10 years ago
The find bar no longer initializes the search string to the selected text in the document
Categories
(Toolkit :: Find Toolbar, defect)
Tracking
()
VERIFIED
FIXED
mozilla32
People
(Reporter: ehsan.akhgari, Assigned: mikedeboer)
References
Details
(Keywords: regression, Whiteboard: p=0 s=it-32c-31a-30b.1 [qa!])
Attachments
(1 file, 6 obsolete files)
5.06 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
WAG. Something in Bug 326743 regressed normal selection prefill What is your setting for: accessibility.typeaheadfind.prefillwithselection
Assignee | ||
Comment 2•10 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...
Blocks: 326743
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 3•10 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.
status-firefox30:
--- → affected
tracking-firefox30:
--- → ?
Updated•10 years ago
|
Updated•10 years ago
|
Blocks: fxdesktoptriage
Updated•10 years ago
|
Keywords: regression
Assignee | ||
Comment 4•10 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.)
Comment 5•10 years ago
|
||
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?
Updated•10 years ago
|
Whiteboard: p=0
Reporter | ||
Comment 6•10 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.
Updated•10 years ago
|
Flags: firefox-backlog?
Updated•10 years ago
|
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•10 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
Updated•10 years ago
|
No longer blocks: fxdesktopbacklog
Updated•10 years ago
|
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•10 years ago
|
||
Ehsan, what do you think of this? It should work for you methinks...
Attachment #8416490 -
Flags: review?(ehsan)
Reporter | ||
Comment 9•10 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•10 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•10 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•10 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+
Assignee | ||
Comment 13•10 years ago
|
||
Re-landed on fx-team as: https://hg.mozilla.org/integration/fx-team/rev/df4ae5ba7d6e
Reporter | ||
Comment 14•10 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•10 years ago
|
||
Ah, apologies. I'll attach a patch soon. Are you able to review that quickly?
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8418023 -
Flags: review?(ehsan)
Assignee | ||
Comment 17•10 years ago
|
||
now without trailing garbage.
Attachment #8418023 -
Attachment is obsolete: true
Attachment #8418023 -
Flags: review?(ehsan)
Attachment #8418024 -
Flags: review?(ehsan)
Assignee | ||
Comment 18•10 years ago
|
||
Backed out in https://hg.mozilla.org/integration/fx-team/rev/aa15867d381b. This is not my finest moment. *sigh*
Assignee | ||
Comment 19•10 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•10 years ago
|
||
Attachment #8418068 -
Attachment is obsolete: true
Attachment #8418068 -
Flags: review?(ehsan)
Attachment #8418070 -
Flags: review?(ehsan)
Assignee | ||
Comment 21•10 years ago
|
||
Try run accompanying patch v1.2: https://tbpl.mozilla.org/?tree=Try&rev=78d8a0a900ab
Reporter | ||
Comment 22•10 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•10 years ago
|
||
Or just use SimpleTest.pushPrefEnv.
Assignee | ||
Comment 24•10 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+
Assignee | ||
Comment 25•10 years ago
|
||
Fx-team: https://hg.mozilla.org/integration/fx-team/rev/ecd6d1b3670c
Updated•10 years ago
|
Whiteboard: p=0 s=it-32c-31a-30b.1 [qa?] → p=0 s=it-32c-31a-30b.1 [qa+]
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ecd6d1b3670c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 27•10 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?
Updated•10 years ago
|
QA Contact: cornel.ionce
Updated•10 years ago
|
Attachment #8418116 -
Flags: approval-mozilla-beta?
Attachment #8418116 -
Flags: approval-mozilla-beta+
Attachment #8418116 -
Flags: approval-mozilla-aurora?
Attachment #8418116 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 28•10 years ago
|
||
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/7ea30f70ccd6 Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/b431c1c19a26
Comment 29•10 years ago
|
||
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
status-firefox32:
--- → verified
Updated•10 years ago
|
status-firefox31:
--- → fixed
Comment 30•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b431c1c19a26
status-b2g-v1.4:
--- → fixed
Comment 31•10 years ago
|
||
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.
Description
•