Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

form submission for <textarea> and <select> #7919

Merged
merged 1 commit into from Dec 30, 2015
Merged

Conversation

@6112
Copy link
Contributor

6112 commented Oct 7, 2015

Fixes #7849.
Fixes #7850.

Review on Reviewable

@jdm
Copy link
Member

jdm commented Oct 7, 2015

@nox, would you review this?

@nox
Copy link
Member

nox commented Oct 7, 2015

Sure.

@nox nox self-assigned this Oct 7, 2015
@nox
Copy link
Member

nox commented Oct 8, 2015

Thanks for your contribution! See remarks on Reviewable.

-S-awaiting-review +S-needs-code-changes


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


components/script/dom/htmlformelement.rs, line 255 [r1] (raw file):
This is an anti-pattern, do this instead:

if let Some(datum) = input.get_form_datum(submitter) {
    data_set.push(datum);
}

components/script/dom/htmlformelement.rs, line 269 [r1] (raw file):
There should be no space after the negation operator.


components/script/dom/htmlformelement.rs, line 272 [r1] (raw file):
Reuse name here.


components/script/dom/htmlselectelement.rs, line 67 [r1] (raw file):
Here you can instead do:

if let Some(option) = HTMLOptionElementCast::to_ref(&child) {
    ...
}

components/script/dom/htmlselectelement.rs, line 68 [r1] (raw file):
Upcast the option to Node (let node = NodeCast::from_ref(option);) and use the method get_disabled_state() on that.

@jdm Does our IDL getter for the disabled attribute properly accounts for the disabled state in NodeFlags? It doesn't seem so. Should it?


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Oct 8, 2015

Mmm, I knew that Disabled thing sounded familiar: #4765 (comment)

@6112 6112 force-pushed the 6112:master branch from 1dd55f1 to 4b4a932 Oct 8, 2015
@nox
Copy link
Member

nox commented Oct 8, 2015

One last nitpick. Btw @6112, why are there no test expectations changes?

-S-awaiting-review +S-needs-code-changes


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/htmlselectelement.rs, line 68 [r1] (raw file):
Err, silly me, you can just do child.get_enabled_state() here.


Comments from the review on Reviewable.io

@6112
Copy link
Contributor Author

6112 commented Oct 8, 2015

As for the test expectations, I'm not quite sure what you mean. I'm very new here, and mostly clueless about the tests.

@jdm
Copy link
Member

jdm commented Oct 8, 2015

@nox I'm not sure how much automated testing exists for form submission right now; http://mxr.mozilla.org/servo/search?string=.submit doesn't reveal very many likely tests.

@eefriedman
Copy link
Contributor

eefriedman commented Oct 8, 2015

#7923 has a test for this.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

The latest upstream changes (presumably #7873) made this pull request unmergeable. Please resolve the merge conflicts.

@nox
Copy link
Member

nox commented Oct 16, 2015

-S-awaiting-review


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

@nox nox removed the S-awaiting-review label Oct 16, 2015
@6112 6112 force-pushed the 6112:master branch from 5426a1a to dea291e Oct 16, 2015
@nox nox removed the S-needs-rebase label Oct 19, 2015
@6112 6112 force-pushed the 6112:master branch 2 times, most recently from 2320e60 to e42b8ff Dec 9, 2015
@nox
Copy link
Member

nox commented Dec 13, 2015

The file tests/wpt/metadata/html/semantics/forms/form-submission-0/url-encoded.html.ini should be completely removed given there are no remaining failing tests.

-S-awaiting-review +S-needs-code-changes


Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Dec 14, 2015

The latest upstream changes (presumably #8955) made this pull request unmergeable. Please resolve the merge conflicts.

@KiChjang
Copy link
Member

KiChjang commented Dec 29, 2015

@6112 You're almost there! Is there anything that we can do to help?

small changes from code review

!child.get_disabled_state() becomes child.get_enabled_state()
@6112 6112 force-pushed the 6112:master branch from e42b8ff to 1f234af Dec 30, 2015
@6112
Copy link
Contributor Author

6112 commented Dec 30, 2015

Thanks for reminding me, @KiChjang . I'd forgotten about this.

@jdm jdm removed the S-needs-rebase label Dec 30, 2015
@nox
Copy link
Member

nox commented Dec 30, 2015

@bors-servo r+

Perfect, thanks! Sorry didn't notice earlier that you had rebased.


Reviewed 3 of 3 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

📌 Commit 1f234af has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

Testing commit 1f234af with merge 3d969e4...

bors-servo added a commit that referenced this pull request Dec 30, 2015
form submission for <textarea> and <select>

Fixes #7849.
Fixes #7850.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7919)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

💔 Test failed - mac-rel-wpt

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Dec 30, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

The build was interrupted to prioritize another pull request.

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@jdm
Copy link
Member

jdm commented Dec 30, 2015

@bors-servo: retry

  • forced
@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

@bors-servo bors-servo merged commit 1f234af into servo:master Dec 30, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.