Bug 238126

Summary: [css-cascade] No need to defer applying text-decoration properties
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 237175    
Bug Blocks: 238125    
Attachments:
Description Flags
Patch none

Description Oriol Brufau 2022-03-20 08:41:05 PDT 
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.
Comment 1 Oriol Brufau 2022-03-20 08:54:10 PDT 
Created attachment 455196 [details]
Patch
Comment 2 Oriol Brufau 2022-03-20 10:40:48 PDT 
PTAL, trivial patch.
Comment 3 Darin Adler 2022-03-21 09:29:56 PDT 
Retrying to see if the iOS-wk2 failure goes away.
Comment 4 Oriol Brufau 2022-03-21 10:53:29 PDT 
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+
Comment 5 EWS 2022-03-21 11:01:18 PDT 
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].
Comment 6 Radar WebKit Bug Importer 2022-03-21 11:02:20 PDT 
<rdar://problem/90578051>
Comment 7 Darin Adler 2022-03-21 11:02:58 PDT 
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.
Comment 8 Oriol Brufau 2022-03-21 12:30:20 PDT 
(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.
Comment 9 Darin Adler 2022-03-21 12:39:30 PDT 
Seems like we need to change EWS to run the tests that failed *with* the patch too then, otherwise the A/B is invalid.