Bug 44879

Summary:

Setting form.enctype reflected attribute behaves strangely

Product:

WebKit

Reporter:

Aryeh Gregor <ayg>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

abarth, ap, code.vineet, dglazkov, dominicc, japhet, ojan, tabatkins, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
Proposed Patch	webkit.review.bot: commit-queue-
Updated Patch	ap: review+, ap: commit-queue-
updated patch	none

Aryeh Gregor

Reported 2010-08-30 11:46:17 PDT

Pointed out to me a while ago on IRC by Anne van Kesteren. Test case: <!doctype html> <script> var el = document.createElement("form"); el.enctype = "text"; alert(el.enctype); </script> The spec says this should alert application/x-www-form-urlencoded, since that's the default for form.enctype, and "text" is not a valid value, so the set is ignored. This is what Firefox nightlies do. Opera 10.60 and IE8 alert "text", because they don't limit this to known values (which is wrong per spec but at least is coherent). However, WebKit (Chrome dev and Safari 5) alerts "text/plain", which doesn't match the spec and doesn't make a lot of sense to me. I tried testing in a WebKit nightly, but it crashed.

Attachments
Proposed Patch (5.44 KB, patch) 2011-10-17 06:53 PDT, Vineet Chaudhary (vineetc)	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Updated Patch (6.23 KB, patch) 2011-10-18 03:08 PDT, Vineet Chaudhary (vineetc)	ap: review+ ap: commit-queue-	Details Formatted Diff Diff
updated patch (13.20 KB, patch) 2011-10-21 15:37 PDT, Vineet Chaudhary (vineetc)	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2010-08-31 10:25:15 PDT

> I tried testing in a WebKit nightly, but it crashed. Could you please file a separate bug for that, attaching a crash log <http://webkit.org/quality/crashlogs.html>? I'm not getting a crash with r66356 nightly on Mac OS X.

Aryeh Gregor

Comment 2 2010-08-31 11:53:46 PDT

Filed bug 44968.

Vineet Chaudhary (vineetc)

Comment 3 2011-10-17 06:53:01 PDT

Created attachment 111258 [details] Proposed Patch This is proposed patch. While performing layout test one more issue observed that, firefox doesn't allows the leading white-spaces white specifying attributes value.

WebKit Review Bot

Comment 4 2011-10-17 06:55:16 PDT

Attachment 111258 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource From git://git.webkit.org/WebKit 9b6f4d5..f208d85 master -> origin/master M Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def M Source/JavaScriptCore/ChangeLog r97615 = f208d85eb2fdfaa89e946e8805b5fddf848b866b (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Dominic Cooney

Comment 5 2011-10-17 07:50:30 PDT

Comment on attachment 111258 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111258&action=review > LayoutTests/fast/forms/enctype-attribute.html:36 > debug('Valid values with whitespace:'); You might want to revise this now, it is not accurate. Maybe the comment, too.

Alexey Proskuryakov

Comment 6 2011-10-17 09:16:54 PDT

Comment on attachment 111258 [details] Proposed Patch In bug description, it is mentioned that enctype attribute is not limited to known values in IE. What does it do when actually submitting the form?

WebKit Review Bot

Comment 7 2011-10-17 14:50:40 PDT

Comment on attachment 111258 [details] Proposed Patch Attachment 111258 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10102259 New failing tests: fast/forms/encoding-test.html

Vineet Chaudhary (vineetc)

Comment 8 2011-10-18 03:08:10 PDT

Created attachment 111414 [details] Updated Patch Updated patch.

Vineet Chaudhary (vineetc)

Comment 9 2011-10-18 03:16:37 PDT

(In reply to comment #6) > (From update of attachment 111258 [details]) > In bug description, it is mentioned that enctype attribute is not limited to known values in IE. What does it do when actually submitting the form? I could not get the access to windows machine but checked with Opera linux(as both behaves same). So I tried to get network logs to check encoded url data but irrespective of enctype it was same as in for input "1234 @zxcv" encoded data was "1234+%40zxcv" each time. May be I am checking something wrong here can you please suggest a way to check enctype on submit?

Alexey Proskuryakov

Comment 10 2011-10-18 09:02:50 PDT

Could you attach your test case, and provide more detail about the results you see? You certainly shouldn't be seeing that when doing a POST with multipart/form-data encoding.

Vineet Chaudhary (vineetc)

Comment 11 2011-10-19 00:37:34 PDT

(In reply to comment #10) > Could you attach your test case, and provide more detail about the results you see? You certainly shouldn't be seeing that when doing a POST with multipart/form-data encoding. I tested this behavior with help of http://w3schools.com/tags/tryit.asp?filename=tryhtml_form_enctype this link. 1) In opera browser fetch this link then edit the enctype="text/plain" to enctype="text" and also add method="post" 2) Then pressed "edit and click me" button. Insert "qwerty asdf" in first text box and "123@@" in second or anything.. 3) Before submit open Inspect Element -> Network in opera. 4) Then after submitting the form click form_action.asp in network menu, look for header it says Content-Type:application/x-www-form-urlencoded; Content-Type: will only change to "text/plain" if it specify enctype="text/plain" correctly. Sorry for this annoying procedure.

Alexey Proskuryakov

Comment 12 2011-10-19 12:49:00 PDT

From that data, IE seems to match what the proposed patch does (default to application/x-www-form-urlencoded for any unknown types). The code you're changing looks very curious though - someone must have intentionally added this more permissive logic. Did you check svn blame to see why that was necessary?

Vineet Chaudhary (vineetc)

Comment 13 2011-10-20 00:48:43 PDT

(In reply to comment #12) > From that data, IE seems to match what the proposed patch does (default to application/x-www-form-urlencoded for any unknown types). > > The code you're changing looks very curious though - someone must have intentionally added this more permissive logic. Did you check svn blame to see why that was necessary? Hi Alexey, I tried to follow the changelog for the change but it seems to be very old change (from almost 6 years in html_formimpl.cpp). In my opinion can we change this as its in accordance with spec as well IE, OPERA also behaves same. Please let me your thoughts on this.

Alexey Proskuryakov

Comment 14 2011-10-20 14:01:45 PDT

Sure, I'm not opposing this change, but I'd like to have more information. You said that this change is old. That's not necessarily making it wrong. What was the reason for making this change? Could you post revision number here?

Vineet Chaudhary (vineetc)

Comment 15 2011-10-20 23:17:12 PDT

(In reply to comment #14) > Sure, I'm not opposing this change, but I'd like to have more information. > > You said that this change is old. That's not necessarily making it wrong. What was the reason for making this change? Could you post revision number here? Sorry ap, I didn't mean that. Your comments are always helpful. Here is the more information regarding for those changes. It is present in trunk since this http://trac.webkit.org/browser/trunk/WebCore/khtml/html/html_formimpl.cpp?rev=4#L295 revision. Unfortunately changelog was missing in that commit :(. After that it was copied from one file to another currently in FormSubmission.cpp.

Alexey Proskuryakov

Comment 16 2011-10-21 10:15:29 PDT

Comment on attachment 111414 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111414&action=review Revision 4 is initial KHTML import, and following the history further than that is rarely useful. This change looks good, r=me. > LayoutTests/fast/forms/enctype-attribute.html:27 > +shouldBe('form1.enctype = "text"; form1.enctype', '"application/x-www-form-urlencoded"'); > +shouldBe('form1.enctype = "plain"; form1.enctype', '"application/x-www-form-urlencoded"'); > +shouldBe('form1.enctype = "multipart"; form1.enctype', '"application/x-www-form-urlencoded"'); > +shouldBe('form1.enctype = "form-data"; form1.enctype', '"application/x-www-form-urlencoded"'); Please add subtests for proper MIME types, and verify that they also pass in Firefox. E.g. multipart/mixed, multipart/digest, text/css. Whatever the reason for the existing code was, it was likely not to support "text", but to support other "text/*" subtypes etc. > LayoutTests/fast/forms/enctype-attribute.html:39 > +debug('Webkit should not allow leading whitespace.'); This message is slightly misleading. There is nothing special about WebKit - no major engine trims whitespace when parsing enctype attribute. Besides, it's not only leading, but also trailing whitespace (and the latter should also be tested here).

Vineet Chaudhary (vineetc)

Comment 17 2011-10-21 14:13:12 PDT

(In reply to comment #16) > +shouldBe('form1.enctype = "form-data"; form1.enctype', '"application/x-www-form-urlencoded"'); > > Please add subtests for proper MIME types, and verify that they also pass in Firefox. E.g. multipart/mixed, multipart/digest, text/css. > Hello Alexey, I tried adding different subtypes for multipart and text but as of now none of then supported on firefox so all of them fails. So should we add those subtypes? I refer http://www.iana.org/assignments/media-types/text/index.html for different subtypes. > > LayoutTests/fast/forms/enctype-attribute.html:39 > > +debug('Webkit should not allow leading whitespace.'); > > This message is slightly misleading. There is nothing special about WebKit - no major engine trims whitespace when parsing enctype attribute. Besides, it's not only leading, but also trailing whitespace (and the latter should also be tested here). I will incorporate this.

Alexey Proskuryakov

Comment 18 2011-10-21 14:23:26 PDT

> So should we add those subtypes? Yes, that will make the regression test stronger.

Vineet Chaudhary (vineetc)

Comment 19 2011-10-21 15:37:12 PDT

Created attachment 112034 [details] updated patch Updated patch as per review comments.

WebKit Review Bot

Comment 20 2011-10-21 17:03:25 PDT

Comment on attachment 112034 [details] updated patch Clearing flags on attachment: 112034 Committed r98172: <http://trac.webkit.org/changeset/98172>

WebKit Review Bot

Comment 21 2011-10-21 17:03:31 PDT

All reviewed patches have been landed. Closing bug.

Vineet Chaudhary (vineetc)

Comment 22 2011-10-21 17:07:22 PDT

Thanks Alexey & Darin for landing this.

Note You need to log in before you can comment on or make changes to this bug.