Created attachment 393055 [details] Comparison between the correct text baseline alignment and incorrect baseline alignment Webkit has the -webkit-text-orientation property, and ideally we want to have it match the spec mentioned in https://drafts.csswg.org/css-writing-modes/#text-orientation and https://drafts.csswg.org/css-writing-modes/#text-baselines Currently, the problem for -webkit-text-orientation is in the text baseline selection. According to the spec related to 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 in the case of Webkit, alphabetic baseline is used in both sideways and mixed, which is incorrect. One example is shown below: <style> div > div { font-size: 200%; } </style> <div style="writing-mode: vertical-lr; -webkit-text-orientation: mixed"> before <div style="display: inline-block; background: lime; height: 100px; width: 100px;">foobar</div> after </div> Current WebKit renders the text in a fashion shown in the attached image on the left, which is aligned on their alphabetic baseline, whereas according to the spec, they should be aligned using the ideographic baseline because we are in vertical writing mode and text orientation is mixed. The correct result is shown on the right.
<rdar://problem/60231910>
Created attachment 393065 [details] Patch
Created attachment 393072 [details] Patch
Created attachment 393087 [details] Patch
Comment on attachment 393087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393087&action=review > Source/WebCore/ChangeLog:12 > + (WebCore::InlineFlowBox::requiresIdeographicBaseline const): Please explain the intent of the change. What weren't we matching correctly?
Created attachment 393094 [details] Patch
Comment on attachment 393094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393094&action=review > Source/WebCore/ChangeLog:8 > + According to the CSS documentation, in vertical writing mode, ideographic baseline Use check-webkit-style before uploading patches. You have tabs here (should be 8 spaces)
Created attachment 393106 [details] Patch
Comment on attachment 393106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393106&action=review Not done reviewing yet, more coming later tonight. > Source/WebCore/ChangeLog:3 > + -webkit-text-orientation Behavior Doesn't Match the Spec (Almost) every bug causes us to not match the spec. Please be more specific. > Source/WebCore/ChangeLog:8 > + According to the CSS documentation, in vertical writing mode, ideographic baseline s/CSS documentation/CSS writing-mode specification/ Please quote the full sentence from the spec, and link to it.
Comment on attachment 393106 [details] 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?
Created attachment 393286 [details] Patch
Comment on attachment 393286 [details] Patch This patch look good with respect to standard compliance. This patch will lead to a BAD user experience because: 1. This patch changes the behavior of a WebKit-prefixed CSS property, which was NEVER guaranteed to match the spec. 2. Because of (1) this change will break content, including Apple-produced iBooks and iTunes content that people may HAVE PAID FOR. This content will NEVER be updated. So, this change will negatively impact the customer experience. 3. Because of (1) this change will break content in third-party apps. The optimal solution is to implement the unprefixed CSS property THEN do this fix for it leaving the prefixed one behaving like it is.
Comment on attachment 393286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393286&action=review > Source/WebCore/WebCore.xcodeproj/xcshareddata/xcschemes/WebCore.xcscheme:45 > + FilePath = "/Users/framiere/projects/build/Debug/MiniBrowser.app"> This change needs to be removed. > LayoutTests/ChangeLog:10 > + WebKit, which is 1) webkit cannot parse "text-orientation" since it's not supported yet, and 2) Webkit doesn't distinguish This is ok as-is. No change is needed. webkit, Webkit => WebKit <--- that,# the official marketing name. Also, again ok as-is, no change needed, the lines in the changelog should be wrapped at 100 columns
(In reply to Daniel Bates from comment #12) > Comment on attachment 393286 [details] > Patch > > This patch look good with respect to standard compliance. This patch will > lead to a BAD user experience because: > > 1. This patch changes the behavior of a WebKit-prefixed CSS property, which > was NEVER guaranteed to match the spec. > 2. Because of (1) this change will break content, including Apple-produced > iBooks and iTunes content that people may HAVE PAID FOR. This content will > NEVER be updated. So, this change will negatively impact the customer > experience. > 3. Because of (1) this change will break content in third-party apps. > > The optimal solution is to implement the unprefixed CSS property THEN do > this fix for it leaving the prefixed one behaving like it is. Using the ideographic baseline in vertical text is a progression, not a regression. this patch improves typography for all those users who have paid for their books. It's a calculated risk: - The new behavior matches all the other browsers - The new behavior matches the spec - Typographically it's the right behavior - As you can see from the updated layout test render tree dumps, the behavior change is generally quite small, moving text only a few in the block direction. The risk is not very high. I don't think our policy should be to never try to improve anything that might possibly be used in paid content.
* only a few pixels
I shouldn’t say “might possibly be used.” Vertical writing mode is used in paid content. The current behavior, before this patch, is to use the ideographic baseline if any of the used or primary fonts in the line have the VORG table or the vhea table, which is the common case for vertical writing mode content. So this behavior change is already more finely scoped than I mentioned in my previous comment. I also don’t know if it’s really possible to only have the behavior change affect the unprefixed property, because the behavior change occurs when text-orientation is mixed, which is the initial value. If content sets vertical writing mode but doesn’t set any text-orientation, the computed style will be that both text-orientation and -webkit-text-orientation will be set to mixed. At this point, there’s no way to differentiate between the two cases.
Created attachment 393418 [details] Patch
Created attachment 393420 [details] Patch
Comment on attachment 393420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393420&action=review This looks good. > LayoutTests/editing/selection/vertical-rl-rtl-extend-line-forward-br-mixed.html:9 > +<div id="test" style="font-family: monospace; font-size: 20px; height: 15ex; -webkit-writing-mode: vertical-rl; -webkit-text-orientation: mixed; outline: none;" contenteditable> It's kind of surprising that you made a new test with the same content as the old test, and then modified the old test, instead of not modifying the old test and adding a new one with the modifications .... but I guess it's fine. Not really worth changing at this point, but just surprising.
Comment on attachment 393420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393420&action=review > LayoutTests/editing/selection/vertical-rl-rtl-extend-line-backward-br-mixed-expected.txt:1 > +layer at (0,0) size 800x600 Render tree dumps are platform-dependent, so these should go in platform/mac instead.
Created attachment 393951 [details] Patch
Created attachment 393954 [details] Patch
Created attachment 394040 [details] Patch
Created attachment 394056 [details] Patch
Created attachment 394065 [details] Patch
Created attachment 394448 [details] Patch
Created attachment 394451 [details] Patch
Created attachment 394454 [details] Patch
Created attachment 394460 [details] Patch
Attachment 394460 [details] does not apply
Created attachment 394505 [details] Patch
Committed r258990: <https://trac.webkit.org/changeset/258990> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394505 [details].