Bug 238126 – [css-cascade] No need to defer applying text-decoration properties

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

In Source/WebCore/style/PropertyCascade.cpp, shouldApplyPropertyInParseOrder() returns true for

 - CSSPropertyWebkitTextDecoration
 - CSSPropertyTextDecorationLine
 - CSSPropertyTextDecorationStyle
 - CSSPropertyTextDecorationColor
 - CSSPropertyTextDecorationSkip
 - CSSPropertyTextDecorationSkipInk
 - CSSPropertyTextUnderlinePosition
 - CSSPropertyTextUnderlineOffset
 - CSSPropertyTextDecorationThickness
 - CSSPropertyTextDecoration

This was previously necessary for text-decoration-line and text-decoration, since they were implemented as longhands that shared a computed value.
But that's no longer the case, text-decoration became a shorthand in bug 237175.
AFAIK -webkit-text-decoration has always been a shorthand since it was implemented in bug 92000, so having it in the list it's pointless, only longhands matter.
text-decoration-skip became a shorthand in bug 230244, so it's also pointless.
And the other longhands seem unnecessary too, since they don't share a computed style with other properties.

[Oriol Brufau]

Created attachment 455196 [details]
Patch

[Oriol Brufau]

PTAL, trivial patch.

[Darin Adler]

Retrying to see if the iOS-wk2 failure goes away.

[Oriol Brufau]

Comment on attachment 455196 [details]
Patch

I already retried iOS-wk2 a few times, but fast/text/text-shadow-ink-overflow-missing.html keeps failing all the time, and not just with my patch: https://ews-build.webkit.org/#/builders/iOS-15-Simulator-WK2-Tests-EWS
So it's unrelated, cq+

[EWS]

Committed r291568 (248669@main): <https://commits.webkit.org/248669@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455196 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90578051>

[Darin Adler]

That's not how EWS is supposed to work. If it fails both with and without the patch it's not supposed to be red. Look at an example like this one:

https://ews-build.webkit.org/#/builders/68/builds/11070

Note that it says the test *passes* without the patch it was evaluating. I wonder why EWS comes to that conclusion.

[Oriol Brufau]

(In reply to Darin Adler from comment #7)
> That's not how EWS is supposed to work. If it fails both with and without
> the patch it's not supposed to be red. Look at an example like this one:
> 
> https://ews-build.webkit.org/#/builders/68/builds/11070
> 
> Note that it says the test *passes* without the patch it was evaluating. I
> wonder why EWS comes to that conclusion.

I think it may be because when running the tests without the patch, it only runs the tests that previously failed, not all of them. Maybe this uses less resources on the machine or something, and affects the result? So it's not a matter of whether the patch is applied or not, but the number tests running at the same time. These just happen to be correlated.

[Darin Adler]

Seems like we need to change EWS to run the tests that failed *with* the patch too then, otherwise the A/B is invalid.