| Summary: | REGRESSION: CSS not() selector does not work when it appears after or within @supports | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Koji Ishii <kojii> | ||||||||
| Component: | CSS | Assignee: | Benjamin Poulain <benjamin> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, benjamin, betravis, commit-queue, darin, kling, rniwa, webkit-bug-importer, yuki.sekiguchi | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Mac (Intel) | ||||||||||
| OS: | OS X 10.9 | ||||||||||
| Attachments: |
|
||||||||||
Confirmed it works in the shipping Safari but fails in nightly builds of WebKit. 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. Created attachment 236856 [details]
Patch
I confirmed that combining the patch in Bug 136062 and this patch fixed http://ikilote.net/fr/Collection/Personnalit%C3%A9.html?id=394 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 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. (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 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 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 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. Created attachment 236914 [details]
Patch
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 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 on attachment 236914 [details] Patch Clearing flags on attachment: 236914 Committed r172833: <http://trac.webkit.org/changeset/172833> All reviewed patches have been landed. Closing bug. 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. |
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