Bug 237175

<rdar://problem/89479461>

Created attachment 453232 [details] Patch

Created attachment 453312 [details] Patch

Created attachment 453390 [details] Patch

Created attachment 453413 [details] Patch

Comment on attachment 453413 [details] Patch PTAL. I did some editing changes to preserve test expectations using text-decoration, but not sure if you would prefer just replacing text-decoration with text-decoration-line and keep the logic as-is.

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review This looks good to me overall. Could use better test coverage for some of the new behaviors and at least one function has gotten noticeably less clear. > Source/WebCore/ChangeLog:28 > + - Use 'text-decoration-line' get getting values. get getting -> when getting > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3524 > + return nullptr; Is it really OK to just return null in cases like this? Can we add test coverage for this so we can compare behavior with other browser engines for interoperability? > Source/WebCore/css/StyleProperties.cpp:286 > + return String(); What is the impact of returning null string in cases like this? Again can we create test coverage? > Source/WebCore/css/parser/CSSPropertyParser.cpp:6288 > + RefPtr<CSSValue> line = consumeTextDecorationLine(m_range); auto > Source/WebCore/editing/EditingStyle.cpp:602 > + style->setProperty(CSSPropertyTextDecoration, valueList->cssText()); This seems really unfortunate, to be serializing snd then parsing the text form. The rest of this code goes out of its way to use the object model, not text. Presumably for multiple reasons including performance. > Source/WebCore/editing/EditingStyle.cpp:604 > + style->setProperty(CSSPropertyTextDecoration, "none"); Ditto. > Source/WebCore/editing/EditingStyle.cpp:903 > + newInlineStyle->setProperty(CSSPropertyTextDecoration, newValueList->cssText()); Again. > Source/WebCore/editing/EditingStyle.cpp:1467 > void EditingStyle::removeEquivalentProperties(T& style) Disappointing how much more complex the algorithm has become. Worth considering refactoring further to make it easier to understand. > Source/WebCore/editing/markup.cpp:939 > + fullySelectedRootStyle->style()->setProperty(CSSPropertyTextDecoration, "none"); Again.

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3524 >> + return nullptr; > > Is it really OK to just return null in cases like this? Can we add test coverage for this so we can compare behavior with other browser engines for interoperability? When a shorthand can't represent the values of it's longhands, it should be serialized as empty string. This is per CSSOM spec: https://drafts.csswg.org/cssom/#serialize-a-css-value «If there is no such shorthand or shorthand cannot exactly represent the values of all the properties in list, return the empty string.» It's just that other browsers obey the text decoration spec, and the text-decoration shorthand accepts values for any of its longhands, so the serialization works. But in WebKit, text-decoration only accepts the same values as text-decoration-line. I thought it would be better to keep the accepted syntax as-is for now, but I can expand it if you prefer. >> Source/WebCore/css/StyleProperties.cpp:286 >> + return String(); > > What is the impact of returning null string in cases like this? Again can we create test coverage? The WPT css/css-text-decor/parsing/text-decoration-valid.html checks that the shorthand can accept and serialize the various longhands. But again, I wanted to minimize the changes so I kept the syntax as-is. So the test continues failing. >> Source/WebCore/editing/EditingStyle.cpp:602 >> + style->setProperty(CSSPropertyTextDecoration, valueList->cssText()); > > This seems really unfortunate, to be serializing snd then parsing the text form. The rest of this code goes out of its way to use the object model, not text. Presumably for multiple reasons including performance. Yes, but when using a CSSValue, shorthands are not expanded into longhands :( If you prefer I can leave the logic as-is, and just get/set/remove CSSPropertyTextDecorationLine instead of CSSPropertyTextDecoration. Then I will need to update various editing test expectations.

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review >>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3524 >>> + return nullptr; >> >> Is it really OK to just return null in cases like this? Can we add test coverage for this so we can compare behavior with other browser engines for interoperability? > > When a shorthand can't represent the values of it's longhands, it should be serialized as empty string. > This is per CSSOM spec: https://drafts.csswg.org/cssom/#serialize-a-css-value > «If there is no such shorthand or shorthand cannot exactly represent the values of all the properties in list, return the empty string.» > > It's just that other browsers obey the text decoration spec, and the text-decoration shorthand accepts values for any of its longhands, so the serialization works. > But in WebKit, text-decoration only accepts the same values as text-decoration-line. > I thought it would be better to keep the accepted syntax as-is for now, but I can expand it if you prefer. That’s fine. We need test coverage for this behavior, even if it’s behavior we plan to fix and improve. >>> Source/WebCore/css/StyleProperties.cpp:286 >>> + return String(); >> >> What is the impact of returning null string in cases like this? Again can we create test coverage? > > The WPT css/css-text-decor/parsing/text-decoration-valid.html checks that the shorthand can accept and serialize the various longhands. > But again, I wanted to minimize the changes so I kept the syntax as-is. So the test continues failing. That’s fine. We need test coverage for this behavior, even if it’s behavior we plan to fix and improve.

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review >>> Source/WebCore/editing/EditingStyle.cpp:602 >>> + style->setProperty(CSSPropertyTextDecoration, valueList->cssText()); >> >> This seems really unfortunate, to be serializing snd then parsing the text form. The rest of this code goes out of its way to use the object model, not text. Presumably for multiple reasons including performance. > > Yes, but when using a CSSValue, shorthands are not expanded into longhands :( > If you prefer I can leave the logic as-is, and just get/set/remove CSSPropertyTextDecorationLine instead of CSSPropertyTextDecoration. > Then I will need to update various editing test expectations. Is the use of serializing through text the long term strategy here? Or a short term stopgap? I don’t understand why we need to serialize and deserialize to get the effect you describe.

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review >> Source/WebCore/editing/EditingStyle.cpp:604 >> + style->setProperty(CSSPropertyTextDecoration, "none"); > > Ditto. The way to do this without involving parsing would be to set the four longhands instead of setting the shorthand.

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review >> Source/WebCore/ChangeLog:28 >> + - Use 'text-decoration-line' get getting values. > > get getting -> when getting Done. >>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3524 >>>> + return nullptr; >>> >>> Is it really OK to just return null in cases like this? Can we add test coverage for this so we can compare behavior with other browser engines for interoperability? >> >> When a shorthand can't represent the values of it's longhands, it should be serialized as empty string. >> This is per CSSOM spec: https://drafts.csswg.org/cssom/#serialize-a-css-value >> «If there is no such shorthand or shorthand cannot exactly represent the values of all the properties in list, return the empty string.» >> >> It's just that other browsers obey the text decoration spec, and the text-decoration shorthand accepts values for any of its longhands, so the serialization works. >> But in WebKit, text-decoration only accepts the same values as text-decoration-line. >> I thought it would be better to keep the accepted syntax as-is for now, but I can expand it if you prefer. > > That’s fine. We need test coverage for this behavior, even if it’s behavior we plan to fix and improve. Extended fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html to check this. >>>> Source/WebCore/css/StyleProperties.cpp:286 >>>> + return String(); >>> >>> What is the impact of returning null string in cases like this? Again can we create test coverage? >> >> The WPT css/css-text-decor/parsing/text-decoration-valid.html checks that the shorthand can accept and serialize the various longhands. >> But again, I wanted to minimize the changes so I kept the syntax as-is. So the test continues failing. > > That’s fine. We need test coverage for this behavior, even if it’s behavior we plan to fix and improve. Extended fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html to check this. >> Source/WebCore/css/parser/CSSPropertyParser.cpp:6288 >> + RefPtr<CSSValue> line = consumeTextDecorationLine(m_range); > > auto Done. >>>> Source/WebCore/editing/EditingStyle.cpp:602 >>>> + style->setProperty(CSSPropertyTextDecoration, valueList->cssText()); >>> >>> This seems really unfortunate, to be serializing snd then parsing the text form. The rest of this code goes out of its way to use the object model, not text. Presumably for multiple reasons including performance. >> >> Yes, but when using a CSSValue, shorthands are not expanded into longhands :( >> If you prefer I can leave the logic as-is, and just get/set/remove CSSPropertyTextDecorationLine instead of CSSPropertyTextDecoration. >> Then I will need to update various editing test expectations. > > Is the use of serializing through text the long term strategy here? Or a short term stopgap? I don’t understand why we need to serialize and deserialize to get the effect you describe. Changed to setting the 4 longhands. >>> Source/WebCore/editing/EditingStyle.cpp:604 >>> + style->setProperty(CSSPropertyTextDecoration, "none"); >> >> Ditto. > > The way to do this without involving parsing would be to set the four longhands instead of setting the shorthand. Done. >> Source/WebCore/editing/EditingStyle.cpp:903 >> + newInlineStyle->setProperty(CSSPropertyTextDecoration, newValueList->cssText()); > > Again. Changed to setting the 4 longhands. >> Source/WebCore/editing/EditingStyle.cpp:1467 >> void EditingStyle::removeEquivalentProperties(T& style) > > Disappointing how much more complex the algorithm has become. Worth considering refactoring further to make it easier to understand. What refactoring do you have in mind? >> Source/WebCore/editing/markup.cpp:939 >> + fullySelectedRootStyle->style()->setProperty(CSSPropertyTextDecoration, "none"); > > Again. Changed to setting the 4 longhands.

Created attachment 453644 [details] Patch

Comment on attachment 453413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review >>> Source/WebCore/editing/EditingStyle.cpp:1467 >>> void EditingStyle::removeEquivalentProperties(T& style) >> >> Disappointing how much more complex the algorithm has become. Worth considering refactoring further to make it easier to understand. > > What refactoring do you have in mind? Mainly cutting down on loops and making logic more direct. Well, one refactoring would be that since the loop handles each property once, a separate step would be to build a set and then we would iterate the set rather than building the "handle each property only once" into the same loop that does the work. Might need a ListHashSet to preserve the ordering, though. It also seems strange, on reflection, that the loop handles shorthands of they show up after a longhand than if the show up before it, since the shorthands are all added to "alreadyHandled" if you encounter a longhand. Seems like the canRemoveAllLonghands logic could be broken out into a function. With some other fiddling we could make this easier to read. You don’t have to do this, but this was a really simple function before, and I do think decomposing it a bit will make it simpler.

(In reply to Darin Adler from comment #14) > Comment on attachment 453413 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453413&action=review > > >>> Source/WebCore/editing/EditingStyle.cpp:1467 > >>> void EditingStyle::removeEquivalentProperties(T& style) > >> > >> Disappointing how much more complex the algorithm has become. Worth considering refactoring further to make it easier to understand. > > > > What refactoring do you have in mind? > > Mainly cutting down on loops and making logic more direct. > > Well, one refactoring would be that since the loop handles each property > once, a separate step would be to build a set and then we would iterate the > set rather than building the "handle each property only once" into the same > loop that does the work. Might need a ListHashSet to preserve the ordering, > though. > > It also seems strange, on reflection, that the loop handles shorthands of > they show up after a longhand than if the show up before it, since the > shorthands are all added to "alreadyHandled" if you encounter a longhand. > > Seems like the canRemoveAllLonghands logic could be broken out into a > function. > > With some other fiddling we could make this easier to read. You don’t have > to do this, but this was a really simple function before, and I do think > decomposing it a bit will make it simpler. Should I file another bug for that?

(In reply to Oriol Brufau from comment #15) > Should I file another bug for that? It’s not mandatory, but if you have the time to follow up and improve it a bit I’d welcome that.

Committed r290756 (248000@main): <https://commits.webkit.org/248000@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453644 [details].

Bug 230083 was originally filed for this task fwiw, and had a patch on it. Thanks for working on this!

Re-opened since this is blocked by bug 237412

Testing locally, it seems that making text-decoration be a shorthand of only text-decoration-line doesn't regress perf, so I guess I can try that as a first step, and leave the other longhands for bug 230083.

Sounds like a good strategy; I’m sure we’ll find a way to do it all with high performance, too.

Created attachment 453793 [details] Patch

Created attachment 454019 [details] Patch

Darin, can you confirm if the new patch looks good?

Committed r291244 (248399@main): <https://commits.webkit.org/248399@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454019 [details].