Bug 21440 - CSS 2.1 failure: bidi-override ignored
Summary: CSS 2.1 failure: bidi-override ignored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/International/tests...
Keywords:
: 45500 47214 (view as bug list)
Depends on:
Blocks: 27806 47141
  Show dependency treegraph
 
Reported: 2008-10-07 11:44 PDT by Jon@Chromium
Modified: 2011-03-23 14:28 PDT (History)
8 users (show)

See Also:


Attachments
test case (1.17 KB, text/html)
2010-10-11 17:58 PDT, Xiaomei Ji
no flags Details
patch w/ layout test (21.82 KB, patch)
2011-03-17 17:47 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (26.65 KB, patch)
2011-03-18 11:22 PDT, Xiaomei Ji
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon@Chromium 2008-10-07 11:44:17 PDT
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
Comment 1 vivianz 2010-09-09 16:05:54 PDT
*** Bug 45500 has been marked as a duplicate of this bug. ***
Comment 2 Xiaomei Ji 2010-10-11 17:58:57 PDT
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
Comment 3 Xiaomei Ji 2010-10-11 17:59:34 PDT
*** Bug 47214 has been marked as a duplicate of this bug. ***
Comment 4 Xiaomei Ji 2011-03-15 16:56:56 PDT
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.
Comment 5 Xiaomei Ji 2011-03-17 17:47:19 PDT
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.
Comment 6 Xiaomei Ji 2011-03-18 11:22:41 PDT
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.
Comment 7 Dave Hyatt 2011-03-23 11:52:06 PDT
Comment on attachment 86183 [details]
patch w/ layout test

r=me
Comment 8 Xiaomei Ji 2011-03-23 14:28:03 PDT
Committed r81807: <http://trac.webkit.org/changeset/81807>