Closed Bug 1111947 Opened 5 years ago Closed 5 years ago

Dropping text on the searchbar shouldn't search it immediately

Categories

(Firefox :: Search, defect)

x86_64
Windows 7
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox34 --- wontfix
firefox35 - wontfix
firefox36 - wontfix
firefox37 - verified
firefox38 --- verified

People

(Reporter: alice0775, Assigned: florian)

References

(Depends on 1 open bug)

Details

(Keywords: regression, ux-discovery)

Attachments

(1 file)

Not possible to recognize which search engine will be used before performing drop text to Searchbar

I.e., transmits the data to the unexpected site.

The browser should be explicit search engine to be used.
Philipp, I don't think we have ever discussed this case; do you see anything obvious we can do here?
Flags: needinfo?(philipp)
Summary: Not possible to recognize which search engine will be used before performing drop text to Searchbar → Dropping text on the searchbar searches it immediately without showing which engine will be used
As far as I understand, this is polishing. We won't block the release for this.
In addition to dropping text this also happens when right-clicking in the search field and selecting "paste" / "paste & search" and when jumping into the search field with ctrl+k and ctrl+v.

This ultimately stems from the removal of the search engine name from the search box. This is something that should be reconsidered.
The behavior that the search bar triggers a search automatically seems odd in general.
The awesomebar just shows the text as if the user has pasted it manually. Seems like the search box should do the same.
Flags: needinfo?(philipp)
Rephrasing the summary to match my understanding of comment 4.
Summary: Dropping text on the searchbar searches it immediately without showing which engine will be used → Dropping text on the searchbar shouldn't search it immediately
Attached patch PatchSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57f8515822f1

Also asking for ui-review for confirmation, because the behavior we are changing has been in place since probably Firefox 1.0 (I couldn't find the exact check-in, but it's from the aviary branch).
Assignee: nobody → florian
Attachment #8554541 - Flags: ui-review?(philipp)
Attachment #8554541 - Flags: review?(dtownsend)
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Comment on attachment 8554541 [details] [diff] [review]
Patch

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

::: browser/components/search/test/browser_426329.js
@@ -226,5 @@
>          "Shift+MiddleClick loaded results in background tab");
>    is(event.originalTarget.URL, expectedURL(searchBar.value), "testShiftMiddleClick opened correct search page");
>  });
>  
> -add_task(function testDropText() {

Can we still test that the popup opened here?
(In reply to Dave Townsend [:mossop] from comment #7)

> ::: browser/components/search/test/browser_426329.js
> @@ -226,5 @@
> >          "Shift+MiddleClick loaded results in background tab");
> >    is(event.originalTarget.URL, expectedURL(searchBar.value), "testShiftMiddleClick opened correct search page");
> >  });
> >  
> > -add_task(function testDropText() {
> 
> Can we still test that the popup opened here?

How would this be different from the test I added in browser_searchbar_openpopup.js?
Comment on attachment 8554541 [details] [diff] [review]
Patch

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

Never mind, missed the added test. r+ assuming this gets ui+
Attachment #8554541 - Flags: review?(dtownsend) → review+
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> Created attachment 8554541 [details] [diff] [review]
> Patch
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=57f8515822f1

This is consistently failing on Windows XP debug (but not on Windows XP opt, or Windows 7 debug). I don't see any obvious way to debug this. Is there an easy way to disable the test only for XP debug (but keep it running on 7 debug)?
Flags: needinfo?(dtownsend)
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > Created attachment 8554541 [details] [diff] [review]
> > Patch
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=57f8515822f1
> 
> This is consistently failing on Windows XP debug (but not on Windows XP opt,
> or Windows 7 debug). I don't see any obvious way to debug this. Is there an
> easy way to disable the test only for XP debug (but keep it running on 7
> debug)?

I am reliably informed that this should work:

skip-if = (os == "win" && os_version == "5.1" && debug)
Flags: needinfo?(dtownsend)
https://hg.mozilla.org/integration/fx-team/rev/a882d834faee
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
sorry had to back this out for merge conflicts down from m-c in  browser_searchbar_openpopup.js
https://hg.mozilla.org/mozilla-central/rev/4f63c1180219
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
QA Contact: petruta.rasa
Comment on attachment 8554541 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1088660.
[User impact if declined]: Dropping text on the searchbox starts a search without the user having an opportunity to see which search engine will be used.
[Describe test coverage new/current, TreeHerder]: the patch contains a test.
[Risks and why]: Not much technical risk; the code change is a one liner and has tests, but some users may be upset by this new behavior, so I don't think we should uplift it to beta; better have a whole 6 weeks for people to file bugs if there are edge cases where we should polish the behavior.
[String/UUID change made/needed]: none
Attachment #8554541 - Flags: approval-mozilla-aurora?
Comment on attachment 8554541 [details] [diff] [review]
Patch

I agree that we should be cautious about changing behaviour that some users may have come to rely upon. Let's take this simple change on Aurora and get a full Beta cycle to test as well. Aurora+
Attachment #8554541 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The search panel is opened when dropping text in it - note that this only happens in non e10s windows (due to bug 936092).
Tested with Nightly 38.0a1 2015-02-08 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> Backed out from Aurora for test failures.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a57cce6af02c
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=569134&repo=mozilla-
> aurora

This is failing only on XP debug. This test is disabled on central for (os == "win" && os_version == "5.1" && debug) (see comment 12).

I think this code confusing because of the backout that happened when merging fx-team and mozilla-central.
I landed in comment 14 with the test disabled, Tomcat backed it out in comment 16 (without reenabling the test), and then relanded it in comment 17 (which I guess is what was uplifted).
Flags: needinfo?(florian)
Verified as fixed using Firefox 37 beta 1 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.
Depends on: 1218162
You need to log in before you can comment on or make changes to this bug.