Bug 139850 – Form elements should match :valid and :invalid based on their associated elements

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Form elements should match :valid and :invalid based on their associated elements

[Benjamin Poulain]

Created attachment 243604 [details]
Patch

[Darin Adler]

Comment on attachment 243604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243604&action=review

> Source/WebCore/ChangeLog:18
> +        To work around the lifetime problem, the code of HTMLFormControlElement::didChangeForm()
> +        uses m_willValidateInitialized and m_willValidate direclty instead
> +        of invoking willValidate(). That way we don't try to validate an incomplete object.

We might be able to side step all or part of this mess if we construct FormAssociatedElement objects with a non-null form, without calling willChangeForm and didChangeForm inside the contructor. There might some necessary work that didChangeForm does, but it’s possible we can accomplish that another way. It’s already kind of bad that the initial didChangeForm call can happen when the form elements isn’t completely constructed yet.

> Source/WebCore/html/HTMLFormControlElement.cpp:70
> +    setForm(nullptr);

This line of code is really subtle. People will be tempted to remove it. You need to add a comment about why it’s needed so they don’t just delete it and then get surprised when a test fails.

> Source/WebCore/html/HTMLFormControlElement.cpp:482
> +    if (form() && m_willValidateInitialized && m_willValidate && !isValidFormControlElement())
> +        form()->registerInvalidAssociatedFormControl(*this);

Why didn’t you put form() into a local variable here, given you did that everywhere else? Just so it could all be in a single if condition?

[Benjamin Poulain]

(In reply to comment #2)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243604&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        To work around the lifetime problem, the code of HTMLFormControlElement::didChangeForm()
> > +        uses m_willValidateInitialized and m_willValidate direclty instead
> > +        of invoking willValidate(). That way we don't try to validate an incomplete object.
> 
> We might be able to side step all or part of this mess if we construct
> FormAssociatedElement objects with a non-null form, without calling
> willChangeForm and didChangeForm inside the contructor. There might some
> necessary work that didChangeForm does, but it’s possible we can accomplish
> that another way. It’s already kind of bad that the initial didChangeForm
> call can happen when the form elements isn’t completely constructed yet.

I did try that but when setting the form on FormAssociatedElement we must have a valid HTMLElement.

I also tried moving the setForm() after the constructor to have the real object, but HTMLInputElement explicitly rely on not getting willChangeForm()/didChangeForm() in some cases.

The whole thing is very convoluted. We should solve it at some point but I could not find an easy fix with this patch.

[Benjamin Poulain]

Committed r177664: <http://trac.webkit.org/changeset/177664>