1485987 - Generated search shortcuts paste links after being copied instead of the string in newtab

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Cristian Fogel [:cfogel]]

Attachment: 2018-08-24 16-14-21.mp4

[Affected versions]:
- 62.0b20, 63.0a1(build id: 20180823220048)

[Affected platforms]:
- win 10x64, ubuntu 16.04LTS, macOS 10.13

[Steps to reproduce]:
1. Launch Firefox;
2. Access the newtab page;
3. Click on the button for google or amazon search;
4. Copy the generated string from the address bar "@google ";
5. Paste it anywhere;

[Expected result]:
- the exact string is pasted

[Actual result]:
- "http://google/" is pasted

[Regression range]:
- will check if it's a regression asap

[Additional notes]:
- attached screen recording with the issue;
- the link for google is not working either;

Comment 1

[Cristian Fogel [:cfogel]]

Copying without the space pastes the string properly.

Comment 2

[Cristian Fogel [:cfogel]]

The same behaviour is found for the other @search shortcuts (ex: amazon, bing, duckduckgo, ebay, yandex, twitter)

Comment 3

[Cristian Fogel [:cfogel]]

Not a regression, introduced with 1482205.

Comment 4

[Jenn Chaulk (:jchaulk)]

Having trouble reproducing on the latest nightly.  Tried on both Mac and Windows.

Comment 5

[Cristian Fogel [:cfogel]]

Still reproducible with current nightly 63.0a1 (2018-08-28), checked over win10.

It's really important to copy the space after the string as well, otherwise it will not happen.
ex: [@google ] leads to the issue, while [@google] is fine.

Comment 6

[Adam Hillier :ahillier]

I can also replicate this on Mac. It doesn't happen if you manually type in "@google " but does if you click through from the search shortcut.

Comment 7

[Drew Willcoxon :adw]

When you type it manually, we end up here in the copy code because this.valueIsTyped is true: https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/browser/base/content/urlbarBindings.xml#1321

But when you click a tile, we end up farther down that method and fix up the value, which is why you end up with a weird pseudo URL.  I'm not sure what the best fix is.  Maybe urlbar.search() should just set valueIsTyped to true.

Is this a high priority for Activity Stream?

Comment 8

[Drew Willcoxon :adw]

Attachment: Bug 1485987 - Make urlbar.search() fire an input event.

`urlbar.search` needs to simulate the user's typing. Fire an input event, as we do elsewhere to simulate it.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 9010105 [details]
Bug 1485987 - Make urlbar.search() fire an input event.

Marco Bonardo [::mak] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9828a37ea75f
Make urlbar.search() fire an input event. r=mak

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9828a37ea75f

Comment 12

[Cristian Fogel [:cfogel]]

Issue is no longer reproducible on 64.0a1 (2018-09-21) over macOS 10.13.6, Ubuntu 16.04, win10x64.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Drew, this bug is a P1, does it make sense to uplift it to 63 or can it ride the trains to 64? Thanks

Comment 14

[Drew Willcoxon :adw]

Yes, this seems OK to uplift.  Will request

Comment 15

[Drew Willcoxon :adw]

Attachment: Beta/63 patch

Approval Request Comment
[Feature/Bug causing the regression]:
Search shortcut tiles on the new tab page -- bug 1480505 added the blue highlight in the urlbar that this bug is about, and bug 1479806 is the main meta bug for the project

[User impact if declined]:
The original bug (bug 1480505) landed in 62, and this bug landed in 64.  So if declined, users will have to wait for two releases before this bug is fixed.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
See comment 0

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Low risk

[Why is the change risky/not risky?]:
The patch mainly touches a single urlbar function, urlbar.search(), which is only used by the search tiles on the newtab page

[String changes made/needed]:
None

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9011031 [details] [diff] [review]
Beta/63 patch

Low risk patch fixing a bug in a new 62 feature, uplift approved for 63 beta 9, thanks.

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5fa1ca7189f9

Comment 18

[Cristian Fogel [:cfogel]]

Fix verified with 63.0b9 on win10x64, macOS 10.9, Ubuntu 16.04.