Bug 119795

Summary: Propagate writing-mode from the first region to the flow thread
Product: WebKit Reporter: Morten Stenshorne <mstensho>
Component: Layout and RenderingAssignee: Morten Stenshorne <mstensho>
Status: RESOLVED FIXED    
Severity: Normal CC: abucur, commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, WebkitBugTracker
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Test case
none
Patch
none
Patch
none
Patch none

Morten Stenshorne
Reported 2013-08-14 05:24:20 PDT
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/
Attachments
Test case (656 bytes, text/html)
2013-08-14 05:24 PDT, Morten Stenshorne 		no flags
Details
Patch (48.53 KB, patch)
2013-08-14 05:42 PDT, Morten Stenshorne 		no flags
DetailsFormatted DiffDiff
Patch (52.17 KB, patch)
2013-08-16 06:08 PDT, Morten Stenshorne 		no flags
DetailsFormatted DiffDiff
Patch (52.58 KB, patch)
2013-08-16 13:32 PDT, Morten Stenshorne 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Morten Stenshorne
Comment 1 2013-08-14 05:24:42 PDT
Created attachment 208716 [details] Test case
Morten Stenshorne
Comment 2 2013-08-14 05:42:04 PDT
Created attachment 208719 [details] Patch
Andrei Bucur
Comment 3 2013-08-14 08:54:01 PDT
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.
Morten Stenshorne
Comment 4 2013-08-15 12:23:49 PDT
(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?
Morten Stenshorne
Comment 5 2013-08-16 06:08:46 PDT
Created attachment 208920 [details] Patch
Morten Stenshorne
Comment 6 2013-08-16 06:10:21 PDT
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.
Morten Stenshorne
Comment 7 2013-08-16 06:11:27 PDT
@hyatt: could you review this, please?
Darin Adler
Comment 8 2013-08-16 12:12:11 PDT
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());
Morten Stenshorne
Comment 9 2013-08-16 13:32:12 PDT
Created attachment 208951 [details] Patch
Darin Adler
Comment 10 2013-08-17 05:06:38 PDT
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.
WebKit Commit Bot
Comment 11 2013-08-17 05:30:31 PDT
Comment on attachment 208951 [details] Patch Clearing flags on attachment: 208951 Committed r154221: <http://trac.webkit.org/changeset/154221>
WebKit Commit Bot
Comment 12 2013-08-17 05:30:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.