Bug 136063 - REGRESSION: CSS not() selector does not work when it appears after or within @supports
Summary: REGRESSION: CSS not() selector does not work when it appears after or within ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.9
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-08-19 00:32 PDT by Koji Ishii
Modified: 2014-08-21 14:22 PDT (History)
9 users (show)

See Also:


Attachments
Test file for CSS not() selector after @supports (334 bytes, text/html)
2014-08-19 00:32 PDT, Koji Ishii
no flags Details
Patch (7.82 KB, patch)
2014-08-19 23:10 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (14.02 KB, patch)
2014-08-20 22:18 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Koji Ishii 2014-08-19 00:32:21 PDT
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
Comment 1 Ryosuke Niwa 2014-08-19 04:49:33 PDT
Confirmed it works in the shipping Safari but fails in nightly builds of WebKit.
Comment 2 Radar WebKit Bug Importer 2014-08-19 04:49:45 PDT
<rdar://problem/18061252>
Comment 3 Benjamin Poulain 2014-08-19 23:09:00 PDT
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.
Comment 4 Yuki Sekiguchi 2014-08-19 23:10:21 PDT
Created attachment 236856 [details]
Patch
Comment 5 Yuki Sekiguchi 2014-08-19 23:17:49 PDT
I confirmed that combining the patch in Bug 136062 and this patch fixed http://ikilote.net/fr/Collection/Personnalit%C3%A9.html?id=394
Comment 6 Yuki Sekiguchi 2014-08-20 00:46:21 PDT
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.
Comment 7 Darin Adler 2014-08-20 09:06:14 PDT
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.
Comment 8 Darin Adler 2014-08-20 09:11:46 PDT
(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".
Comment 9 Bear Travis 2014-08-20 11:16:33 PDT
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.
Comment 10 Bear Travis 2014-08-20 11:16:33 PDT
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.
Comment 11 Benjamin Poulain 2014-08-20 13:23:14 PDT
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.
Comment 12 Yuki Sekiguchi 2014-08-20 22:18:35 PDT
Created attachment 236914 [details]
Patch
Comment 13 Yuki Sekiguchi 2014-08-21 04:33:09 PDT
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.
Comment 14 Darin Adler 2014-08-21 12:57:31 PDT
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
Comment 15 WebKit Commit Bot 2014-08-21 13:29:59 PDT
Comment on attachment 236914 [details]
Patch

Clearing flags on attachment: 236914

Committed r172833: <http://trac.webkit.org/changeset/172833>
Comment 16 WebKit Commit Bot 2014-08-21 13:30:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Benjamin Poulain 2014-08-21 14:22:42 PDT
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.