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

refactor the way of getting controls owned by the form #21495

Open
tigercosmos opened this issue Aug 23, 2018 · 7 comments
Open

refactor the way of getting controls owned by the form #21495

tigercosmos opened this issue Aug 23, 2018 · 7 comments

Comments

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Aug 23, 2018

fn static_validation(&self) -> Result<(), Vec<FormSubmittableElement>> {
let node = self.upcast::<Node>();
// FIXME(#3553): This is an incorrect way of getting controls owned by the
// form, refactor this when html5ever's form owner PR lands

#3553 is closed.
There is a merged PR in #15938.

@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Aug 23, 2018

@jdm
Copy link
Member

@jdm jdm commented Aug 23, 2018

I don't think that code has been updated to take advantage of the changes in #3553 yet.

@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Aug 23, 2018

I think we usually fill the issue number, which is opened for the description, into FIXME(#<number>).

In this case, maybe modifying this comment to avoid misleading is better.
And since the PR is already merged, it could be:

// FIXME: This is an incorrect way of getting controls owned by the 
//        form, should refactor this based on html5ever's form owner(#3553)
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Aug 23, 2018

I think the better solution here is to fix the underlying issue, rather than to fix the wording of the comments. @tigercosmos Do you want to make a PR for the changes?

@tigercosmos
Copy link
Collaborator Author

@tigercosmos tigercosmos commented Aug 24, 2018

I can have a try. I don't look much about this code. @KiChjang Do you remember why you said it is incorrect, and what do you expect it would be like?

@tigercosmos tigercosmos changed the title Shall we delete the comment `fixme(#3553)`? refactor the way of getting controls owned by the form Aug 24, 2018
@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 6, 2020

At least one thing is testably wrong relating to this. html/semantics/forms/the-form-element/form-nameditem.html expects an earlier <input form=a name=b> to be picked up when a <form id=a></form> appearing later in the page is going through its controls, and it doesn't find it. (broke this out into its own issue #25711)

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Feb 4, 2020

There's a FIXME(#3553) that probably really means this issue.

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

Successfully merging a pull request may close this issue.

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