If you set a writing mode different from the viewport's on some DIV, then put some simple flow-into and flow-from elements inside it, layout becomes incorrect. The spec - http://www.w3.org/TR/css3-regions/#the-flow-into-property - says: "The first region defines the principal writing mode for the entire flow. The writing mode on subsequent regions is ignored." What went wrong? The anonymous element created for the flow thread is a child of RenderView, so it doesn't inherit the right writing mode that is set further down in the tree. This is a back-port of a Blink patch: http://code.google.com/p/chromium/issues/detail?id=257965 https://codereview.chromium.org/18374008/
Created attachment 208716 [details] Test case
Created attachment 208719 [details] Patch
Comment on attachment 208719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208719&action=review Unofficial review. Nice patch with a small observation. > Source/WebCore/rendering/RenderRegion.cpp:295 > + m_flowThread->regionChangedWritingMode(this); Can this be called for invalid regions (those that create cycles)? If so, calling m_regionList.first() in regionChangedWritingMode might hit an assertion for isEmpty(). A test for this case might be useful, with a block element having both flow-into and flow-from set on it.
(In reply to comment #3) > Can this be called for invalid regions (those that create cycles)? If so, calling m_regionList.first() in regionChangedWritingMode might hit an assertion for isEmpty(). A test for this case might be useful, with a block element having both flow-into and flow-from set on it. As far as I understand, a region that tries to flow from the very same thread that it's in will get a NULL m_flowThread. That's how I understand RenderRegion::installFlowThread(), at least. So that scenario seems impossible to me, so unless I'm mistaken, an assertion failure here would suggest that something is horribly wrong, so I'd very much like to keep it and not guard against it. Do you want me to write a layout test for this? What would the pass condition be? No crash? I suppose it would be required to run such a test in debug mode, in order for it to be useful?
Created attachment 208920 [details] Patch
OK, I've added a couple of tests where a region sets a non-default writing-mode and tries to flow from the very same thread that it's inside.
@hyatt: could you review this, please?
Comment on attachment 208920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208920&action=review > Source/WebCore/ChangeLog:1 > +2013-08-14 mstensho@opera.com <mstensho@opera.com> This should have your real name rather having than your email address twice. > Source/WebCore/rendering/RenderNamedFlowThread.cpp:95 > + if (RenderRegion* firstRegion = m_regionList.first()) { > + if (style()->writingMode() != firstRegion->style()->writingMode()) { We normally prefer to use early return rather than nesting the whole function inside an if statement. > Source/WebCore/rendering/RenderNamedFlowThread.cpp:99 > + setStyle(newStyle); This should be: setStyle(newStyle.release());
Created attachment 208951 [details] Patch
Comment on attachment 208951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208951&action=review > Source/WebCore/rendering/RenderNamedFlowThread.h:71 > + virtual void regionChangedWritingMode(RenderRegion*) OVERRIDE; I normally prefer to make overrides like this private since we don’t expect any non-polymorphic calls to this function, but that’s less important in a class already marked FINAL where the call will be a normal function call rather than a virtual dispatch.
Comment on attachment 208951 [details] Patch Clearing flags on attachment: 208951 Committed r154221: <http://trac.webkit.org/changeset/154221>
All reviewed patches have been landed. Closing bug.