Bug 177975 – Can't change @font-face descriptors from fontFaceRule.style.setProperty()

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

fontFaceRule.style returns a regular MutableStyleProperties, which uses the regular parser mode instead of the @font-face rule descriptor mode

[Simon Fraser (smfr)]

This breaks css/css-syntax/urange-parsing.html

[Radar WebKit Bug Importer]

<rdar://problem/56648761>

[Simon Fraser (smfr)]

Created attachment 382021 [details]
Patch

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

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

[Antti Koivisto]

Moving the enum to a separate header would help with that (as StyleRuleType).

[Simon Fraser (smfr)]

Created attachment 382028 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antti Koivisto]

Comment on attachment 382028 [details]
Patch

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

> Source/WebCore/ChangeLog:35
> [phone]  Simon Fraser  <[e-mail]>
> +
> +        Fix nth-child An+B serialization to match the spec
> +        https://bugs.webkit.org/show_bug.cgi?id=203464

ChangeLog has extra stuff.

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

Created attachment 382043 [details]
Patch

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/251655/webkit