Search engine URL does not match navigation URL on form.submit() |
||||||
Issue description
Chrome Version: 70.0.3538.67
OS: MacOS (and all?)
What steps will reproduce the problem?
(1) Create a search form with
- an input with name "i" and value "v"
- a submit button with name "b" and a clickhandler that do some work then submit the form programmatically
(2) click on the button
(3) Note that you navigate to https://result?i=v
What is the expected result?
A search engine with URL https://result?i=%s should be created
What happens instead?
A search engine with URL https://result?i=%s&b=Submit is created
Example of fake page that will trigger this
<html>
<body>
<form id='test' action='res.php' method="get">
<input name='i'/>
<input type="submit" name="b" id="testb"/>
</form>
<script language="javascript">
document.getElementById("testb").addEventListener("click", function(e) {
e.preventDefault();
setTimeout(function() {document.forms[0].submit()}, 1000);
return false;
});
</script>
</body>
</html>
,
Oct 24
,
Oct 29
ping
,
Oct 29
This isn't really my area. Nonetheless, I'm curious: why this is a problem? The whole create-search-engine stuff is a heuristic, and it's really not smart enough to deal with javascript at all. (I think it needs to work without evaluating Javascript.)
,
Oct 30
I don't think it is a big issue. At the moment, there is native code https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/exported/web_searchable_form_data.cc?g=0&l=71 that adds this button to the URL if no button is activated. The question is why do we do that? The two arguments to remove it are - The purpose of the heuristic is to simulate a form submission, so altering the URL that way goes agains this purpose (and could generate an invalid URL) - We are implementing the feature on iOS and face two different URLs: one generated by the page, and one generated by other platforms when creating a search engine. Do we need to add code to add this extra field in the submission URL?
,
Oct 30
> The question is why do we do that? I don't know. :-) > Do we need to add code to add this extra field in the submission URL? I think the answer is yes. If we ever end up syncing search engines from iOS to other platforms, ideally we wouldn't end up with conflicts for the same keyword. I think mirroring the existing behavior would be useful for that.
,
Oct 31
>> The question is why do we do that? > I don't know. :-) Do you know somebody who could answer this? >> Do we need to add code to add this extra field in the submission URL? > I think the answer is yes. If we ever end up syncing search engines from iOS to other platforms, ideally we wouldn't end up with conflicts for the same keyword. I think mirroring the existing behavior would be useful for that. Or if it is a bug, we could fix it (so it would be useful to know if it is a bug or not). Adding a JavaScript processing that could interfere with the submission in every single form submission to reproduce a bug seems to be low return on investment. Furthermore, if it is a bug and we do not implement it, we are just in advance for when the bug will be fixed on other platforms.
,
Nov 1
> Do you know somebody who could answer this? I'd suggest looking through the blame / change history for the relevant files. Or you could directly try to get attention from Blink>Forms>Search folks. They may or may not "own" the code. Regardless - they probably certainly have opinions on what the right thing to do is. > Or if it is a bug, we could fix it (so it would be > useful to know if it is a bug or not). I think if it's a "bug", we might want to seriously consider not fixing it. If we fix it, I can easily imagine users getting new custom search engines with different URLs. We might be able to be smart and not re-create a new engine, or maybe we can migrate the old URL over. But I'm still worried about sync getting really confused, and we end up with funky (foo.bar.com_ entries that confuse everyone). I think trying to fix this "bug" might be more trouble than it's worth if it's not actually causing any problems. (That's why I asked in comment #4: "why this is a problem?")
,
Nov 1
,
Nov 1
Removing Blink>Forms>Search. This is not an issue of <input type=search> implementation.
,
Nov 1
>> Do you know somebody who could answer this? >I'd suggest looking through the blame / change history for the relevant files. Or you could directly try to get attention from Blink>Forms>Search folks. They may or may not "own" the code. Regardless - they probably certainly have opinions on what the right thing to do is. OK, let's add the component. I guess the triage should somehow route this. Note apparently blink forms search is not the correct components. I will add the components/search_engines owner in cc. >> Or if it is a bug, we could fix it (so it would be >> useful to know if it is a bug or not). >I think if it's a "bug", we might want to seriously consider not fixing it. If we fix it, I can easily imagine users getting new custom search engines with different URLs. We might be able to be smart and not re-create a new engine, or maybe we can migrate the old URL over. But I'm still worried about sync getting really confused, and we end up with funky (foo.bar.com_ entries that confuse everyone). I think trying to fix this "bug" might be more trouble than it's worth if it's not actually causing any problems. (That's why I asked in comment #4: "why this is a problem?") First, I don't know if it is a problem, because I don't know if it is a bug or WAI. It is possible that a search engine rejects our request because they are malformed. I don't think we have metrics to know if the page resulting from an omnibox search contains relevant results. It can also be problem because this capability is used by few users in few forms. Adding a delay or a risk of bad interaction with the page in these conditions should be considered carefully. I really think that sync would be able to resolve a URL conflict (I hope at least). Hidden fields change value all the time and they have not lead to any conflict. The keyword resolution will be a bigger challenge IMO, specially because on Android we display them as URL.
,
Nov 1
The caller has a comment on it say how "we need to do this" so I'm definitely scared of removing it unless we know it causes problems. I tried to trace the provenance of the code. I made it back to 2007: https://chrome-internal.googlesource.com/chrome/chrome-history/+/80245eaf19fed52190cb45f654412ab335bf9f60/webkit/glue/searchable_form_data.cc However, that's not far enough to figure this out. Regarding some of the other questions on this bug, there's no problem in changing the URLs of existing search engine entries. The TemplateURL system will either silently update the old URL (if the user hasn't hand-edited it) or reject the changes entirely, so we shouldn't create dupe entries. We shouldn't worry about how our URLs look from a sync consistency perspective. That said, comment 5 says that other platforms generate a different URL for this engine. Why is that? Aren't they running the same code here? I think if iOS and Windows (for example) parsed the same HTML and created different search engine entries, that's bad on its own. Is that what's happening?
,
Nov 1
K, looks like sky added this in https://chrome-internal.googlesource.com/chrome/chrome-history/+/8e874aa48a2f2a071df8f2f68a4819d0d31e6c15 . Scott, could you comment on the code at https://chrome-internal.googlesource.com/chrome/chrome-history/+/8e874aa48a2f2a071df8f2f68a4819d0d31e6c15/webkit/glue/searchable_form_data.cc#248 ? Do you know why we the URL needs to contain the name of the first active submit button?
,
Nov 1
I believe it's because of section 17.13.2 of the form spec: https://www.w3.org/TR/html401/interact/forms.html#h-17.13 . Specifically the second bullet point: If a form contains more than one submit button, only the activated submit button is successful.
,
Nov 5
I think I will recap, because my comment 5 apparently lead to some confusion. 1. I will describe what I expect of the search engine feature. If I navigate to a web page with a search feature and use the search feature to search "foo", the form submission will trigger a navigation to a URL (we don't consider redirections, just the initial navigation). We will call that the "site search URL". This will also create a search engine accessible by the omnibox. If I search "foo" using this newly created search engine, this will navigate to a URL (again, we don't consider redirection). Let's call this the "generated search URL". In my understanding of the feature, these two URLs should be equals (I am not sure about the username/password part of the URL which could lead to privacy issue, but this is not the problem). Any difference between "site search URL" and "generated search URL" in this example which is the simplest use case you can do for the website should be considered as a bug IMO. 2. If you create a web site with the page described in #0, the two URLs won't match in the query fragment. This is true for any Chrome version (Windows, macOS, Linux and Android). iOS does not currently have a "generated search URL", so it is not affected (yet). 3. #14 note that the point in 17.13.2 only concerns forms with multiple submit button, and you should include the activated submit control. In the current case there is only one submit control, and it is not activated. So this point should not apply (actually it should apply and as you only include activated submit control, it should not be included in that case). A more clear spec is at https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set Point 4.10.21.4 point 3.1.third bullet is clear that a button that is not the submitter should be ignored. Also note that on the very same browser, the site search URL is generated and follows these rules. So if the "generated search URL" is correct, then the "site search URL" we generate is buggy (and make the priority of this bug far higher). 4. I am fully aware that this bug has no consequence in the real world, the same way as adding extra random get parameters in the search URL will probably not break it. This can have some really low consequences like the fact that it reduces the maximum size of the search query, or if a site is really strict and checks all parameters, it can fail, but this is very unlikely. The thing is that we actively add code that will run on a lot of form submission and this code is not adding any functionality other than breaking the spec. I am also fully aware that it is probably less than 1ms per form submission (probably more on iOS because we will need to do this in JS), but as this is only to add a useless field, the return on investment of this ms is still very low. 5. If we decide to keep the current behavior (because there is a good reason, or because we consider that the change is not worth it), we should at least document in the code saying why we do this.
,
Nov 12
ping
,
Nov 13
What you have outlined seems reasonable to me. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mpear...@chromium.org
, Oct 23