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

Include <button type=submit> data whilst constructing the form dataset #11333

Merged
merged 2 commits into from May 23, 2016

Conversation

@Manishearth
Copy link
Member

Manishearth commented May 23, 2016

This makes it possible to close things in github (see
Manishearth/mitochondria#1)

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

Either:

  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented May 23, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/htmlbuttonelement.rs
@highfive
Copy link

highfive commented May 23, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth
Copy link
Member Author

Manishearth commented May 23, 2016

r? @nox

@highfive highfive assigned nox and unassigned mbrubeck May 23, 2016
@Manishearth Manishearth mentioned this pull request May 23, 2016
14 of 24 tasks complete
if name == "" {
// unspecced, however all the browsers do it
// and it could break urlencoded parsing if
// there are random equal signs floating around

This comment has been minimized.

Copy link
@Manishearth

Manishearth May 23, 2016

Author Member

I will file a spec bug on this in a while

@Manishearth
Copy link
Member Author

Manishearth commented May 23, 2016

@bors try

@Manishearth Manishearth force-pushed the Manishearth:submit-submit-button branch from 15703f2 to aaa85de May 23, 2016
@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@Manishearth
Copy link
Member Author

Manishearth commented May 23, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2016

Trying commit aaa85de with merge b74cb98...

bors-servo added a commit that referenced this pull request May 23, 2016
Include <button type=submit> data whilst constructing the form dataset

This makes it possible to close things in github (see
Manishearth/mitochondria#1)

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

Either:
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Don't merge yet, I need to handle tests and work out a possible spec bug
regarding no-name form elements

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11333)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2016

@Manishearth Manishearth force-pushed the Manishearth:submit-submit-button branch from aaa85de to 2cd9564 May 23, 2016
@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented May 23, 2016

Thanks for your contribution, just some nits to address.

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

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 3 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


components/script/dom/htmlbuttonelement.rs, line 145 [r1] (raw file):

    /// Steps range from 3.1 to 3.7 (specific to HTMLButtonElement)
    pub fn form_datum(&self, submitter: Option<FormSubmitter>) -> Option<FormDatum> {
        // 3.1: disabled state check is in get_unclean_dataset

Nit: "Step 3.1"


components/script/dom/htmlbuttonelement.rs, line 147 [r1] (raw file):

        // 3.1: disabled state check is in get_unclean_dataset

        // 3.1: only run steps if this is the submitter

Nit: "Step 3.1"


components/script/dom/htmlbuttonelement.rs, line 160 [r1] (raw file):

        let name = self.Name();

        if name == "" {

Nit: name.is_empty().


components/script/dom/htmlbuttonelement.rs, line 167 [r1] (raw file):

        }

        // Step 3.6

AFAICT, this is step 3.9. not 3.6..


components/script/dom/htmlbuttonelement.rs, line 161 [r3] (raw file):

        if name == "" {
            // 3.1: Must have a name

Nit: "Step 3.1"


Comments from Reviewable

@Manishearth Manishearth force-pushed the Manishearth:submit-submit-button branch from 2cd9564 to 102cdaa May 23, 2016
@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented May 23, 2016

@bors-servo r+

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2016

📌 Commit 90d72c5 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2016

Testing commit 90d72c5 with merge de79f96...

bors-servo added a commit that referenced this pull request May 23, 2016
Include <button type=submit> data whilst constructing the form dataset

This makes it possible to close things in github (see
Manishearth/mitochondria#1)

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

Either:
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11333)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2016

@bors-servo bors-servo merged commit 90d72c5 into servo:master May 23, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth Manishearth deleted the Manishearth:submit-submit-button branch May 7, 2019
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

5 participants
You can’t perform that action at this time.