Summary: | Add parsing for :role() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sukolsak Sakshuwong <sukolsak> | ||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, commit-queue, dino, jcraig, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 138196 | ||||||||||||
Attachments: |
|
Description
Sukolsak Sakshuwong
2014-11-03 06:16:34 PST
Created attachment 240847 [details]
Patch
Comment on attachment 240847 [details]
Patch
First round or review. Sorry for the delay, I have a huge backlog on my review queue :(
The patch looks good but:
-The new selector should be under ENABLE(CSS_SELECTORS_LEVEL4). Sorry I did not mention that by mail, :role() is part of the new selectors we are adding for Level 4 :(
-You should also include test for things that can go wrong, invalid input. For example, :role(), :role()), :role(25), :role(a, b), etc. We need to ensure we do not generate valid CSSSelector for invalid input.
-You should include tests covering uppercase and lowercase. We still need to define how case-sensitivity will work with :role(), in the meantime we should make sure the behavior is tested so that some tests would break when the behavior changes.
-I have recently added the test fast/selectors/invalid-functional-pseudo-class.html for functional pseudo class. Can you add :role() there too and extend the results?
I am a bit surprised you did not need to modify the parser.
Any update? I can help if you have questions. Created attachment 241664 [details]
Patch
Created attachment 241666 [details]
Test for case sensitivity
Sorry for the delay. I was busy with school work. Thank you very much for the review. (In reply to comment #2) > -The new selector should be under ENABLE(CSS_SELECTORS_LEVEL4). Sorry I did > not mention that by mail, :role() is part of the new selectors we are adding > for Level 4 :( Done. > -You should also include test for things that can go wrong, invalid input. > For example, :role(), :role()), :role(25), :role(a, b), etc. We need to > ensure we do not generate valid CSSSelector for invalid input. Done. > -You should include tests covering uppercase and lowercase. We still need to > define how case-sensitivity will work with :role(), in the meantime we > should make sure the behavior is tested so that some tests would break when > the behavior changes. Done? > -I have recently added the test > fast/selectors/invalid-functional-pseudo-class.html for functional pseudo > class. Can you add :role() there too and extend the results? Done. > I am a bit surprised you did not need to modify the parser. That seemed to be wrong. The first patch generated valid CSSSelector for invalid input such as :role(42). Comment on attachment 241666 [details] Test for case sensitivity View in context: https://bugs.webkit.org/attachment.cgi?id=241666&action=review > LayoutTests/ChangeLog:18 > + Add parsing for :role() > + https://bugs.webkit.org/show_bug.cgi?id=138310 Doubled change log here. Comment on attachment 241666 [details] Test for case sensitivity View in context: https://bugs.webkit.org/attachment.cgi?id=241666&action=review Looks great. Some minor comments. The tests look good, you'll just need to clean up the changelog. > Source/WebCore/css/CSSGrammar.y.in:67 > + case ROLEFUNCTION: I don't think you need to define a new token type. The type "FUNCTION should work. > Source/WebCore/css/CSSParser.cpp:10703 > + m_token = ROLEFUNCTION; You still need this, but I think the token type could be simply FUNCTION. > Source/WebCore/css/CSSSelector.h:162 > + PseudoClassRole, Uh, I should really sort this enum some day :) Thank you for the review. (In reply to comment #7) > > LayoutTests/ChangeLog:18 > > + Add parsing for :role() > > + https://bugs.webkit.org/show_bug.cgi?id=138310 > > Doubled change log here. I will fix this. (In reply to comment #8) > > Source/WebCore/css/CSSGrammar.y.in:67 > > + case ROLEFUNCTION: > > I don't think you need to define a new token type. The type "FUNCTION > should work. > > > Source/WebCore/css/CSSParser.cpp:10703 > > + m_token = ROLEFUNCTION; > > You still need this, but I think the token type could be simply FUNCTION. If I use "FUNCTION", :role(42) will be valid because there is "| ':' FUNCTION maybe_space maybe_unary_operator INTEGER maybe_space ')' {" in the grammar. Created attachment 241754 [details]
Remove doubled changelog
(In reply to comment #9) > > I don't think you need to define a new token type. The type "FUNCTION > > should work. > > > > > Source/WebCore/css/CSSParser.cpp:10703 > > > + m_token = ROLEFUNCTION; > > > > You still need this, but I think the token type could be simply FUNCTION. > > If I use "FUNCTION", :role(42) will be valid because there is > > "| ':' FUNCTION maybe_space maybe_unary_operator INTEGER maybe_space ')' {" > > in the grammar. Oh, of course. I was wondering how :lang() got away with it...but Dhi recently changed lang to have its own token type. Comment on attachment 241754 [details]
Remove doubled changelog
Let's do it!
Comment on attachment 241754 [details] Remove doubled changelog Clearing flags on attachment: 241754 Committed r176241: <http://trac.webkit.org/changeset/176241> All reviewed patches have been landed. Closing bug. |