Closed Bug 547165 Opened 10 years ago Closed 10 years ago

Need tests for form submission

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch to fix (obsolete) — 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)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)
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)
Attached patch Patch v1.1.1Splinter Review
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)
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+
(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 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+
Checked in

http://hg.mozilla.org/mozilla-central/rev/40326c4f714c

Thanks for reviews
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
From testing/mochitest/dynamic/Makefile.in:

# The Initial Developer of the Original Code is
# Netscape Communications Corporation.

Is it really?
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: 583211
Depends on: 607217
Depends on: 695339
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.