Closed Bug 1310737 Opened 8 years ago Closed 8 years ago

Keyword bookmarks don't expand keyword query "%S" or POST data


(Firefox :: Bookmarks & History, defect, P1)




Firefox 52
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- verified
firefox52 --- verified


(Reporter: flod, Assigned: mak)


(Depends on 1 open bug)


(Keywords: regression, Whiteboard: [fxsearch])


(2 files)

I have several keyword bookmarks using %S, and they stopped working in Nightly. I can reproduce in at least 52.0a1 (2016-10-16), on two different machines.

For example I have a bookmark with keyword "b", "b 123456" takes me to instead of
I suspect bug 1306639, now it's the backend generating the url, and we're doing
replace("%s", queryString); fixing that would be trivial but...

The browser code is far more complex

Indeed we likely also broke POST keywords.
It may be the right time to finally move the code to the backend and remove getShortcutOrURIAndPostData from urlbarbindings (and maybe even simplify it for other consumers).
Blocks: 1306639
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
Summary: Keyword bookmarks don't expand keyword query "%S" → Keyword bookmarks don't expand keyword query "%S" or POST data
Assignee: nobody → mak77
we also need to fix the encoding (see duped bug 1310953) but it should come for free once we port getShortcutOrURI
Blocks: 1126143
Blocks: 1312018
Blocks: 1313620
Ugh, I forgot bug 1312018. will update the patch and tests for it.
Looks like there were failures on try.  Does that try push on reviewboard correspond to the latest revision?
the X is fixed, the b-c are not.
The problem is simple though, by removing getShortcutOrURI from handleCommand, it doesn't work anymore to just set .value and press Enter.
There are 2 ways:
1. update these tests to do the "right" thing (simulate input like a normal user)
2. reintroduce getShortcutOrURI in the handleCommand else branch

While I was initially tempted to go path 1, I then thought the most add-ons compatible choice is path 2. Since most of the encoding/decoding code is now shared between GSOU and UC, it should still be a good compromise.
there are still a couple things I'm thinking about.
1. I'm not a big fan of the fallback I introduced because creating a URL is more expensive than just checking for some boolean... but it should be acceptable. So far I can't think of a better way to understand if the result came from the search component or not.
2. It would be great to provide a way for add-ons to intercept Enter and replace the result, for bug 1313620. I'm wondering something about overrideValue. I'll think about that.
Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

This is a pretty big patch but I've looked it over a couple of times now and don't see anything clearly wrong (naturally!)...

::: browser/base/content/browser.js:2074
(Diff revision 2)
> - * @param callback (optional, deprecated)
> - *        The callback function invoked when done. This parameter is
> - *        deprecated, please use the Promise that is returned.
>   *
> - * @return Promise<{ postData, url, mayInheritPrincipal }>
> + * @return {Promise}
> + * @resolves <{ url, postData, mayInheritPrincipal }>. If it's not possible

Nit: I think the < and > here are unnecessary and a little confusing.
Attachment #8805585 - Flags: review?(adw) → review+
Blocks: 1232132
I actually found a bug in browser_urlbarEnter.js test and thus this is likely to fix an intermittent.
Pushed by
Urlbar doesn't properly handle %S and POST keywords. r=adw
Blocks: 1314013
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi ::mak,
Since this bug is a regression and also affects 51, do you think it's worth uplifting to 51 if this patch is not too risky??
Flags: needinfo?(mak77)
No longer blocks: 1312018
Yep, the bug 1312018 fixed this problem.
(In reply to Gerry Chang [:gchang] from comment #18)
> Hi ::mak,
> Since this bug is a regression and also affects 51, do you think it's worth
> uplifting to 51 if this patch is not too risky??

Yes, I plan to request approval for an uplift, I was actually trying to understand if bug 1303333 will be uplifted cause it bitrots this patch. I will ask Alessio today.
Flags: needinfo?(mak77)
OK, we will wait for bug 1303333 data validation and uplift approval, that should happen on Monday, otherwise we should heavily unbitrot both that patch and this one, since they partly touch the same code. We'll queue up after it.
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
See Also: → 1315514
Depends on: 1315514
Attached patch Aurora patchSplinter Review
NOTE: this also includes the typo fix in bug 1315514 (along with 2 additional tests from there)

Approval Request Comment
[Feature/regressing bug #]: Bug 1306639
[User impact if declined]: A large number of bookmark keywords don't work in the location bar
[Describe test coverage new/current, TreeHerder]: automated tests in Nightly
[Risks and why]: The patch is mostly moving existing code to share it across components and we have a ton of tests on autocomplete. The risk looks acceptable.
[String/UUID change made/needed]: none

This is the unbitrotted patch for Aurora, I'm currently running it through Try, I'll take care of the push by myself.
Attachment #8809058 - Flags: approval-mozilla-aurora?
Comment on attachment 8809058 [details] [diff] [review]
Aurora patch

This patch fixes a regression related to keyword bookmarks and also includes tests. Take it in 51 aurora.
Attachment #8809058 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I have reproduced this bug with Nightly 52.0a1 (2016-10-17) (32-bit)! 

This bug's fix is verified with latest Developer Edition (Aurora) and Beta.

Build ID    :	20170105004018
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0

Build ID    :  	20170105155013
User Agent  :	Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0

Thanks Almas!
Depends on: 1534140
You need to log in before you can comment on or make changes to this bug.