Summary: | unicode-bidi should support isolate override and override isolate | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | CSS | Assignee: | 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
Ryosuke Niwa
2011-11-27 01:48:59 PST
Created attachment 116669 [details]
work in progress
Created attachment 116670 [details]
test case
(In reply to comment #2) > Created an attachment (id=116670) [details] > test case Thanks for the tests! Created attachment 116692 [details]
fixes the bug
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 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 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 (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. (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. (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. Created attachment 116789 [details]
Made bidi-override-isolate.html a reftest
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. (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. Asked on www-style: http://lists.w3.org/Archives/Public/www-style/2011Nov/0707.html Any response from www-style? 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 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? Committed r109806: <http://trac.webkit.org/changeset/109806> |