Bug 138310 – Add parsing for :role()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sukolsak Sakshuwong]

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

[Sukolsak Sakshuwong]

Created attachment 240847 [details]
Patch

[Benjamin Poulain]

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.

[Benjamin Poulain]

Any update? I can help if you have questions.

[Sukolsak Sakshuwong]

Created attachment 241664 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 241666 [details]
Test for case sensitivity

[Sukolsak Sakshuwong]

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).

[Darin Adler]

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.

[Benjamin Poulain]

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 :)

[Sukolsak Sakshuwong]

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.

[Sukolsak Sakshuwong]

Created attachment 241754 [details]
Remove doubled changelog

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Comment on attachment 241754 [details]
Remove doubled changelog

Let's do it!

[WebKit Commit Bot]

Comment on attachment 241754 [details]
Remove doubled changelog

Clearing flags on attachment: 241754

Committed r176241: <http://trac.webkit.org/changeset/176241>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.