Bug 237487

Summary: -webkit-border-image should be a shorthand
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: 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: P2 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-
DetailsFormatted DiffDiff
Patch (15.95 KB, patch)
2022-03-05 14:29 PST, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (38.63 KB, patch)
2022-03-07 15:59 PST, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (60.02 KB, patch)
2022-03-14 23:15 PDT, Oriol Brufau 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (60.02 KB, patch)
2022-03-14 23:50 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (45.31 KB, patch)
2022-03-20 09:08 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch for EWS (53.17 KB, patch)
2022-03-20 10:34 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (84.19 KB, patch)
2022-04-01 14:09 PDT, Oriol Brufau 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (84.20 KB, patch)
2022-04-01 14:25 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (108.70 KB, patch)
2022-04-01 18:04 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (109.23 KB, patch)
2022-04-02 08:11 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
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.