Bug 239989

Summary: [cssom] -webkit-text-combine:none serializes as empty string in computed style
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, ntim, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Oriol Brufau 2022-05-02 16:20:23 PDT 
```js
var el = document.documentElement;

el.style.webkitTextCombine = "horizontal";
el.style.webkitTextCombine; // "horizontal"
getComputedStyle(el).webkitTextCombine; // "horizontal"

el.style.webkitTextCombine = "none";
el.style.webkitTextCombine; // "none"
getComputedStyle(el).webkitTextCombine; // ""
```

The last line should produce "none".
Comment 1 Radar WebKit Bug Importer 2022-05-09 16:21:12 PDT 
<rdar://problem/92992438>
Comment 2 Oriol Brufau 2022-05-10 06:11:01 PDT 
Created attachment 459115 [details]
Patch
Comment 3 Tim Nguyen (:ntim) 2022-05-10 12:03:54 PDT 
Comment on attachment 459115 [details]
Patch

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

I originally intentionally changed -webkit-text-combine to be hidden from the computed style when implementing text-combine-upright since it is non-standard.

But on second thought, we should probably just handle this like any alias.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3972
> +            return cssValuePool.createValue(style.textCombine());

I wonder what we should do if we ever add the digits value support to text-combine-upright. How would that reflect in -webkit-text-combine?
Comment 4 Oriol Brufau 2022-05-10 12:15:42 PDT 
(In reply to Tim Nguyen (:ntim) from comment #3)
> Comment on attachment 459115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459115&action=review
> 
> I originally intentionally changed -webkit-text-combine to be hidden from
> the computed style when implementing text-combine-upright since it is
> non-standard.

But it's still indexed, so it's weird. Another alternative could be not indexing it, though.

> But on second thought, we should probably just handle this like any alias.

Aliases have the same grammar, which is not the case here and changing it might have a compat risk.
I think the proper way would be as a legacy shorthand: https://drafts.csswg.org/css-cascade-4/#legacy-shorthand

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3972
> > +            return cssValuePool.createValue(style.textCombine());
> 
> I wonder what we should do if we ever add the digits value support to
> text-combine-upright. How would that reflect in -webkit-text-combine?

I guess it depends on whether the grammar -webkit-text-combine is also expanded to accept that or not.
If -webkit-text-combine accepts digits, then it should serialize normally. If it doesn't, then empty string.
Comment 5 Tim Nguyen (:ntim) 2022-05-10 23:24:16 PDT 
(In reply to Oriol Brufau from comment #4)
> (In reply to Tim Nguyen (:ntim) from comment #3)
> > Comment on attachment 459115 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=459115&action=review
> > 
> > I originally intentionally changed -webkit-text-combine to be hidden from
> > the computed style when implementing text-combine-upright since it is
> > non-standard.
> 
> But it's still indexed, so it's weird. Another alternative could be not
> indexing it, though.
> 
> > But on second thought, we should probably just handle this like any alias.
> 
> Aliases have the same grammar, which is not the case here and changing it
> might have a compat risk.
> I think the proper way would be as a legacy shorthand:
> https://drafts.csswg.org/css-cascade-4/#legacy-shorthand

Legacy shorthand sounds good.

> > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3972
> > > +            return cssValuePool.createValue(style.textCombine());
> > 
> > I wonder what we should do if we ever add the digits value support to
> > text-combine-upright. How would that reflect in -webkit-text-combine?
> 
> I guess it depends on whether the grammar -webkit-text-combine is also
> expanded to accept that or not.
> If -webkit-text-combine accepts digits, then it should serialize normally.
> If it doesn't, then empty string.

-webkit-text-combine should not accept any new syntax, so probably empty string makes sense or not indexing. Anyway, no need to think about this now, but worth leaving a note.
Comment 6 Oriol Brufau 2022-05-12 06:24:39 PDT 
(In reply to Tim Nguyen (:ntim) from comment #5)
> Legacy shorthand sounds good.

Filed bug 240341.
Comment 7 EWS 2022-05-12 06:50:19 PDT 
Committed r294101 (250486@main): <https://commits.webkit.org/250486@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459115 [details].