Bug 73164 - unicode-bidi should support isolate override and override isolate
: unicode-bidi should support isolate override and override isolate
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 50910
  Show dependency treegraph
 
Reported: 2011-11-27 01:48 PST by
Modified: 2012-03-05 15:09 PST (History)


Attachments
work in progress (14.64 KB, patch)
2011-11-27 02:56 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
test case (1.31 KB, text/html)
2011-11-27 03:02 PST, Aharon (Vladimir) Lanin
no flags Details
fixes the bug (72.56 KB, patch)
2011-11-27 18:58 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Made bidi-override-isolate.html a reftest (27.12 KB, patch)
2011-11-28 11:19 PST, Ryosuke Niwa
eric: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


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

Thanks for the tests!
------- Comment #4 From 2011-11-27 18:58:44 PST -------
Created an attachment (id=116692) [details]
fixes the bug
------- Comment #5 From 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 From 2011-11-27 20:06:45 PST -------
(From update of attachment 116692 [details])
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 From 2011-11-27 22:46:55 PST -------
(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
------- Comment #8 From 2011-11-27 22:48:53 PST -------
(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.
------- Comment #9 From 2011-11-27 23:08:56 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 116692 [details] [details] [details])
> > Attachment 116692 [details] [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 From 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 From 2011-11-28 11:19:57 PST -------
Created an attachment (id=116789) [details]
Made bidi-override-isolate.html a reftest
------- Comment #12 From 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 From 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 From 2011-11-28 12:23:35 PST -------
Also see https://bugzilla.mozilla.org/show_bug.cgi?id=613149.
------- Comment #15 From 2011-11-28 14:01:40 PST -------
Asked on www-style: http://lists.w3.org/Archives/Public/www-style/2011Nov/0707.html
------- Comment #16 From 2011-11-30 13:28:23 PST -------
Any response from www-style?
------- Comment #17 From 2012-03-01 14:26:44 PST -------
(From update of attachment 116789 [details])
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 From 2012-03-01 14:27:28 PST -------
(From update of attachment 116789 [details])
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 From 2012-03-05 15:09:28 PST -------
Committed r109806: <http://trac.webkit.org/changeset/109806>