Bug 73164 – unicode-bidi should support isolate override and override isolate

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Ryosuke Niwa]

Right now, WebKit only accepts either -webkit-isolate or override as the value of unicode-bidi. However, per CSS Writing Modes Module level 3, it should also support -webkit-isolate override and override -webkit-isolate.

See http://dev.w3.org/csswg/css3-writing-modes/#unicode-bidi0

[Ryosuke Niwa]

Created attachment 116669 [details]
work in progress

[Aharon (Vladimir) Lanin]

Created attachment 116670 [details]
test case

[Ryosuke Niwa]

(In reply to comment #2)
> Created an attachment (id=116670) [details]
> test case

Thanks for the tests!

[Ryosuke Niwa]

Created attachment 116692 [details]
fixes the bug

[Ryosuke Niwa]

Ideally, I want to land this patch first before landing my patch for the bug 63903 so that I can fix bdi behavior for good in the bug 63903.

[Luke Macpherson]

Comment on attachment 116692 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=116692&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:2602
> +    {

It would be preferable to implement this in CSSStyleApplyProperty.

[WebKit Review Bot]

Comment on attachment 116692 [details]
fixes the bug

Attachment 116692 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10676104

New failing tests:
fast/text/bidi-override-isolate.html

[Ryosuke Niwa]

(In reply to comment #7)
> (From update of attachment 116692 [details])
> Attachment 116692 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10676104
> 
> New failing tests:
> fast/text/bidi-override-isolate.html

We just need a rebaseline for each Chromium platform.

[Aharon (Vladimir) Lanin]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 116692 [details] [details])
> > Attachment 116692 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/10676104
> > 
> > New failing tests:
> > fast/text/bidi-override-isolate.html
> 
> We just need a rebaseline for each Chromium platform.

Can this be done as a ref test (the kind where a screenshot of the test document is compared to screenshot of a reference document, as opposed to a reference image)? It should be very easy to convert the test case I provided to that format.

[Ryosuke Niwa]

(In reply to comment #9)
> Can this be done as a ref test (the kind where a screenshot of the test document is compared to screenshot of a reference document, as opposed to a reference image)? It should be very easy to convert the test case I provided to that format.

That's an excellent idea. Let me do that tomorrow morning.

[Ryosuke Niwa]

Created attachment 116789 [details]
Made bidi-override-isolate.html a reftest

[Eric Seidel (no email)]

I think we should push back against the CSS WG and get them to drop this silly unicode-bidi: isolate override; syntax with some sort of compound values which only work in one case!

Seems they should just have added a 'isolate-override' value to the enum instead.

[Ryosuke Niwa]

(In reply to comment #12)
> I think we should push back against the CSS WG and get them to drop this silly unicode-bidi: isolate override; syntax with some sort of compound values which only work in one case!
> 
> Seems they should just have added a 'isolate-override' value to the enum instead.

I second this.

[Ryosuke Niwa]

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=613149.

[Ryosuke Niwa]

Asked on www-style: http://lists.w3.org/Archives/Public/www-style/2011Nov/0707.html

[Eric Seidel (no email)]

Any response from www-style?

[Eric Seidel (no email)]

Comment on attachment 116789 [details]
Made bidi-override-isolate.html a reftest

View in context: https://bugs.webkit.org/attachment.cgi?id=116789&action=review

I don't really like this spec change, but the implementation looks OK.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:997
> +        {
> +            RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> +            list->append(primitiveValueCache->createIdentifierValue(CSSValueBidiOverride));
> +            list->append(primitiveValueCache->createIdentifierValue(CSSValueWebkitIsolate));
> +            return list;
> +        }

I don' think this is quite the right indent.  I think the { themselves do not get indented.

[Eric Seidel (no email)]

Comment on attachment 116789 [details]
Made bidi-override-isolate.html a reftest

View in context: https://bugs.webkit.org/attachment.cgi?id=116789&action=review

> LayoutTests/fast/text/bidi-override-isolate.html:10
> +<div><span style="direction: rtl; unicode-bidi: bidi-override -webkit-isolate; unicodeo-bidi: bidi-override isolate;">abc</span> 1</div>
> +<div><span style="direction: rtl; unicode-bidi: -webkit-isolate bidi-override; unicodeo-bidi: isolate bidi-override;">abc</span> 1</div>

unicodeo?

[Ryosuke Niwa]

Committed r109806: <http://trac.webkit.org/changeset/109806>