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

Spec :user-invalid & :user-valid #9047

Merged
merged 6 commits into from
Aug 28, 2023
Merged

Conversation

nt1m
Copy link
Member

@nt1m nt1m commented Mar 17, 2023

Fixes #8452

(See WHATWG Working Mode: Changes for more details.)


/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/semantics-other.html ( diff )

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this! Left a number of inline comments.

This would benefit from review by someone in @whatwg/css, @emilio, and @mfreed7.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks reasonable - just some small comments.

source Outdated
@@ -58845,8 +58849,8 @@ fur

<p>The algorithm to <dfn id="constructing-the-form-data-set" export data-lt="constructing the
entry list" data-x="constructing the entry list">construct the entry list</dfn> given a
<var>form</var>, an optional <var>submitter</var>, and an optional <var>encoding</var>, is as
follows. If not specified otherwise, <var>submitter</var> is null.</p>
<var>form</var>, an optional <var>submitter</var>, an optional <var>resetUserInteracted</var>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does resetUserInteracted need a default?

Actually, more important question: is resetUserInteracted needed? It doesn't appear to be set anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I forgot to use it in the form submission algorithm

source Outdated Show resolved Hide resolved
source Outdated
defined <span>activation behavior</span>, and the user has changed the element's <span
data-x="concept-fe-value">value</span> or its list of <span
data-x="concept-input-type-file-selected">selected files</span> while the control was focused
without committing that change (such that it is different to what it was when the control was
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "committing that change" well defined? E.g. for an <input type=file> I'm not sure where the commit happens - clicking ok on the file picker? Moving focus elsewhere?

I realize that this language was here before - just wondering if it needs clarification.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 31, 2023

Side question: is there a plan to expand the WPT test suite for these pseudos? As I mentioned in web-platform-tests/interop#304, the current testing is quite light.

@nt1m
Copy link
Member Author

nt1m commented Mar 31, 2023

Side question: is there a plan to expand the WPT test suite for these pseudos? As I mentioned in web-platform-tests/interop#304, the current testing is quite light.

There's an initial set of tests getting landed in web-platform-tests/wpt#39293

I'm hoping to write more whenever I have time.

I'm off next week, but here's what planning to do:

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 31, 2023

There's an initial set of tests getting landed in web-platform-tests/wpt#39293

I'm hoping to write more whenever I have time.

I'm off next week, but here's what planning to do:

Awesome, thank you!

@mfreed7
Copy link
Contributor

mfreed7 commented May 25, 2023

I'm hoping to write more whenever I have time.

Hello, just checking in on this issue! Any progress on spec or WPTs?

@nt1m
Copy link
Member Author

nt1m commented May 25, 2023

I'm hoping to write more whenever I have time.

Hello, just checking in on this issue! Any progress on spec or WPTs?

No progress on the spec unfortunately (hoping to get to it soon), but regarding WPT, web-platform-tests/wpt#39293 has since been merged, which should have basic WPT regarding behavior (not just parsing).

@mfreed7
Copy link
Contributor

mfreed7 commented May 30, 2023

I'm hoping to write more whenever I have time.

Hello, just checking in on this issue! Any progress on spec or WPTs?

No progress on the spec unfortunately (hoping to get to it soon), but regarding WPT, web-platform-tests/wpt#39293 has since been merged, which should have basic WPT regarding behavior (not just parsing).

Oh great, thanks for landing that. Feel free to cc me on the spec PR, but we'll get started on at least that basic behavior.

source Outdated Show resolved Hide resolved
@nt1m
Copy link
Member Author

nt1m commented Jun 13, 2023

We received some valuable feedback on the :user-invalid behavior on form submission, I'll be reworking a bit the form submission model.

@mfreed7
Copy link
Contributor

mfreed7 commented Aug 1, 2023

We received some valuable feedback on the :user-invalid behavior on form submission, I'll be reworking a bit the form submission model.

Checking in here. While the WPTs look reasonable for the normal use case behavior, the lack of a landed spec makes me a bit nervous about including this in Interop 2023. I previously closed web-platform-tests/interop#304, but I'm wondering if I should re-open it? And we can punt this item until Interop 2024, perhaps?

@nt1m
Copy link
Member Author

nt1m commented Aug 7, 2023

I just updated the spec PR to address the developer feedback received.

@nt1m
Copy link
Member Author

nt1m commented Aug 8, 2023

I added some form submission & reset WPTs at: web-platform-tests/wpt#41378

@nt1m
Copy link
Member Author

nt1m commented Aug 8, 2023

@mfreed7 @josepharhar @annevk @emilio Can you please take a look at this PR?

@nt1m
Copy link
Member Author

nt1m commented Aug 8, 2023

This PR defines FACE or buttons to be equivalent to :valid / :invalid (although that isn't really what WebKit does atm), but I'm open to feedback for what folks want to do for non-textfield form controls in general.

@nt1m nt1m force-pushed the user-valid-invalid branch 2 times, most recently from 030429b to 864288a Compare August 8, 2023 05:29
webkit-commit-queue pushed a commit to nt1m/WebKit that referenced this pull request Aug 8, 2023
https://bugs.webkit.org/show_bug.cgi?id=257988
rdar://110677832

Reviewed by Aditya Keerthi.

Form submission should trigger user-validation styles, to do this, we set the user-interacted flag to true on form submission (but only initiated from user input).

Form reset should reset form control user-interacted flags.

This matches the WIP spec PR at: whatwg/html#9047

* LayoutTests/imported/w3c/web-platform-tests/css/selectors/user-invalid-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/user-invalid.html:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/user-valid-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/user-valid.html:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/selectors/user-invalid-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/selectors/user-valid-expected.txt:
* Source/WebCore/html/FormAssociatedCustomElement.cpp:
(WebCore::FormAssociatedCustomElement::reset):
* Source/WebCore/html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::submitIfPossible):
* Source/WebCore/html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::reset):
* Source/WebCore/html/HTMLOutputElement.cpp:
(WebCore::HTMLOutputElement::reset):
* Source/WebCore/html/HTMLSelectElement.cpp:
(WebCore::HTMLSelectElement::reset):
* Source/WebCore/html/HTMLTextAreaElement.cpp:
(WebCore::HTMLTextAreaElement::reset):

Canonical link: https://commits.webkit.org/266702@main
@mfreed7
Copy link
Contributor

mfreed7 commented Aug 9, 2023

This PR defines FACE or buttons to be equivalent to :valid / :invalid (although that isn't really what WebKit does atm), but I'm open to feedback for what folks want to do for non-textfield form controls in general.

I see the FACE point, but aren't all buttons barred from constraint validation?

Copy link
Contributor

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks pretty reasonable, modulo a few questions.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add Chromium to the commit message - we're supportive.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
@nt1m
Copy link
Member Author

nt1m commented Aug 21, 2023

I've addressed all the feedback here, can you please take another look?

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a small fixup PR. Please take a look, and in particular at the new reset algorithm for the select element. The previous one was too confusing so I had to clean it up to make sense of what the new step was actually trying to do.

I have one question inline abut something I don't understand.

source Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Aug 24, 2023

I pushed a couple nits. Looks good modulo the category comment above and a follow-up issue for custom elements. I guess for custom elements we want some kind of ElementInternals state?

@nt1m
Copy link
Member Author

nt1m commented Aug 26, 2023

I filed #9639 for FACE

@annevk annevk dismissed domenic’s stale review August 28, 2023 08:25

It's addressed.

@annevk annevk merged commit 834da11 into whatwg:main Aug 28, 2023
1 check passed
@nt1m nt1m deleted the user-valid-invalid branch August 28, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Define :user-valid and :user-invalid pseudo-classes
7 participants