Full Text Bug Listing

Said Abou-Hallawa

Reported 2014-12-08 10:31:50 PST

Created attachment 242825 [details] Safari-Rendering Open the following SVG in WebKit: <html> <head> <style> div { height: 100px; width: 100px; } div#intrinsic-size { border-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="100" height="100"><rect width="100" height="100" fill="lime"/></svg>') 0 fill; } div#no-intrinsic-size { border-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" version="1.1"><rect width="100" height="100" fill="lime"/></svg>') 0 fill; } </style> </head> <body> <div id="intrinsic-size"></div> <br> <div id="no-intrinsic-size"></div> </body> </html> Result: Two lime rectangles are drawn but they do not have the same size. The first one is 100 x 100 pixels but the second one is less than that Expected: The two rectangles should have the same size even though the the first SVG image has an intrinsic size and the second SVG image does not have. From the w3c specs for the description of [Image Slicing: the ‘border-image-slice’ property] http://www.w3.org/TR/css3-background/#border-image-area: "If the image must be sized to determine the slices (for example, for SVG images with no intrinsic size), then it is sized using the default sizing algorithm with no specified size and the border image area as the default object size." So it is expected to get the image sized to the size of the object size which is the size of the div object which is 100 X 100.

Said Abou-Hallawa

Comment 1 2014-12-08 10:32:18 PST

Created attachment 242826 [details] FireFox-Rendering

Radar WebKit Bug Importer

Comment 2 2014-12-08 10:33:00 PST

<rdar://problem/19177717>

Said Abou-Hallawa

Comment 3 2015-03-15 19:44:20 PDT

Created attachment 248699 [details] Patch

Said Abou-Hallawa

Comment 4 2015-05-11 11:10:53 PDT

Comment on attachment 248699 [details] Patch working on getting a clearer fix.

Said Abou-Hallawa

Comment 5 2015-05-12 13:35:13 PDT

Created attachment 252978 [details] Patch

Said Abou-Hallawa

Comment 6 2015-05-12 13:37:37 PDT

(In reply to comment #5) > Created attachment 252978 [details] > Patch With this patch the border-image drawing is compatible with FireFox.

Said Abou-Hallawa

Comment 7 2015-05-12 13:50:15 PDT

Created attachment 252981 [details] Test case

Said Abou-Hallawa

Comment 8 2015-05-12 15:52:20 PDT

Created attachment 252994 [details] Patch

Said Abou-Hallawa

Comment 9 2015-05-12 18:28:12 PDT

Created attachment 253002 [details] Test case

Said Abou-Hallawa

Comment 10 2015-05-13 09:06:50 PDT

When the image does have an intrinsic size, any choice for the right or the bottom margin of the image is basically wrong. Drawing the border-image slices depends on knowing the boundaries of the image. The SVG specs does not give a way to know its bounding box. In other words, there is no way to know the rightmost/bottommost drawing point in an SVG. So the only valid point to start drawing a slice from an SVG, which does not have an intrinsic size, is its top-left point, i.e. (0,0).

Dean Jackson

Comment 11 2015-05-14 16:21:23 PDT

Comment on attachment 252994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252994&action=review > Source/WebCore/rendering/RenderBoxModelObject.cpp:924 > +LayoutSize RenderBoxModelObject::calculateImageIntrinsicDimensions(StyleImage* image, const LayoutSize& positioningAreaSize, ScaleByEffectiveZoomOrNot shouldScaleOrNot, bool* hasIntrinsicSize) const I don't really like passing a pointer for this out value. But I guess it is less verbose that a reference and a bool flag. Would a imageHasIntrinsicSizeForIntrinsicDimensions() helper replicate too much of the logic from calculateImageIntrinsicDimensions? > LayoutTests/ChangeLog:14 > + image with no intrinsic size, all the slices has to be gotten from (0, 0) Change "has to be gotten" to "have to be extracted"

Said Abou-Hallawa

Comment 12 2015-05-20 18:08:53 PDT

Created attachment 253483 [details] Patch

Said Abou-Hallawa

Comment 13 2015-05-20 18:38:04 PDT

Created attachment 253488 [details] Patch

Said Abou-Hallawa

Comment 14 2015-05-20 19:30:46 PDT

Created attachment 253497 [details] Patch

Said Abou-Hallawa

Comment 15 2015-05-20 20:10:20 PDT

The new patch deserves another review: -- I had to refactor the nine piece image painting code. The previous patch was injecting the fix without changing the logic much. This makes the sequence of calculation not very obvious. The code was doing the same calculation for intrinsic and non-intrinsic cases. And when the pieces are to be drawn, new values were calculated for the non-intrinsic case. -- I moved the painting code in RenderBoxModelObject::paintNinePieceImage() to a new function named NinePieceImage::paint(). The new code is restructured in a clearer way and from the beginning of this new function, the non-intrinsic case is taken into consideration. -- I needed something similar to LengthBox or LayoutBoxExtent but in float. Since the implementations of LengthBox and LayoutBoxExtent were very similar and I did not want to a add a third one, I defined a new template class called BoxExtent. Since LengthBox has specific constructors for the Length members, I made LengthBox inherits from BoxExtent<Length> and I added these specific constructor to this new class. I made LayoutBoxExtent be a typedef of BoxExtent<LayoutUnit>. The new class which is named FloatBoxExtent is also a typedef of BoxExtent<float>. -- As I mentioned in the ChangeLog that this patch is not agreeing with the css3 w3c specs. But I think this solution gives a more predictable rendering for the non-intrinsic-sized image when it is used as border-image.

Said Abou-Hallawa

Comment 16 2015-05-20 20:18:52 PDT

Created attachment 253500 [details] Patch

Darin Adler

Comment 17 2015-05-26 15:38:35 PDT

Comment on attachment 253500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253500&action=review > Source/WebCore/platform/LengthBox.h:134 > + Vector<T, 4> m_sides; This should probably use std::array instead of Vector.

Said Abou-Hallawa

Comment 18 2015-05-26 19:31:02 PDT

Created attachment 253772 [details] Patch

WebKit Commit Bot

Comment 19 2015-05-26 21:18:21 PDT

Comment on attachment 253772 [details] Patch Clearing flags on attachment: 253772 Committed r184895: <http://trac.webkit.org/changeset/184895>

WebKit Commit Bot

Comment 20 2015-05-26 21:18:27 PDT

All reviewed patches have been landed. Closing bug.

David Vest

Comment 21 2015-06-30 00:06:08 PDT

https://lists.w3.org/Archives/Public/www-style/2015May/0302.html seems to have run out of steam. I closed the corresponding blink bug http://code.google.com/p/chromium/issues/detail?id=492485 due to lack of consensus on what we should change to and why any alternative would be better. I propose WebKit either revert this change for now or suggest an alternative specification text that matches what you want to implement/have implemented. We're not in a particularly good position right now with regards to interoperability.

Darin Adler

Comment 22 2015-06-30 10:12:03 PDT

So you’re saying it’s important to change back to match Blink and not Firefox? Will that make interoperability better?

David Vest

Comment 23 2015-06-30 11:53:40 PDT

Changing back would align with blink and the specification. Current WebKit seems to be something new, not identical to Firefox. Even though this example might just be a bug, it's hard to tell when there is no specified behaviour. See: https://doomdavve.github.io/gists/svg-in-border-image-4.html Screenshot: https://doomdavve.github.io/screenshots/border-image-differences.png

Said Abou-Hallawa

Comment 24 2015-06-30 12:17:42 PDT

Created attachment 255839 [details] Chrome display

Said Abou-Hallawa

Comment 25 2015-06-30 12:20:01 PDT

(In reply to comment #23) > Changing back would align with blink and the specification. Current WebKit > seems to be something new, not identical to Firefox. Even though this > example might just be a bug, it's hard to tell when there is no specified > behaviour. See: > > https://doomdavve.github.io/gists/svg-in-border-image-4.html > > Screenshot: > > https://doomdavve.github.io/screenshots/border-image-differences.png How did you did get the Chrome display you showed in the picture border-image-differences.png. I downloaded Chrome version 43.0.2357.130 (64-bit) and I got the attached picture 'Chrome display'.

Said Abou-Hallawa

Comment 26 2015-06-30 13:32:02 PDT

Created attachment 255844 [details] border-image-slicing

Said Abou-Hallawa

Comment 27 2015-06-30 13:33:00 PDT

(In reply to comment #23) > Changing back would align with blink and the specification. Current WebKit > seems to be something new, not identical to Firefox. Even though this > example might just be a bug, it's hard to tell when there is no specified > behaviour. See: > > https://doomdavve.github.io/gists/svg-in-border-image-4.html > > Screenshot: > > https://doomdavve.github.io/screenshots/border-image-differences.png I am going to explain the difference between the way Chrome and current Safari are displaying the test case: https://doomdavve.github.io/gists/svg-in-border-image-4.html since it has been confusing: Chrome (based on the current display of Chrome as in the picture 'Chrome Display'): -- The size of the SVG is the size of the container element. So it is 200x200 for <div class="test big">, 100x100 for <div class="test medium"> and 50x50 for <div class="test small">. In this case the source and destination rectangles are the same. -- Since the slice is 20% and since the border is also 20% (40px for 'big' and 20px for 'medium' and 10px for 'small'), the top-left corner, the left side and the top side and drawn from the image to the border without scaling. -- Since the SVG drawing is 100x100 <rect x="0" y="0" width="100" height="100" fill="green"/>, the border is drawn as if the SVG is drawn at the top-left point of each div element. For example, when drawing the top side for <div class="test big">, the source rect (40, 0)-(160, 40) to the destination rect (40, 0)-(160, 40) which draws only a green rectangle (40, 0)-(100, 40) and the rest is not drawn. And when drawing the top-right corner the source rect (160, 0)-(200, 40) to the destination rectangle (160, 0)-(200, 40) which draws nothing. Safari: -- It is not necessary to setContainerSizeForRenderer() in this case since the SVG does not have an intrinsic size. So no scaling is done here also. -- We draw all the pieces from the top-left corner of the SVG. So for example when drawing the top side for <div class="test big">, we draw the source rect (0, 0)-(40, 40) to the destination rectangle (0, 0)-(40, 40). When we draw the top side for this dive, we draw the source rect (0, 0)-(120, 40) to the destination rectangle (40, 0)-(160, 40), which leaves (140,0)-(160,40) blank. And when we draw the top-right conner, we draw the source rect (0, 0)-(40, 40) to the destination rectangle (160, 0)-(200, 40). The main difference between the display of Chrome and current Safari is how slice the SVG image when it does not have an intrinsic size. I attached the image border-image-slicing which shows the difference between Chrome and current Safari when slicing the SVG which has no intrinsic size for the big div. For this test case, the border and the slice are the same. When the border and the slice are different, it is really hard to even expect what is going to be drawn in the case of Chrome. As an example change the line "border-image-slice: 20% fill;" in the test case to be "border-image-slice: 30% fill;" or "border-image-slice: 10% fill;" and try to find an explanation for the display you get. Having the slices to start from the top-left corner of the SVG in the case of non-intrinsic size makes things much easier to explain and predict.

David Vest

Comment 28 2015-06-30 13:36:51 PDT

(In reply to comment #25) > (In reply to comment #23) > > Changing back would align with blink and the specification. Current WebKit > > seems to be something new, not identical to Firefox. Even though this > > example might just be a bug, it's hard to tell when there is no specified > > behaviour. See: > > > > https://doomdavve.github.io/gists/svg-in-border-image-4.html > > > > Screenshot: > > > > https://doomdavve.github.io/screenshots/border-image-differences.png > > How did you did get the Chrome display you showed in the picture > border-image-differences.png. I downloaded Chrome version 43.0.2357.130 > (64-bit) and I got the attached picture 'Chrome display'. Yes, I must have changed some parameters since taking that shot. I get the same rendering as you in Chrome, and I incorrectly thought that was what the screenshow was showing.

David Vest

Comment 29 2015-06-30 13:44:53 PDT

Thanks for your explanation! > Safari: > [snip] > Having the slices to start from the top-left corner > of the SVG in the case of non-intrinsic size makes things much easier to > explain and predict. I'm not sure I agree WebKit nightly behaviour is easier to predict (or useful). An SVG with no intrinsic size may still be able to scale to the viewport and be useful as a border-image: http://doomdavve.github.io/gists/svg-in-border-image-5.html#no-intrinsic-relative So there may be cases where such an SVG makes sense to slice. It just depends on how the SVG is written. There are many ways to make a raster bitmap make little sense as a border-image too. On the flip size, when is rendering from top-left (0,0) in each piece more useful?

Said Abou-Hallawa

Comment 30 2015-06-30 15:29:15 PDT

(In reply to comment #29) > Thanks for your explanation! > > > Safari: > > > [snip] > > Having the slices to start from the top-left corner > > of the SVG in the case of non-intrinsic size makes things much easier to > > explain and predict. > > I'm not sure I agree WebKit nightly behaviour is easier to predict (or > useful). An SVG with no intrinsic size may still be able to scale to the > viewport and be useful as a border-image: > > http://doomdavve.github.io/gists/svg-in-border-image-5.html#no-intrinsic- > relative > > So there may be cases where such an SVG makes sense to slice. It just > depends on how the SVG is written. There are many ways to make a raster > bitmap make little sense as a border-image too. On the flip size, when is > rendering from top-left (0,0) in each piece more useful? I attached the screenshot Safari-Chrome-Comparison which compares your last test case in Chrome and current Safari in the case of no-intrisic size. I am not sure why you consider the Chrome behavior acceptable or predictable. And I am wondering why WebKit has to implement this behavior and why you consider it an interoperability issue that WebKit has to conform to. I find it difficult myself to explain the Chrome display in the screenshot. But I know easily how to explain current Safari display. I think we are not discussing two implementation here. I think we should discuss why you prefer Chrome's way to display the image-border in this case. Here is what I think about the image-border with no intrinsic size and why I prefer it the way current Safari is doing it. -- The solution we implement in current Safari is not more useful than what Chrome is doing. As in the attached picture, Chrome is not easily predictable in some cases but Safari implementation is always easy to predict. -- I understand the SVG was designed to be scalable. So setting an intrinsic size in the SVG is not necessary when it is displayed as an image. -- But I think when the SVG is used as an image-border, it has to have an intrinsic size to get a predictable display. The main reason for this requirement is we need the size to slice the image from the four sides and scale these slices into the border widths. If no intrinsic size is specified, we can't guess the desired size correctly all the times since it depends on the drawing in the SVG. -- I think, the message to the user should be: "if you want to have a predictable border, you have to set an intrinsic size for your image; we are not going to guess what the size should be because we can't get it correct for all cases".

Said Abou-Hallawa

Comment 31 2015-06-30 15:29:34 PDT

Created attachment 255866 [details] Safari-Chrome-Comparison

Said Abou-Hallawa

Comment 32 2015-06-30 15:41:10 PDT

(In reply to comment #29) > Thanks for your explanation! > > > Safari: > > > [snip] > > Having the slices to start from the top-left corner > > of the SVG in the case of non-intrinsic size makes things much easier to > > explain and predict. > > I'm not sure I agree WebKit nightly behaviour is easier to predict (or > useful). An SVG with no intrinsic size may still be able to scale to the > viewport and be useful as a border-image: > > http://doomdavve.github.io/gists/svg-in-border-image-5.html#no-intrinsic- > relative > > So there may be cases where such an SVG makes sense to slice. It just > depends on how the SVG is written. There are many ways to make a raster > bitmap make little sense as a border-image too. On the flip size, when is > rendering from top-left (0,0) in each piece more useful? Yes I agree. An SVG with "all" the lengths relative can be displayed as in Chrome display since this SVG will fit nicely in any rectangle. But I think it is really difficult to author such an SVG. How can you express a stoke width in relative units and get the desired results? But if SVG with relative lengths is very common and we agree on that, I think it will be predictable and the implementation in WebKit is very straightforward: check for "all" relative lengths and change an if-statment.

David Vest

Comment 33 2015-07-01 00:56:14 PDT

(In reply to comment #30) > I attached the screenshot Safari-Chrome-Comparison which compares your last > test case in Chrome and current Safari in the case of no-intrisic size. I am > not sure why you consider the Chrome behavior acceptable or predictable. I don't care much for that particular case (no-intrinsic). I just want implementations to match each other and the spec. I don't care how broken the rendering looks in that (nonsensical) case. > And I am wondering why WebKit has to implement this behavior Because it is in the spec. Otherwise we must change the spec. If there is a plan to change the spec and relevant people agree with it and spec text is written, I'm happy to help change Blink to match. > and why you consider it an interoperability issue that WebKit has to conform to. The interoperability issue is that WebKit has now changed to a behavior not matched by any other browser (AFAICT) or the spec. Blink, WebKit and IE (as far as my testing goes) all had the same behavior as the spec. So it feels like we're moving in the wrong direction. (Also, I assume you've read the thread starting with dino at: https://lists.w3.org/Archives/Public/www-style/2015May/0302.html )

Said Abou-Hallawa

Comment 34 2015-07-06 15:14:09 PDT

(In reply to comment #33) > (In reply to comment #30) > > I attached the screenshot Safari-Chrome-Comparison which compares your last > > test case in Chrome and current Safari in the case of no-intrisic size. I am > > not sure why you consider the Chrome behavior acceptable or predictable. > > I don't care much for that particular case (no-intrinsic). I just want > implementations to match each other and the spec. I don't care how broken > the rendering looks in that (nonsensical) case. > Yes the rendering looks broken in that case but this what the specs says to draw it. This particular bug is about fixing the broken rendering for this (nonsensical) case. > > And I am wondering why WebKit has to implement this behavior > > Because it is in the spec. Otherwise we must change the spec. If there is a > plan to change the spec and relevant people agree with it and spec text is > written, I'm happy to help change Blink to match. > Okay I agree with you but I am still new to the WebKit and the w3c communities and I do not know how to move things forward especially with things that do not conform to the specs. If you can help out, this will be great. > > and why you consider it an interoperability issue that WebKit has to conform to. > > The interoperability issue is that WebKit has now changed to a behavior not > matched by any other browser (AFAICT) or the spec. Blink, WebKit and IE (as > far as my testing goes) all had the same behavior as the spec. So it feels > like we're moving in the wrong direction. > > (Also, I assume you've read the thread starting with dino at: > > https://lists.w3.org/Archives/Public/www-style/2015May/0302.html > ) Yes I read the thread and maybe it was mistake not to get involved on time. Do you think the thread can be revived? Do you think email is the right way to discuss this subtle issue? Is not it better to do it over the phone so we can get an agreement faster?

David Vest

Comment 35 2015-08-10 02:01:14 PDT

(/me back from vacation. Sorry for the silence.) Brad added https://lists.w3.org/Archives/Public/www-style/2015Jul/0279.html while I was gone. Getting him and other interested parties on www-style onboard with your change would be a good first step, I think. My preference is to handle this through e-mail and on the proper lists using demos and snippets as we've done so far. I would be far less useful over phone or video.

Said Abou-Hallawa

Comment 36 2015-08-11 08:56:24 PDT

The issue will be discussed in the next w3 csswg meeting: https://wiki.csswg.org/planning/paris-2015

Attachments
Safari-Rendering (15.75 KB, image/png) 2014-12-08 10:31 PST, Said Abou-Hallawa	no flags	Details
FireFox-Rendering (45.96 KB, image/png) 2014-12-08 10:32 PST, Said Abou-Hallawa	no flags	Details
Patch (13.46 KB, patch) 2015-03-15 19:44 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (20.84 KB, patch) 2015-05-12 13:35 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Test case (1.73 KB, text/html) 2015-05-12 13:50 PDT, Said Abou-Hallawa	no flags	Details
Patch (21.20 KB, patch) 2015-05-12 15:52 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Test case (2.22 KB, text/html) 2015-05-12 18:28 PDT, Said Abou-Hallawa	no flags	Details
Patch (106.79 KB, patch) 2015-05-20 18:08 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (106.88 KB, patch) 2015-05-20 18:38 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (106.89 KB, patch) 2015-05-20 19:30 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (106.92 KB, patch) 2015-05-20 20:18 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (106.88 KB, patch) 2015-05-26 19:31 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Chrome display (38.19 KB, image/png) 2015-06-30 12:17 PDT, Said Abou-Hallawa	no flags	Details
border-image-slicing (50.69 KB, image/png) 2015-06-30 13:32 PDT, Said Abou-Hallawa	no flags	Details
Safari-Chrome-Comparison (74.44 KB, image/png) 2015-06-30 15:29 PDT, Said Abou-Hallawa	no flags	Details
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Bug 139405