28868 – [HTML5][Forms] :valid/:invalid/:optional/:required CSS selectors should be applied lively

RESOLVED FIXED 28868

[HTML5][Forms] :valid/:invalid/:optional/:required CSS selectors should be applied lively

https://bugs.webkit.org/show_bug.cgi?id=28868

Summary [HTML5][Forms] :valid/:invalid/:optional/:required CSS selectors should be ap...

Kent Tamura

Reported 2009-09-01 02:56:13 PDT

:valid, :invalid, :optional and :required CSS selectors should be applied immediately when an element's * value, * constraints, or * willValidate state is changed. In the current WebKit, some of them apply selectors immediately and some of them don't.

Attachments
Proposed patch (22.46 KB, patch) 2009-09-01 03:03 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Proposed patch (rev.2) (22.11 KB, patch) 2009-09-01 03:24 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Proposed patch (rev.3) (21.41 KB, patch) 2009-09-07 20:30 PDT, Kent Tamura	eric: commit-queue-	Details Formatted Diff Diff
Proposed patch (rev.4) (23.33 KB, patch) 2009-09-23 16:39 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Proposed patch (rev.5) (23.12 KB, patch) 2009-09-27 21:53 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Proposed patch (rev.6) (21.96 KB, patch) 2009-10-01 06:42 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Proposed patch (rev.7) (22.15 KB, patch) 2009-10-01 17:42 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2009-09-01 03:03:51 PDT

Created attachment 38853 [details] Proposed patch

Kent Tamura

Comment 2 2009-09-01 03:24:28 PDT

Created attachment 38854 [details] Proposed patch (rev.2)

Eric Seidel (no email)

Comment 3 2009-09-01 07:47:47 PDT

Comment on attachment 38854 [details] Proposed patch (rev.2) I probably would have used a function to make the test output more readable: +PASS document.defaultView.getComputedStyle(el, null).getPropertyValue("background-color") is "rgb(255, 0, 0)" function backgroundOf(el) { return document.defaultView.getComputedStyle(el, null).getPropertyValue("background-color"); } PASS backgroundOf(el) is "rgb(255, 0, 0)" or maybe even: PASS backgroundOf(el) is "red" Slightly sad to see custom edited html files instead of just using .js files and the standard TEMPLATE.html, but these are fine. :) Those comments above are just nits, not requiring any change. I'd need to stare at the c++ part of this more to make a real review decision...

Kent Tamura

Comment 4 2009-09-07 20:30:36 PDT

Created attachment 39168 [details] Proposed patch (rev.3) Follows the Eric's advice. - Separate test HTMLs and JavaScript code - Visible text changes in the tests

Kent Tamura

Comment 5 2009-09-21 09:28:27 PDT

Note that we don"t need to update the patch rev.3 for the resoruces -> script-tests change because the tests uses not only .js but also .css.

Eric Seidel (no email)

Comment 6 2009-09-23 10:45:27 PDT

Comment on attachment 39168 [details] Proposed patch (rev.3) Looks fine to me. Sad this has been sitting in the queue for 2 weeks w/o comment. :( 50 if (textArea->willValidate()) 51 // Need to sync the value in order to apply :valid/:invalid selectors. 52 textArea->updateValue(); Needs {} since the comment makes it multi-line. There are 2 instances of that. If updateValue() isn't const anymore, why is this const_cast needed? 297 const_cast<HTMLTextAreaElement*>(this)->updateValue(); I'm not sure I'm the best person to review this change, but the others have been silent. It looks OK to me and looks non-harmful. I'll r+ it, but it can't be auto-committed as is, so you'll have to commit this yourself, find someone to commit it, or post a new patch with modifications for the commit-queue.

Kent Tamura

Comment 7 2009-09-23 16:39:55 PDT

Created attachment 40029 [details] Proposed patch (rev.4) > Needs {} since the comment makes it multi-line. There are 2 instances of that. Fixed 2 instances, and removed {} for single statement at another place. > If updateValue() isn't const anymore, why is this const_cast needed? > 297 const_cast<HTMLTextAreaElement*>(this)->updateValue(); In order to nuke "const" of "this". This method is const and updateValue() is not const.

Kent Tamura

Comment 8 2009-09-27 21:53:22 PDT

Created attachment 40215 [details] Proposed patch (rev.5) Resolved a patch conflict with today's WebKit source.

Kent Tamura

Comment 9 2009-10-01 06:42:21 PDT

Created attachment 40439 [details] Proposed patch (rev.6) - Resolved a conflict with r48959. - Added more explanation to ChangeLog.

Kent Tamura

Comment 10 2009-10-01 17:42:52 PDT

Created attachment 40487 [details] Proposed patch (rev.7) Oops, ChangeLog in the previous patch was old.

Eric Seidel (no email)

Comment 11 2009-10-05 10:59:34 PDT

Comment on attachment 39168 [details] Proposed patch (rev.3) Clearing r+ on this obsolete patch.

Eric Seidel (no email)

Comment 12 2009-10-05 11:00:59 PDT

Comment on attachment 40487 [details] Proposed patch (rev.7) Looks OK.

WebKit Commit Bot

Comment 13 2009-10-05 11:23:31 PDT

Comment on attachment 40487 [details] Proposed patch (rev.7) Clearing flags on attachment: 40487 Committed r49105: <http://trac.webkit.org/changeset/49105>

WebKit Commit Bot

Comment 14 2009-10-05 11:23:35 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.