CSS 2.1 failure: bidi-* tests fail

• Component: Layout and Rendering
• Status: NEW ---

[Simon Fraser (smfr)]

Created attachment 69842 [details]
bidi-002.htm

The following tests fail:

html4/bidi-002	fail
html4/bidi-004	fail
html4/bidi-009	fail

[Aharon (Vladimir) Lanin]

Looks at least superficially very similar to bug 9272.

[Levi Weintraub]

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.

[Yael]

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

[Yael]

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.

[Yael]

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.

[Yael]

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.

[Yael]

Created attachment 89650 [details]
Patch.

Set svn:mime-type on the modified image. Let's hope it applies now.

[Eric Seidel (no email)]

Bleh.  Ugly, but likely necessary.  I would like levi to have a chance to look too.

[Levi Weintraub]

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

[Levi Weintraub]

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.

[Ian 'Hixie' Hickson]

HTML4 is not relevant here. You should not attempt to implement it.

[Yael]

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

[Yael]

Created attachment 90267 [details]
test that is still failing

[Yael]

(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

[Ryosuke Niwa]

What's the status of this bug? Is this bug fixed?

[Eric Seidel (no email)]

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?