Bug 119795 – Propagate writing-mode from the first region to the flow thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Morten Stenshorne]

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/

[Morten Stenshorne]

Created attachment 208716 [details]
Test case

[Morten Stenshorne]

Created attachment 208719 [details]
Patch

[Andrei Bucur]

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]

(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]

Created attachment 208920 [details]
Patch

[Morten Stenshorne]

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]

@hyatt: could you review this, please?

[Darin Adler]

Comment on attachment 208920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208920&action=review

> Source/WebCore/ChangeLog:1
> [phone]  [e-mail]  <[e-mail]>

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]

Created attachment 208951 [details]
Patch

[Darin Adler]

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 on attachment 208951 [details]
Patch

Clearing flags on attachment: 208951

Committed r154221: <http://trac.webkit.org/changeset/154221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.