Bug 28868

Summary: [HTML5][Forms] :valid/:invalid/:optional/:required CSS selectors should be applied lively
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, commit-queue, darin, hyatt, michelangelo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#matching-html-elements-using-selectors
Bug Depends on:    
Bug Blocks: 19264, 28649, 29071    
Attachments:
Description Flags
Proposed patch
none
Proposed patch (rev.2)
none
Proposed patch (rev.3)
eric: commit-queue-
Proposed patch (rev.4)
none
Proposed patch (rev.5)
none
Proposed patch (rev.6)
none
Proposed patch (rev.7) none

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
DetailsFormatted DiffDiff
Proposed patch (rev.2) (22.11 KB, patch)
2009-09-01 03:24 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Proposed patch (rev.3) (21.41 KB, patch)
2009-09-07 20:30 PDT, Kent Tamura 		eric: commit-queue-
DetailsFormatted DiffDiff
Proposed patch (rev.4) (23.33 KB, patch)
2009-09-23 16:39 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Proposed patch (rev.5) (23.12 KB, patch)
2009-09-27 21:53 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Proposed patch (rev.6) (21.96 KB, patch)
2009-10-01 06:42 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Proposed patch (rev.7) (22.15 KB, patch)
2009-10-01 17:42 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
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.