Bug 74135

Hi All, I started working on this issue, will submit the patch soon.

Created attachment 135765 [details] Patch to apply containing block available width for region, whenever width is not specified. Hi, As per my analysis, as region is a replaced element its width is calculated in function RenderReplaced::computeReplacedLogicalWidth. In this function if region width is not specifed in style, it will check for intrinsic width. In present case region width is not specified and intrinsicWidth=0. This intrinsicWidth=0 is applied to region. By this reason(regionWidth=0), in RenderBlock::LineBreaker::nextLineBreak function, while checking for available width for every encounter of space(" ") it is getting availableWidth=0, then it is doing line break. This is how it is showing single line content in multiple lines. Whenever region width is not specified , it should take available width in its containing block. Did the same in the patch to resolve issue.(see the patch) Can any one please review this patch.

Comment on attachment 135765 [details] Patch to apply containing block available width for region, whenever width is not specified. Attachment 135765 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12335199 New failing tests: fast/regions/flows-dependency-same-flow.html fast/regions/render-region-renderer.html fast/regions/top-overflow-out-of-second-region.html fast/regions/bottom-overflow-out-of-first-region.html fast/regions/flows-dependency-dynamic-remove.html

Created attachment 135772 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 136684 [details] Patch to apply containing block available width for region, whenever width is not specified. In this patch , the test cases which are failed in earlier patch were placed in test_expectations.txt purposefully, to make the build bots as green in order to get it reviewed by reviewers. With the fix if region width is not specified , containing block's available width is applied. For these test cases also it is doing the same. By this reason these tests are failing. For this should I replace the expected result of these test cases with the actual result(generated by fix) OR should I make the testcases pass(by changing the fix). Can some one please clarify and review the patch.

Comment on attachment 136684 [details] Patch to apply containing block available width for region, whenever width is not specified. View in context: https://bugs.webkit.org/attachment.cgi?id=136684&action=review > LayoutTests/platform/chromium/test_expectations.txt:3846 > +BUGWK74135 : fast/regions/flows-dependency-same-flow.html = FAIL > +BUGWK74135 : fast/regions/render-region-renderer.html = FAIL > +BUGWK74135 : fast/regions/top-overflow-out-of-second-region.html = FAIL > +BUGWK74135 : fast/regions/bottom-overflow-out-of-first-region.html = FAIL > +BUGWK74135 : fast/regions/flows-dependency-dynamic-remove.html = FAIL Why do all these tests fail after this patch is applied?

(In reply to comment #6) > (From update of attachment 136684 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136684&action=review > > > LayoutTests/platform/chromium/test_expectations.txt:3846 > > +BUGWK74135 : fast/regions/flows-dependency-same-flow.html = FAIL > > +BUGWK74135 : fast/regions/render-region-renderer.html = FAIL > > +BUGWK74135 : fast/regions/top-overflow-out-of-second-region.html = FAIL > > +BUGWK74135 : fast/regions/bottom-overflow-out-of-first-region.html = FAIL > > +BUGWK74135 : fast/regions/flows-dependency-dynamic-remove.html = FAIL > > Why do all these tests fail after this patch is applied? 1. fast/regions/flows-dependency-same-flow.html 2. fast/regions/flows-dependency-dynamic-remove.html In these two test cases for regions size is not specified. In the present code for this regions width is applyed as '0'. But by the fix width is applyed as available containing block width. By this reason actual result is different from expected result. i.e, region size is 784x0, instead of 0x0. (see layout-test-result) 3. fast/regions/render-region-renderer.html In this test case for the 1st div region width is not specified. So same as above by the fix , width is applyed as available containing block width. By this reason image diff is there between expected and actual. (see layout-test-results) 4. fast/regions/top-overflow-out-of-second-region.html 5. fast/regions/bottom-overflow-out-of-first-region.html In these test cases for region3 width is not specified. So same as above by the fix , width is applyed as available containing block width. i.e, region size is 786x2, instead of 2x2 (here 2px is for border) (see layout-test-result) And for RenderNamedFlowThread width is taken as max of regions widths.i.e, 784. And for further elements (x,y,w,h) calculations is also done based on this. By this reason actual result is different from expected result. Conclusion: ---------- With the fix, for all these failing test cases whereever region width is not specified, width is applyed as available containing block width. Failure of these test cases is correct behaviour. So it is needed to change expected result (i.e, replacing the expected result with actual result). Can some one please clarify that am I correct?

Comment on attachment 136684 [details] Patch to apply containing block available width for region, whenever width is not specified. View in context: https://bugs.webkit.org/attachment.cgi?id=136684&action=review > Source/WebCore/rendering/RenderReplaced.cpp:375 > + if (!hasIntrinsicWidth && !style()->regionThread().isEmpty()) > + return containingBlock()->availableLogicalWidth(); I'd rather you find a way to put this in RenderRegion instead. Eventually we have to move RenderRegion to be a RenderBlock subclass rather than a RenderReplaced subclass, so if we put stuff in RenderReplaced, we're just going to be confusing things when we make that switchover.

Comment on attachment 136684 [details] Patch to apply containing block available width for region, whenever width is not specified. Also, I thought the plan was for auto width regions to use the intrinsic width of the flow thread... so why are you using the containing block here? I'm pretty confused here. If all you're trying to do here is implement the RenderBlock behavior of filling your container by default, then that will be fixed by moving RenderRegion to be a RenderBlock subclass and should not be fixed by hacking RenderReplaced.

Hi Swapna, As David pointed out, for auto-width for regions we should use the flow thread intrinsic width. Please take a look at http://dev.w3.org/csswg/css3-regions/#rfbc-width-resolution if you want to move further with this patch.

(In reply to comment #9) > (From update of attachment 136684 [details]) Hi Hyatt, At first thank you for reviewing. > > I'm pretty confused here. If all you're trying to do here is implement the RenderBlock behavior of filling your container by default, then that will be fixed by moving RenderRegion to be a RenderBlock subclass and should not be fixed by hacking RenderReplaced. Yes, I also checked it by making RenderRegion as subclass of RenderBlock, instead of RenderReplaced. Then region is no more a replaced element and region width is applied as available containing block width in function RenderBox::computeLogicalWidthInRegion. Let me know whether my understanding is correct or not.

(In reply to comment #9) > (From update of attachment 136684 [details]) > Also, I thought the plan was for auto width regions to use the intrinsic width of the flow thread... so why are you using the containing block here? (In reply to comment #10) > Hi Swapna, > As David pointed out, for auto-width for regions we should use the flow thread intrinsic width. Please take a look at http://dev.w3.org/csswg/css3-regions/#rfbc-width-resolution if you want to move further with this patch. Hi Dave & Mihnea, At first I had a thought of applying intrinsic width of flow thread to the region. But in the present code at first regions layout (RenderRegion::layout)is done, then flow thread layout(RenderFlowThread::layout) is done. And FlowThread width is calculated as max of regions width in function RenderFlowThread::computeLogicalWidth. Even though RenderRegion::layout is getting fired once again, is it correct approach to apply the flow thread width to region(for which width is not specified)? Please clarify. For first patch I am in doubt, so followed other approach for applying width. Can you please suggest me how can I proceed.

Created attachment 151930 [details] Patch

Created attachment 153498 [details] Patch 2 Same code, more tests, now including min/max-width and different writing modes.

Comment on attachment 153498 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=153498&action=review > Source/WebCore/rendering/RenderRegion.cpp:411 > + return m_flowThread ? m_flowThread->minPreferredLogicalWidth() + borderAndPaddingLogicalWidth() : RenderReplaced::minPreferredLogicalWidth(); I think you should use computeBorderBoxLogicalWidth instead of adding borderAndPaddingLogicalWidth here. This would take care of box-sizing: border though it's not 100% clear how box-sizing would work with regions. Let's also add some testing if possible. I would put an early return instead of a ternary operator for readibility: if (m_flowThread) return computeBorderBoxLogicalWidth(m_flowThread->minPreferredLogicalWidth()); return RenderReplaced::minPreferredLogicalWidth(); > Source/WebCore/rendering/RenderRegion.h:103 > + virtual LayoutUnit maxPreferredLogicalWidth() const; Let's annotate every virtual with OVERRIDE > Source/WebCore/rendering/RenderRegion.h:111 > + virtual bool isInlineBlockOrInlineTable() const { return isInline() && treatRegionAsReplaced(); } This function is more and more misnamed but that's orthogonal. > Source/WebCore/rendering/RenderRegion.h:112 > + virtual bool shouldComputeSizeAsReplaced() const { return !treatRegionAsReplaced(); } Do you still need to override this function as RenderBox::shouldComputeSizeAsReplaced should return the right value due to isInlineBlockOrInlineTable above?

Created attachment 162057 [details] Patch 3

(In reply to comment #15) > (From update of attachment 153498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153498&action=review > > > Source/WebCore/rendering/RenderRegion.cpp:411 > > + return m_flowThread ? m_flowThread->minPreferredLogicalWidth() + borderAndPaddingLogicalWidth() : RenderReplaced::minPreferredLogicalWidth(); > > I think you should use computeBorderBoxLogicalWidth instead of adding borderAndPaddingLogicalWidth here. This would take care of box-sizing: border though it's not 100% clear how box-sizing would work with regions. Let's also add some testing if possible. > > I would put an early return instead of a ternary operator for readibility: > > if (m_flowThread) > return computeBorderBoxLogicalWidth(m_flowThread->minPreferredLogicalWidth()); > return RenderReplaced::minPreferredLogicalWidth(); I have followed a similar pattern used in the new flexible box module in computing preferred logical widths. > > > Source/WebCore/rendering/RenderRegion.h:103 > > + virtual LayoutUnit maxPreferredLogicalWidth() const; > > Let's annotate every virtual with OVERRIDE > Done. > > Source/WebCore/rendering/RenderRegion.h:111 > > + virtual bool isInlineBlockOrInlineTable() const { return isInline() && treatRegionAsReplaced(); } > > This function is more and more misnamed but that's orthogonal. > > > Source/WebCore/rendering/RenderRegion.h:112 > > + virtual bool shouldComputeSizeAsReplaced() const { return !treatRegionAsReplaced(); } > > Do you still need to override this function as RenderBox::shouldComputeSizeAsReplaced should return the right value due to isInlineBlockOrInlineTable above? Yes. I want shouldComputeSizeAsReplaced to return true when a region has width != auto. For regions, isInlineBlockOrInlineTable returns true when a region is inline and width == auto. If i would rely on RBox::shouldComputeSizeAsReplaced, i would have this function return true when the region is not inline, without taking width auto into account. I saw a similar pattern with seamless iframes.

Comment on attachment 162057 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=162057&action=review r=me but please do the requested changes. > Source/WebCore/ChangeLog:34 > + (RenderRegion): The individual entries should describe why you needed those changes. > Source/WebCore/rendering/RenderBox.cpp:2554 > + if (!isRenderRegion() || (isRenderRegion() && shouldComputeSizeAsReplaced())) { Techincally this could be simplified to: if (!isRenderRegion() || shouldComputeSizeAsReplaced()) but it would be less readable. > Source/WebCore/rendering/RenderRegion.cpp:503 > + if (m_flowThread) { > I have followed a similar pattern used in the new flexible box module in computing preferred logical widths. Early returns have always been preferred over nested ifs. The fact that some other part of the code doesn't follow that is not a justification. > Source/WebCore/rendering/RenderRegion.cpp:511 > + if (styleToUse->logicalMinWidth().isFixed() && styleToUse->logicalMinWidth().value() > 0) > + minPreferredLogicalWidth = std::max(minPreferredLogicalWidth, computeContentBoxLogicalWidth(styleToUse->logicalMinWidth().value())); > + > + if (styleToUse->logicalMaxWidth().isFixed()) > + minPreferredLogicalWidth = std::min(minPreferredLogicalWidth, computeContentBoxLogicalWidth(styleToUse->logicalMaxWidth().value())); Please add some FIXMEs here as you only handle the <length> case. min-width / max-width can be <percentage>, ... and the code won't handle those cases properly. > Source/WebCore/rendering/RenderRegion.cpp:519 > +LayoutUnit RenderRegion::maxPreferredLogicalWidth() const Same comments as for minPreferredLogicalWidth.

Created attachment 162184 [details] Patch for landing

Comment on attachment 162184 [details] Patch for landing Clearing flags on attachment: 162184 Committed r127596: <http://trac.webkit.org/changeset/127596>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 95928

Looks like the patch introduce a possibility of stack overflow. Please see https://bugs.webkit.org/show_bug.cgi?id=95927 I'm sorry, but I'm going to rollout r127596.

(In reply to comment #23) > Looks like the patch introduce a possibility of stack overflow. Please see https://bugs.webkit.org/show_bug.cgi?id=95927 > > I'm sorry, but I'm going to rollout r127596. I am investigating the cause for this crash. It happens in NRWT chromium-mac, i was not able to reproduce it on NWRT safari-mac.

Created attachment 162528 [details] Patch 4 The crash (stack overflow) was seen in the following situation: * a region with width:auto is attached to a flow thread * because there are some circular dependencies between the regions and the flow threads in the page, the region above is not valid, meaning that it should not receive content from the associated flow thread * the region has the characteristics of using the shrink-to-fit algorithm for computing the width In this situation, when computing the width of the region, the code used the min/maxPreferredWidth from flowThread, which, because of circular dependencies, caused the repeated call of minPreferredLogicalWidth, thus causing the stack overflow. The fix is to test the validity of the region (besides being attached to a flow thread) before attempting to use the min/maxPreferredWidth from the flowThread.

Comment on attachment 162528 [details] Patch 4 r=me, please make sure you get a green EWS or use the cq. Thanks!

Created attachment 163286 [details] Patch for landing

Comment on attachment 163286 [details] Patch for landing Clearing flags on attachment: 163286 Committed r128155: <http://trac.webkit.org/changeset/128155>

All reviewed patches have been landed. Closing bug.