Summary: | CSS 2.1 failure: bidi-override ignored | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon@Chromium <jon> | ||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bdakin, hyatt, jshin, mitz, playmobil, simon.fraser, vivianz, xji | ||||||||
Priority: | P2 | ||||||||||
Version: | 525.x (Safari 3.1) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
URL: | http://www.w3.org/International/tests/test-rlo-blocks | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 47141, 27806 | ||||||||||
Attachments: |
|
Description
Jon@Chromium
2008-10-07 11:44:17 PDT
*** Bug 45500 has been marked as a duplicate of this bug. *** 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 *** Bug 47214 has been marked as a duplicate of this bug. *** 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. 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.
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 on attachment 86183 [details]
patch w/ layout test
r=me
Committed r81807: <http://trac.webkit.org/changeset/81807> |