Bug 138769 - Implement :valid and :invalid matching for the fieldset element
Summary: Implement :valid and :invalid matching for the fieldset element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-15 18:03 PST by Benjamin Poulain
Modified: 2014-11-16 23:01 PST (History)
1 user (show)

See Also:


Attachments
Patch (352.64 KB, patch)
2014-11-15 18:37 PST, Benjamin Poulain
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-11-15 18:03:12 PST
Implement :valid and :invalid matching for the fieldset element
Comment 1 Benjamin Poulain 2014-11-15 18:37:02 PST
Created attachment 241669 [details]
Patch
Comment 2 Darin Adler 2014-11-16 20:08:53 PST
Comment on attachment 241669 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        a fielset element based on its descendants:

Typo: fieldset

> Source/WebCore/ChangeLog:35
> +        With the validity state fully encapsulated in HTMLForlControlElement, all I need

Typo: HTMLFormControlElement.

> Source/WebCore/html/HTMLFormControlElement.cpp:407
> +static void addInvalidElementToAncestors(HTMLFormControlElement& element)

Seems a shame to not share code with addInvalidElementToAncestorFromInsertionPoint. Can we refactor so they don’t have to be entirely separate? It’s annoying to have four separate functions, not even all that close to each other in the file, all knowing the special rule about HTMLFieldSetElement. Would be nicer to have just two.

> Source/WebCore/html/HTMLFormControlElement.cpp:473
>  void HTMLFormControlElement::setNeedsValidityCheck()

The name of this function is not good. It should be called updateValidity, since it does this, synchronously. It doesn’t just record that there’s a need for a validity check later!

> Source/WebCore/html/HTMLFormControlElement.h:202
> +inline bool HTMLFormControlElement::isValidFormControlElement() const
> +{
> +    // If the following assertion fails, setNeedsValidityCheck() is not called
> +    // correctly when something which changes validity is updated.
> +    ASSERT(m_isValid == valid());
> +    return m_isValid;
> +}

This doesn’t need to be in the header file. Lets move it to the .cpp file.

> Source/WebCore/html/HTMLKeygenElement.h:40
> +    virtual bool recalcWillValidate() const override { return false; }

Not a great name for this function. Elsewhere we use “compute” instead of “recalc” for function names like this one.
Comment 3 Benjamin Poulain 2014-11-16 23:01:14 PST
Committed r176174: <http://trac.webkit.org/changeset/176174>