[Webkit-unassigned] [Bug 208824] -webkit-text-orientation Behavior Doesn't Match the Spec

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 9 22:12:59 PDT 2020


https://bugs.webkit.org/show_bug.cgi?id=208824

Myles C. Maxfield <mmaxfield at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #393106|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #10 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 393106
  --> https://bugs.webkit.org/attachment.cgi?id=393106
Patch

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

It's a little scary to add text-orientation: sideways in so many of our vertical-writing-mode tests, because it's not the common case. I believe this patch significantly diminishes our test coverage of the default case of "text-orientation: mixed". I think we should consider updating more -expected.txt files, rather than modifying so many test files themselves.

r=me because I think this is the right direction and this patch is a progression, but I still think there's some work to do before landing.

> Source/WebCore/ChangeLog:9
> +        should be the dominant baseline of the text when text-orientation is "mixed" or "upright", 

"Dominant baseline" is a term-of-art. I don't think you intend to invoke it here. Better to refer to the specific wording used in the spec.

> Source/WebCore/ChangeLog:10
> +        and use alphabetic baseline when text-orientation property is "sideways". However in 

s/However in/However,/

> Source/WebCore/ChangeLog:13
> +        "upright", meaning it applies the same baseline for mixed and sideways text orientation
> +        therefore, a new clause is added to check if text-orientation is "mixed" 

s/orientation therefore/orientation. Therefore/

> Source/WebCore/ChangeLog:15
> +        (A "mixed" text orientaition is when in vertical writing mode, which isHorizontal() == false and 

s/orientaition/orientation/

> Source/WebCore/ChangeLog:16
> +        that font orientation in FontDescription == vertical and NonCJKGlyphOrientation == Mixed) 

I don't understand this sentence. Perhaps consider breaking it up into bullets?

> Source/WebCore/rendering/InlineFlowBox.cpp:456
>      if (lineStyle.fontDescription().nonCJKGlyphOrientation() == NonCJKGlyphOrientation::Upright
> +        || (lineStyle.fontDescription().orientation() == FontOrientation::Vertical
> +            && lineStyle.fontDescription().nonCJKGlyphOrientation() == NonCJKGlyphOrientation::Mixed)

I think this can be simplified. According to RenderStyle::fontAndGlyphOrientation(),

text-orientation | FontOrientation | NonCJKGlyphOrientation
-----------------+-----------------+-----------------------
mixed            | vertical        | mixed
upright          | vertical        | upright
sideways         | horizontal      | mixed

I believe this patch is trying to make InlineFlowBox::requiresIdeographicBaseline() select the "font-orientation: mixed" or "font-orientation: upright" case. This patch's logic:

NonCJKGlyphOrientation == upright // This selects the "text-orientation: mixed" case
|| (FontOrientation == vertical // This selects "text-orientation: mixed" or "text-orientation: upright"
    && NonCJKGlyphOrientation == Mixed) // Among "text-orientation: mixed" or "text-orientation: upright", this filters it down to just "text-orientation: upright"

I think you can get the exact same behavior if you replace all 3 lines with simply "lineStyle.fontDescription().orientation() == FontOrientation::Vertical".

> LayoutTests/imported/w3c/ChangeLog:9
> +        Imported new tests from web platform tests in css-writing-mode that webkit was failing previously
> +        all of which are testing text baseline alignment.

Did you have to modify any of the tests? Were there any tests which should now be passing after fixing this bug, but are still failing?

What percentage of css/css-writing-modes/ are we passing now?

> LayoutTests/editing/selection/vertical-rl-rtl-extend-line-backward-br.html:9
> +<div id="test" style="font-family: monospace; font-size: 20px; height: 15ex; -webkit-writing-mode: vertical-rl; -webkit-text-orientation: sideways; outline: none;" contenteditable>

The reasoning for the addition of these should be stated in the ChangeLog.

> LayoutTests/fast/inline-block/baseline-vertical-01.html:8
> +   -webkit-text-orientation: sideways;

The test name indicates that the test is trying to test baseline behavior. It appears that, by adding this property/value, you are changing what the test is trying to test. Therefore, you should do one of:

A) Indicate in the ChangeLog why it's okay to change the test to test something it wasn't originally trying to test
B) Indicate in the ChangeLog that this test isn't trying to test baseline behavior, so it's okay to make this change
C) Not make this change, and make a different one instead

> LayoutTests/fast/inline-block/baseline-vertical-02-expected.html:8
> -   text-orientation: sideways;
> +   -webkit-text-orientation: sideways;

Similar to above. This one is a little less obvious, because it appears the test is trying to test sideways orientation, but, in fact, it is actually testing mixed orientation.

Perhaps a better solution would be to make a copy of the test, so that we have one copy that tests mixed orientation and one copy that tests upright orientation. Unless we already have both versions?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200310/8f94b90c/attachment.htm>


More information about the webkit-unassigned mailing list