Bug 73164

Summary: unicode-bidi should support isolate override and override isolate
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: CSSAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, dglazkov, eric, leviw, macpherson, mitz, playmobil, tkent, webkit.review.bot, xji, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 50910    
Attachments:
Description Flags
work in progress
none
test case
none
fixes the bug
none
Made bidi-override-isolate.html a reftest eric: review+

Description Ryosuke Niwa 2011-11-27 01:48:59 PST
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
Comment 1 Ryosuke Niwa 2011-11-27 02:56:17 PST
Created attachment 116669 [details]
work in progress
Comment 2 Aharon (Vladimir) Lanin 2011-11-27 03:02:38 PST
Created attachment 116670 [details]
test case
Comment 3 Ryosuke Niwa 2011-11-27 03:16:33 PST
(In reply to comment #2)
> Created an attachment (id=116670) [details]
> test case

Thanks for the tests!
Comment 4 Ryosuke Niwa 2011-11-27 18:58:44 PST
Created attachment 116692 [details]
fixes the bug
Comment 5 Ryosuke Niwa 2011-11-27 19:22:42 PST
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.
Comment 6 Luke Macpherson 2011-11-27 20:06:45 PST
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.
Comment 7 WebKit Review Bot 2011-11-27 22:46:55 PST
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
Comment 8 Ryosuke Niwa 2011-11-27 22:48:53 PST
(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.
Comment 9 Aharon (Vladimir) Lanin 2011-11-27 23:08:56 PST
(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.
Comment 10 Ryosuke Niwa 2011-11-27 23:12:41 PST
(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.
Comment 11 Ryosuke Niwa 2011-11-28 11:19:57 PST
Created attachment 116789 [details]
Made bidi-override-isolate.html a reftest
Comment 12 Eric Seidel (no email) 2011-11-28 11:50:11 PST
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.
Comment 13 Ryosuke Niwa 2011-11-28 11:53:03 PST
(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.
Comment 14 Ryosuke Niwa 2011-11-28 12:23:35 PST
Also see https://bugzilla.mozilla.org/show_bug.cgi?id=613149.
Comment 15 Ryosuke Niwa 2011-11-28 14:01:40 PST
Asked on www-style: http://lists.w3.org/Archives/Public/www-style/2011Nov/0707.html
Comment 16 Eric Seidel (no email) 2011-11-30 13:28:23 PST
Any response from www-style?
Comment 17 Eric Seidel (no email) 2012-03-01 14:26:44 PST
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.
Comment 18 Eric Seidel (no email) 2012-03-01 14:27:28 PST
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?
Comment 19 Ryosuke Niwa 2012-03-05 15:09:28 PST
Committed r109806: <http://trac.webkit.org/changeset/109806>