Bug 236272

Summary:

[css] Can't revert low-priority properties like text-decoration

Product:

WebKit

Reporter:

Oriol Brufau <obrufau>

Component:

CSS

Assignee:

Oriol Brufau <obrufau>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

clopez, darin, ews-watchlist, graouts, koivisto, mmaxfield, simon.fraser, webkit-bug-importer, youennf

Priority:

Keywords:

BrowserCompat, InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://github.com/web-platform-tests/wpt/pull/33036

Bug Depends on:

237175

Bug Blocks:

236199

Attachments:

Description	Flags
testcase	none
Patch	none
Patch	none
Patch	koivisto: review-

Description Oriol Brufau 2022-02-07 16:39:42 PST

Created attachment 451172 [details]
testcase

All of these should look identical:

  <u style="all: revert">foo</u>
  <u style="text-decoration: revert">foo</u>
  <u style="all: revert-layer">foo</u>
  <u style="text-decoration: revert-layer">foo</u>
  <u>foo</u>

But only the last one has the underline set in UA origin.

That's because text-decoration is low-priority / deferred / applied in parse order.
`revert` and `revert-layer` don't seem to work on these properties:
https://webkit-search.igalia.com/webkit/rev/30ea950a1b2947097300016afbe68f78665b0e43/Source/WebCore/style/PropertyCascade.cpp#41-73

Comment 1 Radar WebKit Bug Importer 2022-02-14 16:40:17 PST

<rdar://problem/88935655>

Comment 2 Oriol Brufau 2022-03-02 15:38:54 PST

Created attachment 453668 [details]
Patch

Comment 3 EWS Watchlist 2022-03-02 15:40:43 PST

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Comment 4 Oriol Brufau 2022-03-14 16:35:18 PDT

Created attachment 454635 [details]
Patch

Comment 5 Oriol Brufau 2022-03-14 16:41:55 PDT

Just changed the test to use /html/resources/common.js instead of hardcoding a list of elements.

Comment 6 Oriol Brufau 2022-03-14 17:37:04 PDT

Created attachment 454640 [details]
Patch

Comment 7 Oriol Brufau 2022-03-14 17:38:25 PDT

Comment on attachment 454640 [details]
Patch

I forgot that all-prop-revert-layer-noop.html passes after this.

Comment 8 EWS 2022-03-14 18:49:24 PDT

Committed r291260 (248412@main): <https://commits.webkit.org/248412@main>

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

Comment 9 Antti Koivisto 2022-03-23 06:37:35 PDT

Comment on attachment 454640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454640&action=review

> Source/WebCore/style/PropertyCascade.cpp:176
> +    m_deferredPropertiesIndices.set(id, m_deferredProperties.size());

It is not ok to add a HashMap (and a heap allocation) to this generic code path when it is only needed and used in the rare rollback case. The code should be structured differently.

Comment 10 Antti Koivisto 2022-03-23 09:51:05 PDT

In https://bugs.webkit.org/show_bug.cgi?id=238260 this HashMap is used in sensible way to eliminate the m_deferredProperties member. In that context this patch makes more sense.

Still, it is not clear it should be a HashMap.