Bug 114829

Adding Bem. The problem appears to be that percentages are not correctly resolved in this case. If the shape-outside uses px values, or is not present, the layout happens correctly. Bem, can you take a look at this when you get the chance?

Added to my list. Also created a chromium bug, since we're going to want this in Blink as well. That Chromium issue is: https://code.google.com/p/chromium/issues/detail?id=233339

Created attachment 204175 [details] Simplified test case that doesn't use a delay at all. It looks like it doesn't need to be a delay, it just has to be changed after the initial layout pass.

Hey Zoltan, any chance that you have an idea of what is causing this? I figured that since you've been immersed in float + regions land, you might have a quick idea as to what is going on here. I was hoping that this would be fixed after Bear fixed dynamic setting of shape-outside, but it still seems to be broken, unfortunately.

Created attachment 215937 [details] Patch

Comment on attachment 215937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215937&action=review > Source/WebCore/ChangeLog:6 > + We set the size of the shape for shape-outside in RenderBlockFlow::insertFloatingObject. In that function the floating container's height is not always You have two very long sentences in this description, which makes it very hard to read and understand. Can you try breaking it up into more sentences to make it easier to understand what you are describing? Also, mostly a nit, but can you wrap the description to 80 columns? It's also hard to read these really long lines. > Source/WebCore/ChangeLog:9 > + logic to RenderBlockFlow::insertFloatingObject function, after it sets the height for the float, this way we're going to save some unneccessary layout passes I think you mean RenderBlockFlow::positionNewFloats here. > Source/WebCore/ChangeLog:10 > + with not yet defined heights and fix a layout error which came from a zero height layout pass with ellipses. Is this specific to ellipses, or any shape-outside? If it is specific to ellipses, can you explain how? It isn't clear to me. > LayoutTests/ChangeLog:10 > + with not yet defined heights and fix a layout error which came from a zero height layout pass with ellipses. Shouldn't you be describing what the tests are doing here? I don't see the value in repeating the description from the ChangeLog for the C++ change. Also, line wrapping and less run-on sentences would be good, as I mentioned earlier. :-) > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass-expected.html:10 > + -webkit-shape-outside: ellipse(50px, 50px, 50%, 50%); ideally, you shouldn't be using shape-outside in the ref test. If this doesn't require an ellipse, it would be better to do it with a rectangle so that it's really easy to create a simple ref. If it does require an ellipse, might be worth taking a look at LayoutTests/fast/shapes/resources/rounded-rectangle.js and the rounded rectangle/ellipse/circle tests to see about testing without using shape-outside. > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass-expected.html:11 > + -webkit-clip-path: ellipse(50%, 50%, 50%, 50%); This same effect can also be achieved by the following: border-radius: 50%; overflow: hidden; Which I'm starting to like better than using clip-path, since it is even more independent of the shapes code, and arguably simpler. I don't feel particularly strongly about it, though, so if you really like sticking with clip-path, go ahead. > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass.html:10 > + -webkit-shape-outside: ellipse(50px, 50px, 50%, 50%); See my previous comment about the use of ellipse. > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass.html:11 > + -webkit-clip-path: ellipse(50%, 50%, 50%, 50%); See my previous comment about clip-path.

Created attachment 215948 [details] Patch

(In reply to comment #6) > (From update of attachment 215937 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215937&action=review > > > Source/WebCore/ChangeLog:6 > > + We set the size of the shape for shape-outside in RenderBlockFlow::insertFloatingObject. In that function the floating container's height is not always > > You have two very long sentences in this description, which makes it very hard to read and understand. Can you try breaking it up into more sentences to make it easier to understand what you are describing? Fixed. > Also, mostly a nit, but can you wrap the description to 80 columns? It's also hard to read these really long lines. I reduced the columns size a bit. > > Source/WebCore/ChangeLog:9 > > + logic to RenderBlockFlow::insertFloatingObject function, after it sets the height for the float, this way we're going to save some unneccessary layout passes > > I think you mean RenderBlockFlow::positionNewFloats here. Copy/paste, sorry! I fixed. > > Source/WebCore/ChangeLog:10 > > + with not yet defined heights and fix a layout error which came from a zero height layout pass with ellipses. > > Is this specific to ellipses, or any shape-outside? If it is specific to ellipses, can you explain how? It isn't clear to me. The problem showed up with ellipses. I explained in the changelog. > > LayoutTests/ChangeLog:10 > > + with not yet defined heights and fix a layout error which came from a zero height layout pass with ellipses. > > Shouldn't you be describing what the tests are doing here? I don't see the value in repeating the description from the ChangeLog for the C++ change. Also, line wrapping and less run-on sentences would be good, as I mentioned earlier. :-) Historically, we needed include the same content. I removed it, it's unnecessary. > > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass-expected.html:10 > > + -webkit-shape-outside: ellipse(50px, 50px, 50%, 50%); > > ideally, you shouldn't be using shape-outside in the ref test. If this doesn't require an ellipse, it would be better to do it with a rectangle so that it's really easy to create a simple ref. The test tests the relayout behavior, so I don't see why it's ideal to not use it in the ref test. I also don't see why would a rectangle test better then and an ellipse. The problem showed up only for ellipses because of the incorrect radius computation. > If it does require an ellipse, might be worth taking a look at LayoutTests/fast/shapes/resources/rounded-rectangle.js and the rounded rectangle/ellipse/circle tests to see about testing without using shape-outside. > > > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass-expected.html:11 > > + -webkit-clip-path: ellipse(50%, 50%, 50%, 50%); > > This same effect can also be achieved by the following: > > border-radius: 50%; > overflow: hidden; > > Which I'm starting to like better than using clip-path, since it is even more independent of the shapes code, and arguably simpler. I don't feel particularly strongly about it, though, so if you really like sticking with clip-path, go ahead. I modified to use the border-radius and overflow, however I thin the clip-path is more straightforward (1 property over 2), and this thing is just helps to visualize the test. It has nothing to do with testing the behavior itself. Thanks for the review!

Comment on attachment 215948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215948&action=review > Source/WebCore/ChangeLog:14 > + with JavaScript into a region flow. The contet has been layed out based on the 0 border radius, and relayout hasn't been s/contet/content > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass-expected.html:12 > + overflow: hidden; I thought you said you were going to stick with the clip-path? Either way is fine with me. Also, I was mentioning that if you were going to create a ref test without using a shape-outside, then using a rectangle would be easier than using an ellipse, since emulating a rectangle is really easy (as opposed to emulating an ellipse, which involves a whole bunch of JS). Anyways, you are correct that it is still a valid test using the shape-outside. It is just my preference to have my refs to be as simple as possible (thus using rectangles over more complex shapes if possible, etc), because I've found that makes it easier to debug and understand what the test is actually testing. Note that I'm just explaining my thinking; I'm also fine with you keeping the shape-outside as you have, since it is indeed a valid test as is.

Created attachment 215978 [details] Incorporating Bem's review

(In reply to comment #9) > > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-layout-after-initial-layout-pass-expected.html:12 > > + overflow: hidden; > > I thought you said you were going to stick with the clip-path? Either way is fine with me. > > Also, I was mentioning that if you were going to create a ref test without using a shape-outside, then using a rectangle would be easier than using an ellipse, since emulating a rectangle is really easy (as opposed to emulating an ellipse, which involves a whole bunch of JS). > > Anyways, you are correct that it is still a valid test using the shape-outside. It is just my preference to have my refs to be as simple as possible (thus using rectangles over more complex shapes if possible, etc), because I've found that makes it easier to debug and understand what the test is actually testing. Note that I'm just explaining my thinking; I'm also fine with you keeping the shape-outside as you have, since it is indeed a valid test as is. Fair enough! Good point, you convinced me. I updated the test to use a simple rectangle. The expected doesn't use shape-outside anymore. The patch now perfectly incorporate with your review! Thanks for looking at it.

Comment on attachment 215978 [details] Incorporating Bem's review r=me

Comment on attachment 215978 [details] Incorporating Bem's review Clearing flags on attachment: 215978 Committed r158630: <http://trac.webkit.org/changeset/158630>

All reviewed patches have been landed. Closing bug.