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

Implement form resetting #3981

Closed
Manishearth opened this issue Nov 14, 2014 · 13 comments
Closed

Implement form resetting #3981

Manishearth opened this issue Nov 14, 2014 · 13 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Nov 14, 2014

No description provided.

@jdm
Copy link
Member

@jdm jdm commented Nov 14, 2014

See also the text at https://html.spec.whatwg.org/multipage/forms.html#reset-button-state-%28type=reset%29 for when to initiate a form reset.

@Manishearth Manishearth modified the milestone: Dogfooding Nov 14, 2014
@mttr
Copy link
Contributor

@mttr mttr commented Nov 26, 2014

I'm making some progress on this. I can at least reset text inputs to their original state right now.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Nov 26, 2014

@mttr Could you implement .reset() without implementing <input type=reset>? For the latter you may want to wait until activation (#4002) lands.

Thanks!

@mttr
Copy link
Contributor

@mttr mttr commented Nov 26, 2014

I'll just make sure to leave my input element changes out of any pr I do until then, in that case. I like clicking on my buttons!

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Nov 26, 2014

Heh, sure.

Let me know if you need help! :)

@mttr
Copy link
Contributor

@mttr mttr commented Nov 26, 2014

Continuing from the discussion in #4002 (starting here)

The reset code is going to need some way to determine the initial state as set by the parser.

My understanding so far is that the reset spec allows for the modification of content values (and thus the default values) by javascript (which is why it says to change the checkedness when the checked content attribute is added/removed), so the initial state isn't necessarily what we want.

Actually, from what I've been able to make sense of so far, Servo's behavior in regards to check boxes is wrong (according to the spec). Currently, when we check/uncheck checkboxes with clicks, the checked content attribute gets added/removed (assuming I'm correct that after_set_attr and before_remove_attr are called during content attr changes (I also have a hunch that what we display on the screen is based on the content attrs and not our internal checkedness state)). And since the content attr is our point of reference, this means we have no default to fall back to on for reset.

Here's some relevant spec links:
https://html.spec.whatwg.org/multipage/forms.html#checkbox-state-%28type=checkbox%29
https://html.spec.whatwg.org/multipage/forms.html#concept-input-checked-dirty

I should also point out that while my understanding of this part of the servo code (not to mention the relevant specs) is definitely improving, there's a very good chance that I'm way off base.

@jdm
Copy link
Member

@jdm jdm commented Nov 26, 2014

I would map the spec's "checkedness" to our checked boolean property. As you say, that's modified whenever the attribute changes, whether via input.checked = true, input.removeAttribute('checked'), input.setAttribute('checked', ''), etc. You're also correct that we rely on the presence of the attribute to determine the visual output (http://mxr.mozilla.org/servo/source/resources/servo.css#11)

@mttr
Copy link
Contributor

@mttr mttr commented Nov 26, 2014

So I guess following the spec (display based on our checked property and not the attribute) breaks our current method of seeing a checkbox's state?

If that's the case, maybe I should just do a PR with the text input resets and leave the rest alone for now (still need to figure a couple things out and clean up first, though).

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Nov 27, 2014

Note: I'm pretty sure that that part of the spec is wrong (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27446)

Yeah, a SetFromParser enum would fix it.

-----Original Message-----
From: "Matt Rasmus" notifications@github.com
Sent: ‎11/‎27/‎2014 4:41 AM
To: "servo/servo" servo@noreply.github.com
Cc: "Manish Goregaokar" manishsmail@gmail.com
Subject: Re: [servo] Implement form resetting (#3981)

So I guess following the spec (display based on our checked property and not the attribute) breaks our current method of seeing a checkbox's state?
If that's the case, maybe I should just do a PR with the text input resets and leave the rest alone for now (still need to figure a couple things out and clean up first, though).

Reply to this email directly or view it on GitHub.=

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jan 10, 2015

Fixed in #4133

@jdm
Copy link
Member

@jdm jdm commented Jan 10, 2015

So this can be closed?

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jan 10, 2015

Oops, forgot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.