Bug 21440 – CSS 2.1 failure: bidi-override ignored

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

[Jon@Chromium]

Steps:
1. Go to http://www.w3.org/International/tests/test-rlo-blocks
2. Observe the "bidi-override" section

Result:
The text direction in the first line and third line is still from left to right

Expected:
The text direction should be from right to left since "unicode-bidi:
bidi-override" property specified

Blocking Chromium issue http://code.google.com/p/chromium/issues/detail?id=3157

*** Bug 45500 has been marked as a duplicate of this bug. ***

[Xiaomei Ji]

Created attachment 70504 [details]
test case

the W3C test suite changed, and the original test URL is no longer available.
combine the test case from W3C test suite and bug 47214

[Xiaomei Ji]

*** Bug 47214 has been marked as a duplicate of this bug. ***

[Xiaomei Ji]

From CSS spec on unicode-bidi 
http://www.w3.org/TR/CSS21/visuren.html#propdef-unicode-bidi:
"bidi-override:
For inline-level elements this creates an override. For block container elements this creates an override for inline-level descendants not within another block-level, table-cell, table-caption, or inline-block element. "

Given the example <div style="direction:rtl; unicode-bidi:bidi-override">abc <div>def</div></div>. The (rtl unicode bidi override) style is not populated to the text "abc" due to such non-inherited style is lost when creating anonymous block (for "abc") in RenderBlock::createAnonymousBlock().

The following change seems fix the issue:
Index: Source/WebCore/rendering/RenderBlock.cpp
===================================================================
--- Source/WebCore/rendering/RenderBlock.cpp	(revision 81184)
+++ Source/WebCore/rendering/RenderBlock.cpp	(working copy)
@@ -241,6 +241,7 @@
         if (child->isAnonymousBlock()) {
             RefPtr<RenderStyle> newStyle = RenderStyle::create();
             newStyle->inheritFrom(style());
+newStyle->setUnicodeBidi(style()->unicodeBidi());
             if (style()->specifiesColumns()) {
                 if (child->style()->specifiesColumns())
                     newStyle->inheritColumnPropertiesFrom(style());
@@ -5848,6 +5849,7 @@
 {
     RefPtr<RenderStyle> newStyle = RenderStyle::create();
     newStyle->inheritFrom(style());
+newStyle->setUnicodeBidi(style()->unicodeBidi());
 
     RenderBlock* newBox = 0;
     if (isFlexibleBox) {


The first change is inside RenderBlock::styleDidChange(), and it renders the page correctly when reload (recalc style).

The 2nd change is inside RenderBlock::createAnonymousBlock(), and it renders the page correctly when page first load.

But I am not sure whether these are the correct places to change, and whether we need to change other places (such as RenderRubyRun::createRubyBase() and RenderInline::addChildIgnoringContinuation()) where anonymous blocks are created as well (if so, some codes might need cleanup to call RenderBlock::createAnonymousBlock() instead of creating anonymous block in place).

Looks like we created several types of anonymous blocks in the code (display type is BOX, BLOCK, INLINE_BLOCK), what are the difference among them? Which should inherit unicode-bidi, which should not? I read http://www.w3.org/TR/CSS21/visuren.html#anonymous-block-level and http://www.w3.org/TR/CSS21/visuren.html#anonymous, but did not get much idea.

[Xiaomei Ji]

Created attachment 86124 [details]
patch w/ layout test

I tried to inherit the column properties if he parent style specified it inside RenderStyle::createAnonymousStyle().
The extra code changes are:


Index: RenderBlock.cpp
===================================================================
--- RenderBlock.cpp     (revision 81184)
+++ RenderBlock.cpp     (working copy)
@@ -239,14 +239,9 @@
     // FIXME: We could save this call when the change only affected non-inherited properties
     for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
         if (child->isAnonymousBlock()) {
-            RefPtr<RenderStyle> newStyle = RenderStyle::create();
-            newStyle->inheritFrom(style());
-            if (style()->specifiesColumns()) {
-                if (child->style()->specifiesColumns())
-                    newStyle->inheritColumnPropertiesFrom(style());
-                if (child->style()->columnSpan())
-                    newStyle->setColumnSpan(true);
-            }
+            RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(style());
+            if (style()->specifiesColumns() && child->style()->columnSpan())
+                newStyle->setColumnSpan(true);
             newStyle->setDisplay(BLOCK);
             child->setStyle(newStyle.release());
         }
@@ -5873,9 +5866,7 @@

 RenderBlock* RenderBlock::createAnonymousColumnsBlock() const
 {
-    RefPtr<RenderStyle> newStyle = RenderStyle::create();
-    newStyle->inheritFrom(style());
-    newStyle->inheritColumnPropertiesFrom(style());
+    RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(style());
     newStyle->setDisplay(BLOCK);

     RenderBlock* newBox = new (renderArena()) RenderBlock(document() /* anonymous box */);
Index: Source/WebCore/rendering/style/RenderStyle.cpp
===================================================================
--- Source/WebCore/rendering/style/RenderStyle.cpp      (revision 81184)
+++ Source/WebCore/rendering/style/RenderStyle.cpp      (working copy)
@@ -56,6 +56,16 @@
     return adoptRef(new RenderStyle(true));
 }

+PassRefPtr<RenderStyle> RenderStyle::createAnonymousStyle(const RenderStyle* parentStyle)
+{
+    RefPtr<RenderStyle> newStyle = RenderStyle::create();
+    newStyle->inheritFrom(parentStyle);
+    newStyle->inheritUnicodeBidiFrom(parentStyle);
+    if (parentStyle->specifiesColumns())
+        newStyle->inheritColumnPropertiesFrom(parentStyle);
+    return newStyle;
+}
+


But that breaks 28 Layout tests under fast/multicol.

I am not sure whether we could inherit column properties by just check whether parent specifies it.
The 1st original caller of  inheritColumnPropertiesFrom() inside RenderBlock::styleDidChange() checks both parent and child specifies it.
I do not know the code enough to add it in.

[Xiaomei Ji]

Created attachment 86183 [details]
patch w/ layout test

add tests to cover anonymous block is not the first block under parent.
inherit the column properties in comment #5 still applies.

[Dave Hyatt]

Comment on attachment 86183 [details]
patch w/ layout test

r=me

[Xiaomei Ji]

Committed r81807: <http://trac.webkit.org/changeset/81807>