Bug 237487

Summary:

-webkit-border-image should be a shorthand

Product:

WebKit

Reporter:

Oriol Brufau <obrufau>

Component:

CSS

Assignee:

Oriol Brufau <obrufau>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, clopez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, joepeck, koivisto, kondapallykalyan, macpherson, menard, pdr, simon.fraser, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

238102

Bug Blocks:

238125

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch for EWS	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	none

Oriol Brufau

Reported 2022-03-04 15:07:10 PST

Like border-image, -webkit-border-image should be a shorthand of border-image-*. It can keep its quirk of adding 'fill' to border-image-slice. But it should be a shorthand because otherwise this doesn't work: <!DOCTYPE html> <style> @layer { #target { display: block; width: 100px; height: 100px; border-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 2 2" width="8" height="8"><circle cx="1" cy="1" r="1"></circle></svg>') 4 / 16px; } } @layer { #target { all: revert-layer; } } </style> <div id="target"></div> For starters, revert-layer doesn't work due to bug 236272 since 'border-image-*' are deferred properties. But even after applying the patch for that bug, it will still not work, because the 'all' shorthand will revert both 'border-image-*' and '-webkit-border-image'. And since '-webkit-border-image' is treated as a totally independent property, and it doesn't appear in any declaration (other than in 'all'), it's unset. Since it's around the end of the 'all' expansion, then it takes precedence over 'border-image-*'. This would be fine if '-webkit-border-image' was just a shorthand of 'border-image-*'. It should also be a shorthand of the 'border-*-width' properties, since it may set them. Otherwise: <div id="target" style="-webkit-border-image: 4 / 10px; border-style: solid; border-width: 20px;"></div> <script>document.body.append(getComputedStyle(target).borderWidth);</script> is 10px, should be 20px.

Attachments
Patch (13.83 KB, patch) 2022-03-05 12:32 PST, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (15.95 KB, patch) 2022-03-05 14:29 PST, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (38.63 KB, patch) 2022-03-07 15:59 PST, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (60.02 KB, patch) 2022-03-14 23:15 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (60.02 KB, patch) 2022-03-14 23:50 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (45.31 KB, patch) 2022-03-20 09:08 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch for EWS (53.17 KB, patch) 2022-03-20 10:34 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (84.19 KB, patch) 2022-04-01 14:09 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (84.20 KB, patch) 2022-04-01 14:25 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (108.70 KB, patch) 2022-04-01 18:04 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (109.23 KB, patch) 2022-04-02 08:11 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Oriol Brufau

Comment 1 2022-03-05 12:32:47 PST

Created attachment 453918 [details] Patch

Oriol Brufau

Comment 2 2022-03-05 14:29:34 PST

Created attachment 453921 [details] Patch

Oriol Brufau

Comment 3 2022-03-07 15:59:54 PST

Created attachment 454046 [details] Patch

Radar WebKit Bug Importer

Comment 4 2022-03-10 10:19:26 PST

<rdar://problem/90106626>

Oriol Brufau

Comment 5 2022-03-14 23:15:21 PDT

Created attachment 454667 [details] Patch

EWS Watchlist

Comment 6 2022-03-14 23:17:37 PDT

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

Oriol Brufau

Comment 7 2022-03-14 23:50:41 PDT

Created attachment 454668 [details] Patch

Oriol Brufau

Comment 8 2022-03-20 09:08:07 PDT

Created attachment 455197 [details] Patch

Oriol Brufau

Comment 9 2022-03-20 10:34:27 PDT

Created attachment 455200 [details] Patch for EWS

Oriol Brufau

Comment 10 2022-03-21 18:17:02 PDT

> It should also be a shorthand of the 'border-*-width' properties, since it > may set them. Otherwise: > > <div id="target" style="-webkit-border-image: 4 / 10px; border-style: > solid; border-width: 20px;"></div> > > <script>document.body.append(getComputedStyle(target).borderWidth);</script> > > is 10px, should be 20px. It's actually 20px now as a side-effect of bug 236199. But after further thought, this may not be the best model. Because if we say that -webkit-border-image is a shorthand whose expansion may include border-*-width or not depending on the value, then it gets tricky if it's assigned to a variable. Should border-*-width be set to a pending-substitution value? If not, and border-image-width end up having a length, then the border-*-width won't be affected. But if they are set to the pending-substitution value, and border-*-width isn't a length, then border-*-width will be reset to the initial value... I guess I could make it so that the pending-substitution value is not applied, now border-*-width are applied in parse order so the previous declaration will have been applied. But it seems quite hacky since it wouldn't work with other properties. So it may actually be better to implement this in a way analogous to border-*-width computing to 0 when border-*-style is none or hidden: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/style/BorderData.h#69 float borderLeftWidth() const { if (m_left.style() == BorderStyle::None || m_left.style() == BorderStyle::Hidden) return 0; return m_left.width(); } This means I will need a flag to track whether the border image comes from -webkit-border-image. I guess I can make it set border-image-width to some special internal value.

Oriol Brufau

Comment 11 2022-04-01 14:09:36 PDT

Created attachment 456400 [details] Patch

Oriol Brufau

Comment 12 2022-04-01 14:25:33 PDT

Created attachment 456404 [details] Patch

Oriol Brufau

Comment 13 2022-04-01 18:04:30 PDT

Created attachment 456423 [details] Patch

Oriol Brufau

Comment 14 2022-04-02 08:11:42 PDT

Created attachment 456458 [details] Patch

Antti Koivisto

Comment 15 2022-04-06 01:46:10 PDT

Comment on attachment 456458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456458&action=review > Source/WebCore/css/CSSBorderImageWidthValue.h:48 > + Quad* widths() const { return m_widths ? m_widths->quadValue() : nullptr; } > + > + bool equals(const CSSBorderImageWidthValue&) const; > + > + RefPtr<CSSPrimitiveValue> m_widths; Why do these structures need to be so complex? Here we have CSSBorderImageWidthValue which contains a CSSPrimitiveValue which is just a wrapper around a Quad. And then the Quad just wraps 4 CSSPrimitiveValues. Why can't this type just contain Quad directly? Or contain 4 CSSPrimitiveValues directly without a Quad? It this patch some sort of step towards making this saner?

Oriol Brufau

Comment 16 2022-04-06 04:53:46 PDT

(In reply to Antti Koivisto from comment #15) > Comment on attachment 456458 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=456458&action=review > > > Source/WebCore/css/CSSBorderImageWidthValue.h:48 > > + Quad* widths() const { return m_widths ? m_widths->quadValue() : nullptr; } > > + > > + bool equals(const CSSBorderImageWidthValue&) const; > > + > > + RefPtr<CSSPrimitiveValue> m_widths; > > Why do these structures need to be so complex? Here we have > CSSBorderImageWidthValue which contains a CSSPrimitiveValue which is just a > wrapper around a Quad. And then the Quad just wraps 4 CSSPrimitiveValues. > Why can't this type just contain Quad directly? Or contain 4 > CSSPrimitiveValues directly without a Quad? > > It this patch some sort of step towards making this saner? No, I just thought it would be nice to copy the same approach as CSSBorderImageSliceValue. But if you don't like these structures, I can probably do it without adding this CSSBorderImageWidthValue structure. I don't plan to do further work in -webkit-border-image I'm only doing this change because it's the only case of deferred properties where instead of having a pair of longhands, we have a longhand -webkit-border-image that maps into multiple longhands. But that's a mess with revert-layer and such.

Antti Koivisto

Comment 17 2022-04-06 05:18:02 PDT

Ok. I guess this stuff with Quads is better cleaned separately anyway.

EWS

Comment 18 2022-04-06 08:12:44 PDT

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

Note You need to log in before you can comment on or make changes to this bug.