Bug 190496

Comment on attachment 352116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352116&action=review > Source/WebCore/ChangeLog:9 > + Now all shorthands (except buggy 'font' and '-webkit-mask-repeat') can be > + serialized in cssText. The value will be obtained via 'getPropertyValue'. When was this spec change made? What do other browsers do? This seems like a serious compat risk. > Source/WebCore/ChangeLog:21 > + Test: editing/pasteboard/copy-paste-inserts-clearing-div.html > + Test: fast/css/background-position-serialize.html > + Test: fast/css/cssText-shorthand.html > + Test: fast/css/remove-shorthand.html > + Test: web-platform-tests/css/cssom/shorthand-values.html This is a wrong format. There should be a single "Tests:" which lists all these tests which got updated. > LayoutTests/fast/css/cssText-shorthand-expected.txt:13 > -PASS normalizeCssText(element.style.cssText) is "border-bottom-width: 1px; border-left-width: 1px; border-right-width: 1px; border-top-width: 1px !important" > +FAIL normalizeCssText(element.style.cssText) should be border-bottom-width: 1px; border-left-width: 1px; border-right-width: 1px; border-top-width: 1px !important. Was border-bottom: 1px; border-left: 1px; border-right: 1px; border-top: 1px !important. Please update the test so that this will be pass. > LayoutTests/imported/w3c/web-platform-tests/css/cssom/shorthand-values-expected.txt:9 > -PASS The serialization of border: 1px; border-top: 1px !important; should be canonical. > +FAIL The serialization of border: 1px; border-top: 1px !important; should be canonical. assert_equals: expected "border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px; border-top-width: 1px !important;" but got "border-right: 1px; border-bottom: 1px; border-left: 1px; border-top: 1px !important;" Why is this WPT test failing? Are we sure new results is correct? If so, why is the upstream test not yet fixed?

Comment on attachment 352116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352116&action=review >> Source/WebCore/ChangeLog:9 >> + serialized in cssText. The value will be obtained via 'getPropertyValue'. > > When was this spec change made? What do other browsers do? > This seems like a serious compat risk. Sorry I wasn't clear enough. The spec says that these should also be serialized, but I excluded them because they serialize incorrectly and currently they are excluded. For example, $0.style.fontSize = "12px"; $0.style.fontFamily = "serif"; $0.style.getPropertyValue("font"); The shorthand serializes as "12px serif" but not all longhands are present, so it should be empty string. Or for -webkit-mask-repeat, $0.style.cssText = "-webkit-mask-repeat: repeat-x; -webkit-mask-repeat-y: inherit;"; $0.style.getPropertyValue("-webkit-mask-repeat"); The shorthand returns "inherit" but the -x longhand is "repeat". The can be enabled once the serialization is fixed. Both Firefox and blink serialize 'font' and '-webkit-mask-repeat', but they do it correctly (note Firefox doesn't have the '-webkit-mask-repeat-*' longhands). >> LayoutTests/fast/css/cssText-shorthand-expected.txt:13 >> +FAIL normalizeCssText(element.style.cssText) should be border-bottom-width: 1px; border-left-width: 1px; border-right-width: 1px; border-top-width: 1px !important. Was border-bottom: 1px; border-left: 1px; border-right: 1px; border-top: 1px !important. > > Please update the test so that this will be pass. OK, but then it will be different than the analogous test in WPT. >> LayoutTests/imported/w3c/web-platform-tests/css/cssom/shorthand-values-expected.txt:9 >> +FAIL The serialization of border: 1px; border-top: 1px !important; should be canonical. assert_equals: expected "border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px; border-top-width: 1px !important;" but got "border-right: 1px; border-bottom: 1px; border-left: 1px; border-top: 1px !important;" > > Why is this WPT test failing? Are we sure new results is correct? > If so, why is the upstream test not yet fixed? I'm sure that using border-{side} instead of border-{side}-width is correct, and Firefox and Blink agree. However, there is no interoperability with what should happen regarding border-image. WebKit does not include that because it ignores "initial" values in cssText (bug 185953). Blink includes "border-image: initial". Firefox includes "border-image: none 100% / 1 / 0 stretch;". I suspect the test has not been fixed upstream due to this lack of interoperability. You can see the results in https://wpt.fyi/results/css/cssom/shorthand-values.html?label=experimental

Created attachment 352161 [details] Patch

Fixed ChangeLog format for tests and clarified description. Added TODO comment in StyleProperties.cpp Fixed fast/css/cssText-shorthand.html expectations so that it passes.

Comment on attachment 352116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352116&action=review >> LayoutTests/imported/w3c/web-platform-tests/css/cssom/shorthand-values-expected.txt:9 >> +FAIL The serialization of border: 1px; border-top: 1px !important; should be canonical. assert_equals: expected "border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px; border-top-width: 1px !important;" but got "border-right: 1px; border-bottom: 1px; border-left: 1px; border-top: 1px !important;" > > Why is this WPT test failing? Are we sure new results is correct? > If so, why is the upstream test not yet fixed? A CSSOM editor confirmed that the test expectation is wrong: > Yeah, I agree that that entry is wrong. A browser that passes that subtest is clearly bogus. https://github.com/web-platform-tests/wpt/issues/13523#issuecomment-429944937

I have a serious compatibility concern for this change. The last time we made changes to the way we changed cssText serialization, we got a long tail of random bugs.

(In reply to Ryosuke Niwa from comment #12) > I have a serious compatibility concern for this change. The last time we > made changes to the way we changed cssText serialization, we got a long tail > of random bugs. If you prefer I can exclude all shorthands that currently are not serialized. Then the change in behavior will be minimal. But I need this kind of refactorization in order to be able to properly hide CSS properties behind runtime flags, which I want to do for bug 188697.

(In reply to Oriol Brufau from comment #13) > (In reply to Ryosuke Niwa from comment #12) > > I have a serious compatibility concern for this change. The last time we > > made changes to the way we changed cssText serialization, we got a long tail > > of random bugs. > > If you prefer I can exclude all shorthands that currently are not > serialized. Then the change in behavior will be minimal. But I need this > kind of refactorization in order to be able to properly hide CSS properties > behind runtime flags, which I want to do for bug 188697. It appears to me that we should just special-case logical properties we're adding in the bug 188697. The merit of whether the seralization form of general CSS properties should be discussed separately from implementing logical properties. For example, do we have examples of website compatibility that arises from WebKit's behavior not following that of the spec unlike Blink? We also need to study the history of this code to understand whether such a change is safe to make. That is, we should study where each line of code comes from using SVG/git blame and figure out if any of the behaviors were needed for Web compatibility. I know for a fact, for example, reducing the amount of text we generate in inline style attributes in editing code is extremely important. Are we sure the string we generate with your patch for a given CSS during editing is not longer?

(In reply to Ryosuke Niwa from comment #14) > It appears to me that we should just special-case logical properties we're > adding in the bug 188697. The merit of whether the seralization form of > general CSS properties should be discussed separately from implementing > logical properties. At this point this is not just about logical properties, e.g. due to lack of a proper way to hide CSS properties behind a runtime flag, the CSS3 Text features are hidden behind a compile flag, and all the tests are disabled! There is no protection against regressions. So I do think WebKit needs a general-purpose way to hide CSS properties behind a runtime flag. And in cssText, this means that if a longhand has multiple shorthands and one is disabled, you need to choose another one. This is very cumbersome to do with the current code. > For example, do we have examples of website compatibility that arises from > WebKit's behavior not following that of the spec unlike Blink? We also need > to study the history of this code to understand whether such a change is > safe to make. That is, we should study where each line of code comes from > using SVG/git blame and figure out if any of the behaviors were needed for > Web compatibility. Most of it comes from https://chromium.googlesource.com/chromium/src.git/+/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df. https://crbug.com/612363 provides this reasoning: > While this is primarily motivated by code health, this is also likely to fix many inconsistencies where the generic checks are implemented slightly wrongly in different places. > I know for a fact, for example, reducing the amount of text we generate in > inline style attributes in editing code is extremely important. Are we sure > the string we generate with your patch for a given CSS during editing is not > longer? Currently not all shorthands are taken into consideration in cssText, so multiple longhands are sometimes used even if a single shorthand would work. So I expect my patch to reduce the amount of text. I don't really think my patch can increase the output because the value is still serialized like this: value = getPropertyValue(shorthandPropertyID); The shorthandPropertyID varies with my patch but it prefers shorthand with a greater amount of longhands, and these shorthands tend to have a shorter name (and serializing more longhands with a single shorthand likely shortens the total output). The only cases that currently don't use getPropertyValue are CSSPropertyBorder which uses borderPropertyValue (but getPropertyValue uses borderPropertyValue so it doesn't matter), and CSSPropertyBackgroundRepeat and CSSPropertyBackgroundPosition which build the value manually from the longhands (getPropertyValue uses getLayeredShorthandValue, and if this returns a different value then it's a bug).

(In reply to Oriol Brufau from comment #15) > (In reply to Ryosuke Niwa from comment #14) > > It appears to me that we should just special-case logical properties we're > > adding in the bug 188697. The merit of whether the seralization form of > > general CSS properties should be discussed separately from implementing > > logical properties. > > At this point this is not just about logical properties, e.g. due to lack of > a proper way to hide CSS properties behind a runtime flag, the CSS3 Text > features are hidden behind a compile flag, and all the tests are disabled! > There is no protection against regressions. > > So I do think WebKit needs a general-purpose way to hide CSS properties > behind a runtime flag. And in cssText, this means that if a longhand has > multiple shorthands and one is disabled, you need to choose another one. > This is very cumbersome to do with the current code. Yeah, we probably have to do one property at a time but we shouldn't be changing the behavior of WebKit just because something is easier to code when there is a significant compatibility risk. > > I don't really think my patch can increase the output because the value is > still serialized like this: > value = getPropertyValue(shorthandPropertyID); > > The shorthandPropertyID varies with my patch but it prefers shorthand with a > greater amount of longhands, and these shorthands tend to have a shorter > name (and serializing more longhands with a single shorthand likely shortens > the total output). > > The only cases that currently don't use getPropertyValue are > CSSPropertyBorder which uses borderPropertyValue (but getPropertyValue uses > borderPropertyValue so it doesn't matter), and CSSPropertyBackgroundRepeat > and CSSPropertyBackgroundPosition which build the value manually from the > longhands (getPropertyValue uses getLayeredShorthandValue, and if this > returns a different value then it's a bug). That's good. Have we also studied where this serialization code came from? e.g. I did https://trac.webkit.org/changeset/112177 to support more shorthands in cssText awhile ago. Also, please make sure we're not re-introducing bugs like https://trac.webkit.org/changeset/101970. In general, please look at svn/git blames & track past changes to see if there is any compatibility risk with this change.

(In reply to Ryosuke Niwa from comment #16) > Yeah, we probably have to do one property at a time but we shouldn't be > changing the behavior of WebKit just because something is easier to code > when there is a significant compatibility risk. I would argue that the compatibility risk is with the current code, since it is buggy, error-prone, not conformant with the spec, and not interoperable with other implementations. > That's good. Have we also studied where this serialization code came from? > e.g. I did https://trac.webkit.org/changeset/112177 to support more > shorthands in cssText awhile ago. So my patch just automatizes what you did there in order to cover all shorthands, and properly handles cases with longhands belonging to multiple shorthands (e.g. you had to use "FIXME: Deal with cases where only some of border-(top|right|bottom|left) are specified" because your approach doesn't seem suitable for this). > Also, please make sure we're not re-introducing bugs like > https://trac.webkit.org/changeset/101970. My patch just affects cssText serialization. It doesn't effect things like removeProperty, getPropertyValue nor setProperty. > In general, please look at svn/git blames & track past changes to see if > there is any compatibility risk with this change. Not sure what exactly you want me to look at. My patch should behave exactly like the current code when the longhands have a single shorthand (and this relationship is included in the switch, and the shorthands are not CSSPropertyBackgroundRepeat or CSSPropertyBackgroundPosition).

I still think this patch poses a high risk of backwards compatibility issues particularly against content which only targets iOS Safari, and native apps which embed WebKit since those apps are more likely to be relying on the way we currently serials CSS properties.

Comment on attachment 352161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352161&action=review > Source/WebCore/css/StyleProperties.cpp:887 > + // TODO: remove the following exclusions. Not sure this TODO comment is necessary or helpful. The comments on the specific exclusions explain why they can't be removed for now. > Source/WebCore/css/StyleProperties.cpp:-1092 > - // FIXME: This is a not-so-nice way to turn x/y positions into single background-position in output. > - // It is required because background-position-x/y are non-standard properties and WebKit generated output > - // would not work in Firefox (<rdar://problem/5143183>) > - // It would be a better solution if background-position was CSS_PAIR. The code deleted here for background-positon-x and background-position-y is presumably an observable behavior change, but that change is not covered in tests. > LayoutTests/imported/w3c/web-platform-tests/css/cssom/shorthand-values-expected.txt:9 > +FAIL The serialization of border: 1px; border-top: 1px !important; should be canonical. assert_equals: expected "border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px; border-top-width: 1px !important;" but got "border-right: 1px; border-bottom: 1px; border-left: 1px; border-top: 1px !important;" Can we get this test updated upstream so that the passing expectation matches what is correct per spec? Or at least change our local copy of the test?

Comment on attachment 352161 [details] Patch Also, this patch no longer applies. We'll need a new version.

What I wanted to do here has already been done as part of other issues. cssText still has some bugs like bug 247741, but I think this can be closed.

<rdar://problem/105235157>