fontFaceRule.style returns a regular MutableStyleProperties, which uses the regular parser mode instead of the @font-face rule descriptor mode
This breaks css/css-syntax/urange-parsing.html
<rdar://problem/56648761>
Created attachment 382021 [details] Patch
Comment on attachment 382021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382021&action=review > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:437 > + auto context = styleSheet->parserContext(); > + if (m_parentRule) { > + switch (m_parentRule->type()) { > + case CSSRule::UNKNOWN_RULE: break; > + case CSSRule::STYLE_RULE: break; > + case CSSRule::CHARSET_RULE: break; > + case CSSRule::IMPORT_RULE: break; > + case CSSRule::MEDIA_RULE: break; > + case CSSRule::FONT_FACE_RULE: > + context.enclosingRuleType = CSSParserContext::RuleContext::FontFace; > + break; > + case CSSRule::PAGE_RULE: break; > + case CSSRule::KEYFRAMES_RULE: break; > + case CSSRule::KEYFRAME_RULE: break; > + case CSSRule::NAMESPACE_RULE: break; > + case CSSRule::SUPPORTS_RULE: break; > +#if ENABLE(CSS_DEVICE_ADAPTATION) > + case CSSRule::WEBKIT_VIEWPORT_RULE: > + context.enclosingRuleType = CSSParserContext::RuleContext::Viewport; > + break; > +#endif > + } > + } Alternatively you could just use StyleRuleBase::Type directly in CSSParserContext instead of adding a new enum. Optional could be used for the default case. This would be less code and avoid bugs if something new starts caring about the enclosing rule type. While not pretty, you could then do the mapping here simply by casting CSSRule::Type -> StyleRuleBase::Type. We rely on it already and there is a compile assert to keep them in sync. > Source/WebCore/css/parser/CSSParserImpl.cpp:101 > StyleRule::Type ruleType = StyleRule::Style; > + > + switch (context.enclosingRuleType) { > + case CSSParserContext::RuleContext::Style: > + break; > + case CSSParserContext::RuleContext::FontFace: > + ruleType = StyleRule::FontFace; > + break; > #if ENABLE(CSS_DEVICE_ADAPTATION) > - if (declaration->cssParserMode() == CSSViewportRuleMode) > + case CSSParserContext::RuleContext::Viewport: > ruleType = StyleRule::Viewport; > + break; > #endif > + } This would read better as a lambda.
(In reply to Antti Koivisto from comment #4) > Comment on attachment 382021 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382021&action=review > > > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:437 > > + auto context = styleSheet->parserContext(); > > + if (m_parentRule) { > > + switch (m_parentRule->type()) { > > + case CSSRule::UNKNOWN_RULE: break; > > + case CSSRule::STYLE_RULE: break; > > + case CSSRule::CHARSET_RULE: break; > > + case CSSRule::IMPORT_RULE: break; > > + case CSSRule::MEDIA_RULE: break; > > + case CSSRule::FONT_FACE_RULE: > > + context.enclosingRuleType = CSSParserContext::RuleContext::FontFace; > > + break; > > + case CSSRule::PAGE_RULE: break; > > + case CSSRule::KEYFRAMES_RULE: break; > > + case CSSRule::KEYFRAME_RULE: break; > > + case CSSRule::NAMESPACE_RULE: break; > > + case CSSRule::SUPPORTS_RULE: break; > > +#if ENABLE(CSS_DEVICE_ADAPTATION) > > + case CSSRule::WEBKIT_VIEWPORT_RULE: > > + context.enclosingRuleType = CSSParserContext::RuleContext::Viewport; > > + break; > > +#endif > > + } > > + } > > Alternatively you could just use StyleRuleBase::Type directly in > CSSParserContext instead of adding a new enum. Optional could be used for > the default case. This would be less code and avoid bugs if something new > starts caring about the enclosing rule type. > > While not pretty, you could then do the mapping here simply by casting > CSSRule::Type -> StyleRuleBase::Type. We rely on it already and there is a > compile assert to keep them in sync. I tried including StyleRule.h in CSSParserContext.h and got all kinds of compile errors because it results in circular includes. I can try again.
Moving the enum to a separate header would help with that (as StyleRuleType).
Created attachment 382028 [details] Patch
Comment on attachment 382028 [details] Patch Attachment 382028 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13183675 New failing tests: fast/text/font-face-javascript.html fast/text/unicode-range-javascript.html
Created attachment 382033 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 382028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382028&action=review > Source/WebCore/ChangeLog:35 > +2019-10-26 Simon Fraser <simon.fraser@apple.com> > + > + Fix nth-child An+B serialization to match the spec > + https://bugs.webkit.org/show_bug.cgi?id=203464 ChangeLog has extra stuff.
Comment on attachment 382028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382028&action=review > Source/WebCore/css/CSSSelector.cpp:393 > +// http://dev.w3.org/csswg/css-syntax/#serializing-anb > +static void outputNthChildAnPlusB(const CSSSelector& selector, StringBuilder& str) > +{ From another patch. > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:440 > + auto context = styleSheet->parserContext(); > + if (m_parentRule) { > + switch (m_parentRule->type()) { > + case CSSRule::UNKNOWN_RULE: > + case CSSRule::STYLE_RULE: > + case CSSRule::CHARSET_RULE: > + case CSSRule::IMPORT_RULE: > + case CSSRule::MEDIA_RULE: > + case CSSRule::PAGE_RULE: > + case CSSRule::KEYFRAMES_RULE: > + case CSSRule::KEYFRAME_RULE: > + case CSSRule::NAMESPACE_RULE: > + case CSSRule::SUPPORTS_RULE: > + break; > + case CSSRule::FONT_FACE_RULE: > + context.enclosingRuleType = StyleRuleType::FontFace; > + break; > +#if ENABLE(CSS_DEVICE_ADAPTATION) > + case CSSRule::WEBKIT_VIEWPORT_RULE: > + context.enclosingRuleType = StyleRuleType::Viewport; > + break; > +#endif > + } > + } > + > + return context; As still think you should simply set enclosingRuleType for all types rather than the two that currently have any effect. You could make it Optional<StyleRuleType> to make it clear most parser entering paths don't set it.
Created attachment 382043 [details] Patch
Comment on attachment 382043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382043&action=review > Source/WebCore/css/parser/CSSParserImpl.cpp:88 > + auto ruleType = context.enclosingRuleType.valueOr(StyleRuleType::Style); I think you can ASSERT(enclosingRuleType) here? > Source/WebCore/css/parser/CSSParserImpl.cpp:200 > + auto ruleType = context.enclosingRuleType.valueOr(StyleRuleType::Style); and here.
https://trac.webkit.org/changeset/251655/webkit