Bug 136063 – REGRESSION: CSS not() selector does not work when it appears after or within @supports

• Component: CSS
• Status: RESOLVED FIXED

[Koji Ishii]

Created attachment 236804 [details]
Test file for CSS not() selector after @supports

CSS not() selector does not work when it appears after or within @supports.

Just inserting:
  @supports (display:block) {
  }
before a not() selector causes  the not() selector not working.

* Not work on Nightly build on Mac OS X 10.9
* Safari 7.0.6 (9537.78.2) on the same Mac OS X 10.9 is fine
* Chrome, Firefox are fine

[Ryosuke Niwa]

Confirmed it works in the shipping Safari but fails in nightly builds of WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/18061252>

[Benjamin Poulain]

What a weird bug.
Getting the CSS Rules with CSSOM, it looks like the second style rules disappear when @supports is there. All we have is the first style rule, and the support rule.

I suspect something horribly wrong happens when we are parsing the empty @supports rule.

[Yuki Sekiguchi]

Created attachment 236856 [details]
Patch

[Yuki Sekiguchi]

I confirmed that combining the patch in Bug 136062 and this patch fixed http://ikilote.net/fr/Collection/Personnalit%C3%A9.html?id=394

[Yuki Sekiguchi]

Win EWS was failed with:

> error MSB3073: The command "REM Do not edit from the Visual Studio IDE! Customize via a TestWebKitAPIPreBuild.cmd file.

I think this is not related to my patch.

[Darin Adler]

Comment on attachment 236856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236856&action=review

> Source/WebCore/css/CSSParser.cpp:11347
> +        // Since -webkit-supports-condition uses '{', which is CharacterStartRuleGroup, only SupportsMode should be cleared.
> +        if (m_parsingMode == SupportsMode)
> +            m_parsingMode = NormalMode;

Windows is failing to compile because these three lines of code need to be inside #if ENABLE(CSS3_CONDITIONAL_RULES)

> Source/WebCore/css/CSSParser.h:551
> +    bool isSupportsMode() const { return m_parsingMode == SupportsMode || m_parsingMode == SupportsConditionMode; };

Windows is failing to compile because this function needs to be inside #if ENABLE(CSS3_CONDITIONAL_RULES).

The name of this function is not right. We would not say that a “CSS parser is supports mode”, so we don’t want to write the code like that. Perhaps the name isParsingSupports()?

Extra unneeded semicolon on the end of this line.

> LayoutTests/css3/supports-not-selector-expected.html:6
> +<div class="inside">This should be green if not() pseudo class selector works inside @supports rule.</div>
> +<div class="outside">This should be green if not() pseudo class selector works after/outside @supports rule.</div>

I suggest removing the class attributes.

[Darin Adler]

(In reply to comment #6)
> Win EWS was failed with:
> 
> > error MSB3073: The command "REM Do not edit from the Visual Studio IDE! Customize via a TestWebKitAPIPreBuild.cmd file.
> 
> I think this is not related to my patch.

That’s not the error on the bot. The errors are many like these:

     1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\css\CSSParser.h(551): error C2065: 'SupportsMode' : undeclared identifier (..\page\animation\AnimationController.cpp)
     1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\css\CSSParser.h(551): error C2065: 'SupportsConditionMode' : undeclared identifier (..\page\animation\AnimationController.cpp)

Sorry it’s so hard to find the Windows errors. The trick I use is to search for the string ": error".

[Bear Travis]

Comment on attachment 236856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236856&action=review

Thanks for taking this on. Added some small suggestions that I think could help clean up the patch, mainly I think we should follow media query's lead and avoid the addition of the SupportsConditionMode type.

> Source/WebCore/css/CSSParser.cpp:9985
>      CharacterEndMediaQuery,

This seems more like renaming 'CharacterEndMediaQuery' than adding a new CharacterType, so you may want to drop 'CharacterEndMediaQuery'. You might also choose a name like CharacterEndConditionQuery to remain consistent with the old name, but that's just my opinion.

> Source/WebCore/css/CSSParser.cpp:11349
>      case CharacterEndMediaQuery:

I think it's odd to have this leftover case when you'll never actually have 'CharacterEndMediaQuery' returned by the ASCII array values used in this switch.

> Source/WebCore/css/CSSParser.h:558
> +        SupportsConditionMode,

Rather than adding an extra mode and the above isSupports test, would it be possible to use the same approach as media queries, and use a space as the initial delimiter?  See CSSGrammar.y.in's webkit_mediaquery production, and CSSParser.cpp::parseMediaQuery. It's a little hacky, but has precedent and seems cleaner than needing to track SupportsMode and SupportsConditionMode separately.

[Bear Travis]

Comment on attachment 236856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236856&action=review

Thanks for taking this on. Added some small suggestions that I think could help clean up the patch, mainly I think we should follow media query's lead and avoid the addition of the SupportsConditionMode type.

> Source/WebCore/css/CSSParser.cpp:9985
>      CharacterEndMediaQuery,

This seems more like renaming 'CharacterEndMediaQuery' than adding a new CharacterType, so you may want to drop 'CharacterEndMediaQuery'. You might also choose a name like CharacterEndConditionQuery to remain consistent with the old name, but that's just my opinion.

> Source/WebCore/css/CSSParser.cpp:11349
>      case CharacterEndMediaQuery:

I think it's odd to have this leftover case when you'll never actually have 'CharacterEndMediaQuery' returned by the ASCII array values used in this switch.

> Source/WebCore/css/CSSParser.h:558
> +        SupportsConditionMode,

Rather than adding an extra mode and the above isSupports test, would it be possible to use the same approach as media queries, and use a space as the initial delimiter?  See CSSGrammar.y.in's webkit_mediaquery production, and CSSParser.cpp::parseMediaQuery. It's a little hacky, but has precedent and seems cleaner than needing to track SupportsMode and SupportsConditionMode separately.

[Benjamin Poulain]

Comment on attachment 236856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236856&action=review

> LayoutTests/ChangeLog:11
> +        * css3/supports-not-selector-expected.html: Added.
> +        * css3/supports-not-selector.html: Added.

For completeness, it would be good to also have tests for
-empty @support rule.
-the CSSOM model of the 2 cases.

[Yuki Sekiguchi]

Created attachment 236914 [details]
Patch

[Yuki Sekiguchi]

Hi Darin,  Bear,  Benjamin,

Thank you all for reviewing my patch.

I realized that @supports also handle error case that there is @supports followed by ';' like @media. Chrome and Firefox correctly handle "not" selector after the invalid @supports, but my patch didn't.

I rewrote my patch, and @supports mostly work like @media.

(In reply to comment #7)
> > Source/WebCore/css/CSSParser.h:551
> > +    bool isSupportsMode() const { return m_parsingMode == SupportsMode || m_parsingMode == SupportsConditionMode; };
> 
> Windows is failing to compile because this function needs to be inside #if ENABLE(CSS3_CONDITIONAL_RULES).

Sorry. I should be more careful when I write code behind compile flag.

> > LayoutTests/css3/supports-not-selector-expected.html:6
> > +<div class="inside">This should be green if not() pseudo class selector works inside @supports rule.</div>
> > +<div class="outside">This should be green if not() pseudo class selector works after/outside @supports rule.</div>
> 
> I suggest removing the class attributes.

Removed.

(In reply to comment #10)
> > Source/WebCore/css/CSSParser.cpp:9985
> >      CharacterEndMediaQuery,
> 
> This seems more like renaming 'CharacterEndMediaQuery' than adding a new CharacterType, so you may want to drop 'CharacterEndMediaQuery'. You might also choose a name like CharacterEndConditionQuery to remain consistent with the old name, but that's just my opinion.
> 
> > Source/WebCore/css/CSSParser.cpp:11349
> >      case CharacterEndMediaQuery:
> 
> I think it's odd to have this leftover case when you'll never actually have 'CharacterEndMediaQuery' returned by the ASCII array values used in this switch.

I didn't realized ';' error recovery of @supports. Yes, sharing same code with media query is better! Fixed.

> > Source/WebCore/css/CSSParser.h:558
> > +        SupportsConditionMode,
> 
> Rather than adding an extra mode and the above isSupports test, would it be possible to use the same approach as media queries, and use a space as the initial delimiter?  See CSSGrammar.y.in's webkit_mediaquery production, and CSSParser.cpp::parseMediaQuery. It's a little hacky, but has precedent and seems cleaner than needing to track SupportsMode and SupportsConditionMode separately.

I thought it was a little hacky, too. But, fixing this made my patch cleaner. Fixed.

(In reply to comment #11)
> For completeness, it would be good to also have tests for
> -empty @support rule.
> -the CSSOM model of the 2 cases.

Thanks. I can find the bug in ';' error recovery case.
Added error recovery test case and CSSOM test case. Now, my patch works same as Chrome and Firefox.

[Darin Adler]

Comment on attachment 236914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236914&action=review

> Source/WebCore/css/CSSParser.cpp:11352
> +        bool isParsingCondition = m_parsingMode == MediaQueryMode;
> +#if ENABLE(CSS3_CONDITIONAL_RULES)
> +        isParsingCondition = isParsingCondition || m_parsingMode == SupportsMode;
> +#endif
> +        if (isParsingCondition)
>              m_parsingMode = NormalMode;
>          break;

While this is OK, I think two separate if statements would read more simply:

    if (m_parsingMode == MediaQueryMode)
        m_parsingMode = NormalMode;
#if ENABLE(CSS3_CONDITIONAL_RULES)
    if (m_parsingMode == SupportsMode)
        m_parsingMode = NormalMode;
#endif

[WebKit Commit Bot]

Comment on attachment 236914 [details]
Patch

Clearing flags on attachment: 236914

Committed r172833: <http://trac.webkit.org/changeset/172833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Comment on attachment 236914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236914&action=review

LGTM too.

> LayoutTests/ChangeLog:11
> +        * css3/supports-not-selector-cssom-expected.txt: Added.
> +        * css3/supports-not-selector-cssom.html: Added.

Thanks for the new test. It is great.