Closed
Bug 547165
Opened 15 years ago
Closed 15 years ago
Need tests for form submission
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: sicking)
References
Details
Attachments
(1 file, 2 obsolete files)
|
46.65 KB,
patch
|
mozilla+ben
:
review+
Waldo
:
review+
|
Details | Diff | Splinter Review |
We currently have next to no tests for form submissions. This is obviously bad. Especially since I'm planning on cleaning this code up a bunch.
I wrote a bunch of tests and in the process discovered some cases where we don't follow the IE/HTML5.
Specifically we do submit elements with an empty name attribute, such as
<input type=text name="">. IE does not do this, but rather treats this as
<input type=text>, which neither we nor IE submit.
Also, for an
<input type=image src="..." name="foo" value="bar">
we submit 3 values:
foo.x=<x>
foo.y=<y>
foo=bar
IE and the HTML5 spec says to only submit the first two.
The attached patch fixes these things as well as ads a pile of tests.
Attachment #427719 -
Flags: review?(bnewman)
|
Assignee | |
Comment 1•15 years ago
|
||
This updates the code to get the current filepath so that it works cross platform. I put the generic 'getMyPath.sjs' file in a place where it can be reused by other tests. The location was suggested by waldo.
Attachment #427719 -
Attachment is obsolete: true
Attachment #428616 -
Flags: review?(bnewman)
Attachment #427719 -
Flags: review?(bnewman)
|
|
Assignee | |
Comment 2•15 years ago
|
||
Comment on attachment 428616 [details] [diff] [review]
Patch v1.1
Waldo: Would be great if you could review the changes to testing/mochitest.
I'm not entirely convinced that it makes sense to have a separate static and dynamic directory (in general we use too many directories). Though maybe renaming the existing 'static' directory to something more generic might be better fodder for a separate bug?
Attachment #428616 -
Flags: review?(jwalden+bmo)
|
|
Assignee | |
Comment 3•15 years ago
|
||
Ugh, forgot to hg add the new Makefile.in
Attachment #428616 -
Attachment is obsolete: true
Attachment #428617 -
Flags: review?(bnewman)
Attachment #428616 -
Flags: review?(jwalden+bmo)
Attachment #428616 -
Flags: review?(bnewman)
|
|
Assignee | |
Updated•15 years ago
|
Attachment #428617 -
Flags: review?(jwalden+bmo)
|
||
Comment 4•15 years ago
|
||
Comment on attachment 428617 [details] [diff] [review]
Patch v1.1.1
>- headerEnd = s.indexOf("\r\n\r\n");
>+ let headerEnd = s.indexOf("\r\n\r\n");
> s.substr(2, headerEnd-2).split("\r\n").forEach(function(s) {
Are you sure "\r\n" will be the right line ending regardless of platform?
>+ fileReader1.onload = fileReader2.onload = function() { gen.next(); };
>+ yield;
>+ yield;
I both love and hate the generator idiom you're using here. I think I would love it more and hate it less if you put short in-line comments after at least some of the yields, to indicate which correspond to <body>.onload, which to iframe.onload, which to filereaderN.onload, etc.
r=me with these two points addressed.
Attachment #428617 -
Flags: review?(bnewman) → review+
|
|
Assignee | |
Comment 5•15 years ago
|
||
(In reply to comment #4)
> (From update of attachment 428617 [details] [diff] [review])
> >- headerEnd = s.indexOf("\r\n\r\n");
> >+ let headerEnd = s.indexOf("\r\n\r\n");
> > s.substr(2, headerEnd-2).split("\r\n").forEach(function(s) {
>
> Are you sure "\r\n" will be the right line ending regardless of platform?
Yup. I think this is even defined by http. In any case, servers would never get it right if different user platforms sent different linebreaks.
> >+ fileReader1.onload = fileReader2.onload = function() { gen.next(); };
> >+ yield;
> >+ yield;
>
> I both love and hate the generator idiom you're using here. I think I would
> love it more and hate it less if you put short in-line comments after at least
> some of the yields, to indicate which correspond to <body>.onload, which to
> iframe.onload, which to filereaderN.onload, etc.
Will do.
|
||
Comment 6•15 years ago
|
||
Comment on attachment 428617 [details] [diff] [review]
Patch v1.1.1
getMyPath => getMyDirectory, since it's directory and not including leaf name, and the Mochitest bits I skimmed look good. (I didn't look too closely at the form bits in the actual html of the test itself.)
HTTP sez canonical line-ending is CRLF, servers SHOULD (as I recall) accept CR or LF, but clients definitely have to send CRLF as I recall. A client which sent otherwise probably would work, but it's nog guaranteed. httpd.js, incidentally, only accepts CRLF. Just FWIW.
Attachment #428617 -
Flags: review?(jwalden+bmo) → review+
|
|
Assignee | |
Comment 7•15 years ago
|
||
Checked in
http://hg.mozilla.org/mozilla-central/rev/40326c4f714c
Thanks for reviews
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
|
||
Comment 8•15 years ago
|
||
From testing/mochitest/dynamic/Makefile.in:
# The Initial Developer of the Original Code is
# Netscape Communications Corporation.
Is it really?
|
|
Assignee | |
Comment 9•15 years ago
|
||
It was a mistake, though given that I copied the makefile from elsewhere I guess modifying the original developer might not be ok. I don't really care much.
Depends on: 639378
|
||
Updated•6 years ago
|
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•