Bug 47210 - CSS 2.1 failure: bidi-* tests fail
Summary: CSS 2.1 failure: bidi-* tests fail
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 47141
  Show dependency treegraph
 
Reported: 2010-10-05 14:23 PDT by Simon Fraser (smfr)
Modified: 2012-01-30 15:06 PST (History)
10 users (show)

See Also:


Attachments
bidi-002.htm (1.37 KB, text/html)
2010-10-05 14:23 PDT, Simon Fraser (smfr)
no flags Details
Patch. (146.42 KB, patch)
2011-04-14 14:12 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (130.34 KB, patch)
2011-04-14 14:39 PDT, Yael
eric: review-
Details | Formatted Diff | Diff
test that is still failing (2.01 KB, text/html)
2011-04-19 15:39 PDT, Yael
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-10-05 14:23:46 PDT
Created attachment 69842 [details]
bidi-002.htm

The following tests fail:

html4/bidi-002	fail
html4/bidi-004	fail
html4/bidi-009	fail
Comment 1 Aharon (Vladimir) Lanin 2011-01-13 02:26:25 PST
Looks at least superficially very similar to bug 9272.
Comment 2 Levi Weintraub 2011-02-04 13:56:57 PST
It seems to me that the right border isn't drawn in the BiDi case because the endObject isn't passed in (eventually to be used in InlineFlowBox::determineSpacingForFlowBoxes). The value of end in layoutInlineChildren doesn't seem correct, but I haven't managed to grok the code there well enough to be sure.

It's also worth noting that hit testing in the "GGGHHHIII" part of the second half of the test is broken. You cannot start or end a selection in it by mouse.
Comment 3 Yael 2011-04-10 12:35:41 PDT
(In reply to comment #1)
> Looks at least superficially very similar to bug 9272.

I have a fix for the border issue. It is attached to https://bugs.webkit.org/show_bug.cgi?id=57459.
However, bidi-004.htm is still failing because we don't place some of the text runs in the right order.
Comment 4 Yael 2011-04-14 14:08:06 PDT
After https://bugs.webkit.org/show_bug.cgi?id=23124 was fixed, <br> would reset directionality only if it is set by a unicode character. In order to fix this bug, and also to be compatible with IE, <br> should not reset the directionality even if set by unicode character, if the document is in strict mode. IE allows <br> to reset directionality in quirks mode. I tested both IE8 and IE9.
Firefox does not seem to care about strict mode, and never resets the directionality.
The patch I am about to upload allows reseting the directionality only when in quirks mode.
Comment 5 Yael 2011-04-14 14:12:59 PDT
Created attachment 89641 [details]
Patch.

When a document is in strict mode, a <br> inside a paragraph should not reset the directionality even if it was set by a unicode character.
When a document is in quirks mode, <br> inside a paragraph should reset the directionality.
This matches the behavior of IE and allows us to pass the css tests attached to this bug.
Comment 6 Yael 2011-04-14 14:16:30 PDT
Sorry that the modified images look like garbage. I suppose svn is trying to diff them instead of replace the old image with the new one.
Comment 7 Yael 2011-04-14 14:39:00 PDT
Created attachment 89650 [details]
Patch.

Set svn:mime-type on the modified image. Let's hope it applies now.
Comment 8 Eric Seidel 2011-04-14 14:49:22 PDT
Bleh.  Ugly, but likely necessary.  I would like levi to have a chance to look too.
Comment 9 Levi Weintraub 2011-04-19 10:51:46 PDT
(In reply to comment #5)
> When a document is in strict mode, a <br> inside a paragraph should not reset the directionality even if it was set by a unicode character.
> When a document is in quirks mode, <br> inside a paragraph should reset the directionality.
> This matches the behavior of IE and allows us to pass the css tests attached to this bug.

I was afraid we'd get here...

This isn't the whole story I'm afraid. In its current state, the HTML 5 specification states the following:

"A br element should separate paragraphs for the purposes of the Unicode bidirectional algorithm. This requirement may be implemented indirectly through the style layer. For example, an HTML+CSS user agent could implement these requirements by implementing the CSS 'unicode-bidi' property. [BIDI] [CSS]"

Since this differs from the HTML4 spec, the correct behavior is unclear. The best option could perhaps be to use the actual requested version in the doctype to determine the correct behavior -- treating br's as a paragraph separator for quirks mode and html5 strict mode, and as a line separator in strict modes <5.
Comment 10 Levi Weintraub 2011-04-19 10:57:28 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=89650&action=review

> Source/WebCore/platform/text/BidiResolver.h:478
> +void BidiResolver<Iterator, Run>::createBidiRunsForLine(const Iterator& end, VisualDirectionOverride override, bool hardLineBreak, bool inQuirksMode)

Since this takes inQuirksMode as a parameter, we don't need to track quirksMode in the BidiContext at all; QuirksMode applies to the document as a whole, and BidiContext is a stack of contexts all within that Document's scope. This entire patch could be simplified to just this function's change of signature and an if that avoids copying the contexts for strict mode.
Comment 11 Ian 'Hixie' Hickson 2011-04-19 15:07:05 PDT
HTML4 is not relevant here. You should not attempt to implement it.
Comment 12 Yael 2011-04-19 15:30:00 PDT
(In reply to comment #11)
> HTML4 is not relevant here. You should not attempt to implement it.

So are you suggesting we should not fix these tests?
Comment 13 Yael 2011-04-19 15:39:55 PDT
Created attachment 90267 [details]
test that is still failing
Comment 14 Yael 2011-04-20 11:18:22 PDT
(In reply to comment #13)
> Created an attachment (id=90267) [details]
> test that is still failing

Per discussion on IRC, the test is not valid. A replacement test was created in http://www.hixie.ch/tests/adhoc/css/box/inline/bidi/012.html
Comment 15 Ryosuke Niwa 2011-12-01 15:14:42 PST
What's the status of this bug? Is this bug fixed?
Comment 16 Eric Seidel 2012-01-30 15:06:07 PST
Comment on attachment 89650 [details]
Patch.

Why would we store this flag on each BidiContext?  You have a BidiContext per embedding level, yet you only have quirks vs. not per document.  If anything we would set this flag on the BidiResolver, no?