preventDefault on willSubmitForm inhibits tab-to-search engine creation |
|||||||||||||||||||
Issue descriptionChrome Version : 54.0.2840.6 OS Version: 10.0 On a brand-new profile, visit amazon.com. Use the search box in the page. No tab-to-search entry for Amazon gets added.
,
Sep 28 2016
I'm sure this used to work. Can we get a bisect please? More detailed repro instructions: 1. on a brand-new profile, go to amazon.com. Make sure it fully loads. 2. use the search box on the page to search for something. Make sure the result page fully loads. 3. wait ten seconds. 4. use the omnibox and type amazon.com and press tab. Correct behavior: 5. the omnibox should show a "Search amazon.com:" box. 6. also, in this case, you should be able to go to about:settings "Manage search engines" section and see amazon.com listed in the "other search engines" category. Incorrect behavior: 5. pressing tab scrolls through the omnibox dropdown. 6. also, in this case, if you go to about:settings "Manage search engines" section, amazon.com will not be listed anywhere.
,
Sep 30 2016
Able to reproduce the issue on the latest canary(55.0.2876.0) and the latest stable(53.0.2785.143) on Windows-10, Mac OS 10.11.6 and Linux Ubuntu 14.04 as per the test steps in C#2. Same behavior is seen on amazon.com, on older chrome version: 30.0.1549.0. Attached is the screen-cast of the same. Triaging this as Non-regression issue and for further investigation.
,
Sep 30 2016
ajha@: thanks for checking! pkasting@: how strange. Has this been broken for that long? It works--the search engine is auto-generated--in my current install of Safari (which, by the way, has javascript disabled). But I am having trouble figuring out how Safari knows to create the search engine. Using the web inspector, a search for "opensearch" comes up empty. You're more familiar with the opensearch standard than me; can you please figure out how Safari is creating this engine correctly? According to my read of the document http://www.opensearch.org/Specifications/OpenSearch/1.1#Autodiscovery , all the possible ways of notifying a browser of the existence of an opensearch ability use the word "opensearch" somewhere. I must be missing something.)
,
Sep 30 2016
I would look for "search" rather than "opensearch" -- "link rel='search'" is the canonical OSDD route I know (but I don't see that on the Amazon page I looked at). I don't know how our autodetection works. I would assume that something about Amazon changed in a way we no longer detect but Safari does, but it's also possible Amazon is serving Safari a different source than Chrome and they need to fix it. Either way someone who knows anything at all about web development, i.e. can use the dev tools to look at differences between these pages, should be involved here. Since none of the above useful knowledge areas are me, I don't think I should own this.
,
Oct 5 2016
> I don't see that on the Amazon page I looked at. Nor do I. > it's also possible Amazon is serving Safari a different source than Chrome That's true, but Safari can auto-generate a search engine from whatever HTML+Javascript it's being served, so we ought to be able to figure out how it does that using Safari's web inspector.
,
Oct 5 2016
,
Oct 14 2016
Asked for help on the opensearch mailing list: https://groups.google.com/d/msg/opensearch/EcT73zePg18/WZPGQLqkAwAJ Thus far, no assistance. :-(
,
Oct 25 2016
,
Oct 28 2016
Downgrading priority because I tried other sites and some work. For example, etsy.com does cause creation of a search engine. Other sites such as walmart.com and target.com do. Yet, they cause creation of a search engine on Safari.
,
Oct 28 2016
Pretty sure one of those "does" or "do"s was meant to be negated (the latter?).
,
Oct 28 2016
> Pretty sure one of those "does" or "do"s was meant to be negated (the latter?). Correct. The latter (walmart and target) do not cause creation of a search engine.
,
Nov 1 2016
My best guess from looking at the sources of these pages is that Chromium only creates a search engine when it finds an opensearch element such as on etsy.com <link type="application/opensearchdescription+xml" rel="search" href="/osdd.php"> but Safari creates one whenever it finds a form with role="search", no need for an opensearch document. It would be useful if someone could look through the Safari WebKit code to confirm.
,
Nov 1 2016
Our docs claim that we don't require an OSDD as long as you do some other stuff: https://www.chromium.org/tab-to-search Maybe we need to expand our heuristics to detect more engines?
,
Nov 1 2016
> Our docs claim that we don't require an OSDD as long as you do some other stuff: > https://www.chromium.org/tab-to-search >>> If the site does not provide a link to an OpenSearch description document but the user submits a form, then Chrome automatically adds the site to the list of searchable sites. There are a number of restrictions with this approach though. In particular the form must generate a GET, must result in a HTTP url, and the form must not have OnSubmit script. Additionally there must be only one input element of type text, no passwords, files or text areas and all other input elements must be in their default state. >>> > Maybe we need to expand our heuristics to detect more engines? Indeed. This HTTP restriction in this day and age is preposterous. We should allow HTTPS search engines. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSearchableFormData.cpp?l=240 That alone could explain the lack of amazon and walmart (both of which are https by default). That doesn't explain target; maybe another heuristic is in play there.
,
Nov 1 2016
I think we originally restricted HTTPS to forms exposing an input element to try to avoid exposing sensitive or secure data/sites as a "search engine". I'm OK with relaxing the restrictions here as long as we think carefully through "how could this go wrong and make my bank/sensitive corporate intranet login a search engine". I bet we could remove the POST restriction (don't we support POST properly now?), but in practice that probably fixes few real sites.
,
Nov 28 2016
,
Nov 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0bc8993c2a2766309e91904b742936202bed2b45 commit 0bc8993c2a2766309e91904b742936202bed2b45 Author: mpearson <mpearson@chromium.org> Date: Mon Nov 28 20:23:41 2016 Auto-Generating Search Engines - Remove HTTP Restriction Remove the restriction that, under certain conditions, we require the protocol of an auto-created custom search engine be HTTP. The condition has no connection with a need for privacy. There is no problem with creating custom search engines for secure sites. In fact, we should be encouraging more sites to be secure, not penalizing those that do so by making it more difficult for them to create a custom search engine. BUG=642848 TEST=amazon.com now gets a custom search engine created Review-Url: https://codereview.chromium.org/2505933005 Cr-Commit-Position: refs/heads/master@{#434706} [modify] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/WebSearchableFormData.cpp [modify] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/tests/WebSearchableFormDataTest.cpp [rename] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/tests/data/search_form_http.html [add] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/tests/data/search_form_https.html
,
Nov 29 2016
amazon.com is now fixed. Walmart and target are still broken. >>> I think we originally restricted HTTPS to forms exposing an input element to try to avoid exposing sensitive or secure data/sites as a "search engine". I'm OK with relaxing the restrictions here as long as we think carefully through "how could this go wrong and make my bank/sensitive corporate intranet login a search engine". >>> I also check a few banks and bank-like places and did not see any errant custom search engines created. Happily, I checked this in well before a branch point, so if anything weird comes up, it'll be easy to revert. I'll morph this into a bug about the remaining bad examples.
,
Nov 29 2016
I added debugging lines. For walmart and target, WebSearchableFormData::WebSearchableFormData() is not called. The two relevant places this can be called are RenderFrameImpl::BeginNavigation() https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSearchableFormData.cpp?l=218 and RenderFrameImpl::willSubmitForm() https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=3246 (It's also called somewhere in the context menu code too.) The amazon.com case goes through second case. Now I (or hopefully someone with knowledge of how form navigations should happen) need to figure out why we're not triggering a willSubmitForm() for walmart and target.
,
Dec 1 2016
It looks like both walmart and target have Javascript event handlers for form submissions that call e.preventDefault(). Thus--I think this is a "thus"--RenderFrameImpl::willSubmitForm() is never called.
,
Dec 1 2016
Heh, I re-checked Safari. It appears my Safari has Javascript disabled. (Don't remember when I set that.) Under these conditions, I previously reported that Safari creates search engines for walmart and target. When I have Javascript enabled, it does not, just like Chrome. I guess Safari isn't doing anything better than Chrome here. And I don't think we can stop sites like these from shooting themselves in the foot. I don't see how if they call preventDefault, we can still decide to create a search engine. I guess I'm coming around to sadly closing this as WontFix, unless someone else has something to say...
,
Dec 1 2016
If willSubmitForm is never called, the form shouldn't be submitted at all, right? So how does the search actually occur?
,
Dec 1 2016
>>>
If willSubmitForm is never called, the form shouldn't be submitted at all, right? So how does the search actually occur?
>>>
Target constructs the destination URL in Javascript and then uses window.location.assign deep in the bowels of the Javascript.
navigateSearch: function(e, t, r, i) {
isSecure ? (e = e.charAt(0) === "/" ? e : "/" + e,
e = e.replace("/mobile", ""),
window.location.assign(domain + e)) : typeof i == "undefined" ? n.history.navigate(e, {
trigger: t,
replace: r
}) : n.history.loadUrl(e)
},
The Walmart Javascript does something similar by assigning this._getWindow().location.href directly.
// _handleEmptyInput is misnamed; it gets called on all form submits.
t.prototype._handleEmptyInput = function(e) {
e.preventDefault();
var t = this._getSearchInput()
, n = this._getWindow().document.querySelector("#global-search-catid")
, r = void 0;
if (n && (r = this._getWindow().document.querySelector("#global-search-catid").value),
/\S/.test(t)) {
var o = "/search/?query=" + t;
r && (o += "&cat_id=" + r),
this._getWindow().location.href = o
}
}
,
Dec 1 2016
I wonder if we should try and detect "navigated to something that would match the destination URL of the form" as an alternative to "submitted the form".
,
Dec 1 2016
We'd have to be careful. I see shopping websites that have a dropdown in the form that let the users select a department to search (clothing, electronics, etc.). These web sites also have links on the page to general category pages (e.g., browse clothing). I can easily imagine that sometime the general browsing link goes to the same as the the search form does, simply with an empty search string. I don't think we should create a search engine in these cases because the user didn't use the web site to search. The form may not have registered in their mind. I guess we should look for non-empty search strings and also maybe non-default value search strings. Are there other things that can bite us here? As for how, this is the only way I see. It seems a bit heavy-handed. On every page load, would we look over the form(s) on the web page, remember them, then wait for any sort of navigation in this renderer and attempt to match the form(s) to the destination URL. (Right now Chrome only remembers the one relevant form on willSubmitForm and then waits for the new navigation to happen. It knows the form is relevant.)
,
Dec 1 2016
Rather than looking at all forms, I would only handle the specific case where a page did a preventDefault() on willSubmitForm and then navigated itself. In other words, I would try to detect this case as a "submitted form", rather than the more aggressive "match any navigation to any form". The "and then" is potentially tricky. "And then" in the same synchronous callstack? Can we detect that?
,
Dec 1 2016
Clarifying the subject line for the remaining issue on this bug and unassigning from myself. Hopefully someone who's an expert on Blink + Javascript interaction can look into it, answer the question ("how do we detect 'and then' navigated?"), and maybe even take this on. Peter, can you nominate someone who knows this area?
,
Dec 1 2016
I'm clueless :( Adding some likely areas and setting as untriaged. For triagers, brief summary: * Chrome detects form submission and tries to create omnibox search engine entries if applicable, to facilitate tab-to-search * Some sites preventDefault on willSubmitForm and then manually navigate to the submission result instead (why???) * As a result we don't create tab-to-search entries for major sites like Walmart and Target * Thought: detect a prevented willSubmitForm followed (synchronously?) by a navigation to something like the form submission URL and trigger the same search engine detection code as if the form were actually submitted See comments 21 onward for more detail.
,
Dec 5 2016
abarth@, perhaps you comment on this? It seems like it touches on areas you know. See comment #29.
,
Dec 9 2016
,
Dec 14 2016
tkent@: you converted this bug from Untriaged to Available. Can you please comment on the feasibility of the proposal in comment #29 (the most recent content-full comment)? I assume you must've read it when flipping the status. Did you decide that it's reasonable / possible?
,
Dec 14 2016
> you converted this bug from Untriaged to Available. I had to do so because we shouldn't leave bugs as Untriaged :) Anyway, My impression: * Detecting prevented form submission + navigation would be too complicated. I don't want to introduce such complexity just for a limited number of sites. * Changing willSubmitForm timing from navigation to just after 'submit' event dispatching may work. It would introduce false detection.
,
Dec 15 2016
@33: I think the prevention logic here may be coming from jQuery, which may mean that "limited number of sites" is inaccurate. Not sure.
,
Feb 22 2017
Just waylaid ojan@ and adamk@ to discuss this issue. From a web platform perspective, Ojan said that in principle it's okay to create custom search engines even if the web site sets preventDefault in willSubmitForm. If the web site wants to prevent custom search engine creation, they should use a different way. He also said changing this behavior--creating search engines in this condition--will require an "intent to ship" message to blink-dev. Ojan said he has ideas on which team should own this bug. (It's clear the omnibox team doesn't have the knowledge to muck around with either the javascript or navigation stacks.) Ojan, can you please revise the components, tags, bug status, etc. as appropriate? In your message, you can probably point out that the short summary of the bug is comment #29. thanks!
,
Feb 22 2017
I think the fixes to this would be in the navigation code. So, Nasko, I think it'd be you're team? +dcheng so he can comment on feasibility of implementation. Regarding a different way to prevent custom search engine creation, ideally we'd give them a simple way to opt-out. FeaturePolicy could probably give us a simple, low-effort way to add a directive here.
,
May 8 2017
ping nasko@ per comment #37.
,
May 8 2017
There's no reason why the callback can't be moved, but someone will need to do due diligence that it doesn't have any unanticipated side effects. Researching this is technically easy: these are just callbacks from Blink to the embedder, so it's "just" a matter of moving https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?rcl=0f727a7e734fbbf7f018cda55a97bb751d8bcdac&l=1467 to be called after https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLFormElement.cpp?rcl=eee2186f0e38677b7fa0e804da2037b07ab7b6c5&l=336. The tricky part of this would making sure this doesn't break extant uses of this callback in the embedder (it looks like it's mainly used for autofill).
,
Aug 1 2017
japhet@, I see your name as someone who touches both these files. Would you be a good person to own this?
,
Nov 2 2017
,
Nov 29
vabr going hobby only -> reducing involvement. Please contact me directly in urgent matters. |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by mpear...@chromium.org
, Sep 27 2016