Bug 196139 – Unprefix -webkit-text-orientation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

WebKit has "-webkit-text-oritation" propperty, but that's somehow outdated compared to the "text-orientation" property from css-writing-modes spec: https://drafts.csswg.org/css-writing-modes/#text-orientation
"text-orientation" is already supported in Chromium and Firefox.
It would be nice to unprefix it and align with the support in other browsers.

One example of usage is related to beaseline in vertical writing modes by default we should use the central baseline, and only use the alphabetic baseline if "text-orientation: sideways".

Right now in WebKit it uses the alphabetic baseline independtly of "text-orientation" porperty (as it's not supported yet).
So the in the following example, both cases look the same.

  <div style="writing-mode: vertical-lr;">
    before
    <div style="display: inline-block; font-size: 50px; width: 200px; background: lime;">baseline</div>
    after
  </div>
  <hr>
  <div style="writing-mode: vertical-lr; text-orientation: sideways;">
    before
    <div style="display: inline-block; font-size: 50px; width: 200px; background: lime;">baseline</div>
    after
  </div>

Even the default case is wrong as it should use the central baseline. This is part of bug #94410.

Regarding the baseline issue the spec text is very clear (https://drafts.csswg.org/css-writing-modes-4/#text-baselines):
"In vertical typographic mode, the central baseline is used as the dominant baseline when text-orientation is mixed or upright. Otherwise the alphabetic baseline is used."
However there are some discussion about what's the central baseline exactly as Chromium and Firefox behave different right now (see https://crbug.com/942734).

[Myles C. Maxfield]

In addition to the problem described above, we should find any other problems remaining in WPT before we unprefix. The best way of doing this is probably to unprefix and run the suite, and see which tests fail.

Given that we support two non-standard values, I'd recommend that we don't make a pure alias, but instead make a new property which only accepts the standard values, and sets the same field in RenderStyle.

[Myles C. Maxfield]

s/probably to unprefix and run the suite/probably to write the code to unprefix, not land it, and run the suite/

[Radar WebKit Bug Importer]

<rdar://problem/56623427>

Created attachment 391816 [details]
Test file that demonstrates text and block baseline

[Koji Ishii]

FYI, the Blink planning doc:
https://docs.google.com/document/d/1lGrcTSlKMDeOEKZbHqvdLnW_Soywn7oICci2ApBXB00/edit?usp=sharing

Created attachment 393089 [details]
Patch

[Jon Lee]

Comment on attachment 393089 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Frank Yang  <[e-mail]>

Please explain what your patch does in the Changelog. I suggest a brief paragraph that says overall what's going on, and then for each file what's being accomplished and/or why.

Created attachment 393104 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 393104 [details]
Patch

r- for red EWS bubbles

Created attachment 394194 [details]
Patch

Created attachment 394203 [details]
Patch

Created attachment 394216 [details]
Patch

Created attachment 394219 [details]
Patch

Created attachment 394220 [details]
Patch

Created attachment 394225 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 394225 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        In order to unprefix -webkit-text-orientation to be text-orientation, a new property, "text-orientation"

This should explain why using "aliases" was not acceptable. (The answer is that the prefixed property accepts non-standard values, and the unprefixed property shouldn't.)

> Source/WebCore/ChangeLog:10
> +        is a high-priority property, and without extra logic, the CSS property ordering rule cannot be enforced because 

Which ordering rule? I think you mean the "last rule wins" ordering rule.

> Source/WebCore/ChangeLog:14
> +        each other, so that when applying high priorities, the algorithm treats both -webkit-text-orientation and text-orientation
> +        as the same property, and thus the CSS inline property ordering rule will take effect. 

I don't think "treats them as the same property" is really what you mean.

"CSS inline" is a term-of-art, and I don't think you mean to invoke it here.

> Source/WebCore/ChangeLog:29
> +        Changes made in:
> +                CSSComputedStyleDeclaration.cpp
> +                CSSParserFastPaths.cpp
> +                StyleBuilderCustom.h
> +        are for parsing the newly added "text-orientation" property
> +
> +        Changes made in:
> +                CSSProperties.json
> +                makeprop.pl
> +                CSSParserImpl.cpp
> +        are for implementing the logic to treat the prefixed and unprefixed CSS properties as the
> +        same property so that the CSS inline property order rule will hold

These comments are better put on the bulleted lines below (lines 41-53). The colons indicate than file-specific descriptions can be made there.

> Source/WebCore/ChangeLog:32
> +        Tests: fast/text/orientation-inheritance.html

Do any Web Platform Tests need to be updated? Or any TestExpectations?

> Source/WebCore/WebCore.xcodeproj/xcshareddata/xcschemes/WebCore.xcscheme:47
> +      <PathRunnable
> +         runnableDebuggingMode = "0"
> +         BundleIdentifier = "org.webkit.MiniBrowser"
> +         FilePath = "/Users/framiere/projects/build/Debug/MiniBrowser.app">
> +      </PathRunnable>

This should not be part of the patch.

> Source/WebCore/css/CSSProperties.json:120
> +        "Indicates the prefixed or unprefixed version of the same property",

I think this comment should describe what the behavior does, not what it happens to be used for in this patch. A human-readable description of what "related-property" actually does (aka what the changes in CSSParserImpl.cpp do) would be a better explanation.

> Source/WebCore/css/CSSProperties.json:641
> +                "related-property": "-webkit-text-orientation",

The name "related-property" isn't great, because it has no indication
A) What it does
B) What it's used for
C) That it only is meaningful for high priority properties

What do you think of the name "enforce-order-with"? Perhaps you can come up with something better.

> Source/WebCore/css/parser/CSSParserImpl.cpp:126
> +        const unsigned relatedPropertyId = getRelatedPropertyId(property.id());

Why is this before the "if" statement directly below it? It's usually best to keep related code together.

> Source/WebCore/style/StyleBuilderCustom.h:764
> +inline void BuilderCustom::applyValueTextOrientation(BuilderState& builderState, CSSValue& value)
> +{
> +    builderState.setTextOrientation(downcast<CSSPrimitiveValue>(value));
> +}

Is there any chance that we don't have to have an exact duplicate of BuilderCustom::applyValueWebkitTextOrientation()? What happens if you try to use the "name-for-methods" key in CSSProperties.json?

> LayoutTests/fast/text/orientation-mixed-unprefix-expected.html:9
> +<div id="t" style="-webkit-writing-mode: vertical-rl; -webkit-text-orientation: mixed;">è¹æå¬å¸abcd</div>

I think all these tests need <meta charset="utf-8"> in order to properly display this Unicode text.

> LayoutTests/fast/text/text-orientation-parse-competition.html:7
> +		<div id="test1" style="-webkit-writing-mode: vertical-rl; -webkit-text-orientation: sideways; text-orientation: upright">你好ABC</div>

I think there should be another check in here with the order of text-orientation and -webkit-text-orientation reversed.

> LayoutTests/fast/text/text-orientation-parse.html:61
> +			shouldBeEqualToString("window.getComputedStyle(document.getElementById('test1')).webkitTextOrientation", "sideways");

I think we also need a parsing test that tests CSSValues themselves. Something like:

<style id="thestyle">
dummy {
    text-orientation: upright;
    -webkit-text-orientation: sideways;
}
</style>
...
document.getElementById("thestyle").sheet.cssRules[0].style.getPropertyValue("text-orientation") == ...;
document.getElementById("thestyle").sheet.cssRules[0].style.getPropertyValue("-webkit-text-orientation") == ...;

Testing all the various combinations of this would be helpful.

> LayoutTests/fast/text/text-orientation-parse.html:94
> +			shouldBeEqualToString("window.getComputedStyle(document.getElementById('test27')).webkitTextOrientation", "sideways");

Shouldn't we also be checking .textOrientation in addition to .webkitTextOrientation?

Shouldn't we also be testing .getPropertyValue("text-orientation") in addition to .getPropertyValue("-webkit-text-orientation")?

I don't see any checks to make sure the new unprefixed property explicitly doesn't parse any of the nonstandard values. Did I just miss them?

Created attachment 394292 [details]
Patch

Created attachment 394297 [details]
Patch

[EWS]

Myles C. Naxfield found in /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Created attachment 394527 [details]
Patch

[EWS]

Committed r259006: <https://trac.webkit.org/changeset/259006>

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