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.
Created attachment 453918 [details] Patch
Created attachment 453921 [details] Patch
Created attachment 454046 [details] Patch
<rdar://problem/90106626>
Created attachment 454667 [details] Patch
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
Created attachment 454668 [details] Patch
Created attachment 455197 [details] Patch
Created attachment 455200 [details] Patch for EWS
> 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.
Created attachment 456400 [details] Patch
Created attachment 456404 [details] Patch
Created attachment 456423 [details] Patch
Created attachment 456458 [details] Patch
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?
(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.
Ok. I guess this stuff with Quads is better cleaned separately anyway.
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].