Bug 177975 - Can't change @font-face descriptors from fontFaceRule.style.setProperty()
Summary: Can't change @font-face descriptors from fontFaceRule.style.setProperty()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar, WPTImpact
Depends on:
Blocks:
 
Reported: 2017-10-05 15:55 PDT by Myles C. Maxfield
Modified: 2019-10-28 09:04 PDT (History)
16 users (show)

See Also:


Attachments
Patch (17.57 KB, patch)
2019-10-26 22:34 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (687.76 KB, patch)
2019-10-27 11:34 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.61 MB, application/zip)
2019-10-27 13:42 PDT, EWS Watchlist
no flags Details
Patch (79.53 KB, patch)
2019-10-27 20:58 PDT, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-10-05 15:55:41 PDT
fontFaceRule.style returns a regular MutableStyleProperties, which uses the regular parser mode instead of the @font-face rule descriptor mode
Comment 1 Simon Fraser (smfr) 2019-10-26 15:28:44 PDT
This breaks css/css-syntax/urange-parsing.html
Comment 2 Radar WebKit Bug Importer 2019-10-26 15:29:37 PDT
<rdar://problem/56648761>
Comment 3 Simon Fraser (smfr) 2019-10-26 22:34:55 PDT
Created attachment 382021 [details]
Patch
Comment 4 Antti Koivisto 2019-10-27 02:33:21 PDT
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.
Comment 5 Simon Fraser (smfr) 2019-10-27 08:59:31 PDT
(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.
Comment 6 Antti Koivisto 2019-10-27 10:16:45 PDT
Moving the enum to a separate header would help with that (as StyleRuleType).
Comment 7 Simon Fraser (smfr) 2019-10-27 11:34:58 PDT
Created attachment 382028 [details]
Patch
Comment 8 EWS Watchlist 2019-10-27 13:42:01 PDT
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
Comment 9 EWS Watchlist 2019-10-27 13:42:04 PDT
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 10 Antti Koivisto 2019-10-27 14:18:24 PDT
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 11 Antti Koivisto 2019-10-27 14:38:30 PDT
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.
Comment 12 Simon Fraser (smfr) 2019-10-27 20:58:00 PDT
Created attachment 382043 [details]
Patch
Comment 13 Antti Koivisto 2019-10-28 00:21:17 PDT
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.
Comment 14 Simon Fraser (smfr) 2019-10-28 09:04:50 PDT
https://trac.webkit.org/changeset/251655/webkit