Bug 238356

Summary: [css-cascade] "related-property" logic for (-webkit-)text-orientation is broken
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, joepeck, koivisto, macpherson, menard, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 238350    
Attachments:
Description Flags
testcase
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch ews-feeder: commit-queue-

Description Oriol Brufau 2022-03-24 16:46:36 PDT 
Created attachment 455703 [details]
testcase

The text-orientation and -webkit-text-orientation properties are defined with the "related-property" flag.
They are two different longhands that share a computed value, so this flag tries to address the case where both properties are specified.
Typically, properties are applied in alphabetical order within their priority group, but "related-property" achieves that the last specified one wins:

  <div style="-webkit-text-orientation: mixed; text-orientation: upright">I'm upright</div>
  <div style="text-orientation: mixed; -webkit-text-orientation: upright">I'm upright</div>

However, it doesn't work here (see attached testcase):

  <style>
  div { -webkit-text-orientation: mixed }
  div { text-orientation: upright }
  </style>
  <div>I should be upright</div>

This case works well for (-webkit-)box-shadow, which instead of "related-property" is handled by shouldApplyPropertyInParseOrder().

I thought I could handle (-webkit-)text-orientation in the same way (bug 238350), but when shouldApplyPropertyInParseOrder() returns true, the property is deferred and applied after low-priority properties.
This conflicts with (-webkit-)text-orientation, which need to be high priority.
Comment 1 Oriol Brufau 2022-03-24 17:53:00 PDT 
For revert-layer this implies a problem like bug 238125 (but it would need to be addressed in a different way). This example is not upright:

  @layer { div { writing-mode: vertical-lr; text-orientation: upright; } }
  @layer { div { -webkit-text-orientation: revert-layer; } }

And CSSOM is also broken:

  CSS.supports("text-orientation", "sideways-right"); // false
  document.body.style.webkitTextOrientation = "sideways-right";
  document.body.style.textOrientation; // "sideways-right" !!! invalid value

Since the only difference between these properties is that -webkit-text-orientation accepts sideways-right, but this value always computes to sideways, I would:

 1. Make -webkit-text-orientation an shorthand of text-orientation
 2. Alias sideways-right into sideways at parse time

This is similar to the example in the spec https://drafts.csswg.org/css-cascade/#legacy-shorthand, which says that page-break-before:always expands to break-before:page at parse time.
Comment 2 Oriol Brufau 2022-03-24 19:41:50 PDT 
Created attachment 455718 [details]
Patch
Comment 3 Oriol Brufau 2022-03-25 05:59:00 PDT 
Created attachment 455750 [details]
Patch
Comment 4 Oriol Brufau 2022-03-25 06:00:52 PDT 
Created attachment 455751 [details]
Patch
Comment 5 Oriol Brufau 2022-03-25 06:21:46 PDT 
Created attachment 455754 [details]
Patch
Comment 6 Oriol Brufau 2022-03-25 07:29:03 PDT 
PTAL
Comment 7 Radar WebKit Bug Importer 2022-03-31 16:47:15 PDT 
<rdar://problem/91133741>
Comment 8 Antti Koivisto 2022-04-06 01:49:05 PDT 
Comment on attachment 455754 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5659
> +    else
> +        return false;
> +    if (!keyword || !m_range.atEnd())
> +        return false;

could just remove the else brach, it will hit the !keyword case
Comment 9 Oriol Brufau 2022-04-06 04:29:01 PDT 
Created attachment 456807 [details]
Patch
Comment 10 Oriol Brufau 2022-04-06 04:31:13 PDT 
Comment on attachment 456807 [details]
Patch

(In reply to Antti Koivisto from comment #8)
> Comment on attachment 455754 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455754&action=review
> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:5659
> > +    else
> > +        return false;
> > +    if (!keyword || !m_range.atEnd())
> > +        return false;
> 
> could just remove the else brach, it will hit the !keyword case

Done.
Comment 11 EWS 2022-04-06 07:20:26 PDT 
Committed r292463 (249314@main): <https://commits.webkit.org/249314@main>

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