Bug 196139

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.

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

<rdar://problem/56623427>

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

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

Created attachment 393089 [details] Patch

Comment on attachment 393089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393089&action=review > Source/WebCore/ChangeLog:1 > +2020-03-09 Frank Yang <guowei_yang@apple.com> 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

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

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

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

Committed r259006: <https://trac.webkit.org/changeset/259006> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394527 [details].