See https://bugs.webkit.org/show_bug.cgi?id=138196
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.