RESOLVED FIXED 63903
dir=auto should imply unicode-bidi:isolate by default
https://bugs.webkit.org/show_bug.cgi?id=63903
Summary dir=auto should imply unicode-bidi:isolate by default
Aharon (Vladimir) Lanin
Reported 2011-07-04 06:26:41 PDT
Per http://dev.w3.org/html5/spec/Overview.html#bidirectional-text, the default style sheet should say, among other things: bdi, output, [dir=auto] { unicode-bidi: isolate; } /* case-insensitive */ bdo, bdo[dir] { unicode-bidi: bidi-override; } bdo[dir=auto] { unicode-bidi: bidi-override isolate; } /* case-insensitive */ I.e. dir=auto implies unicode-bidi:isolate by default. Other dir values, e.g. dir=ltr, should continue to imply unicode-bidi:embed.
Attachments
work in progress (12.49 KB, patch)
2011-11-23 19:38 PST, Ryosuke Niwa
no flags
A test case for the default unicode-bidi value, covering https://bugs.webkit.org/show_bug.cgi?id=70373, https://bugs.webkit.org/show_bug.cgi?id=63903, and https://bugs.webkit.org/show_bug.cgi?id=71188 (2.81 KB, text/html)
2011-11-23 21:20 PST, Aharon (Vladimir) Lanin
no flags
Ready for review but can't land until the bug 73116 is fixed (14.94 KB, patch)
2011-11-25 00:12 PST, Ryosuke Niwa
no flags
Fixed per Aharon's comment (15.38 KB, patch)
2011-11-27 00:26 PST, Ryosuke Niwa
mitz: review+
Aharon (Vladimir) Lanin
Comment 1 2011-09-25 01:25:20 PDT
This is very low-hanging fruit; Eric, Yael, does one of you want to take it on? Of course, currently the new property value is -webkit-isolate, so I guess dir=auto should set unicode-bidi to that, until the rename. Please note that currently (Chrome 16.0.891.0), <bdi dir=auto> has the effect of turning on auto-direction, but turning *off* isolation (unicode-bidi becomes embed), which is really a shame.
Aharon (Vladimir) Lanin
Comment 2 2011-09-25 01:29:26 PDT
Please note related bug https://bugs.webkit.org/show_bug.cgi?id=68773 (BDI element should have dir=auto by default)
Eric Seidel (no email)
Comment 3 2011-09-29 11:45:48 PDT
I suspect this may be as simple as adding the CSS from the HTML5 spec into html.css in WebCore: http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#bidirectional-text http://trac.webkit.org/browser/trunk/Source/WebCore/css/html.css The important piece is of course the LayoutTests which get landed with any change.
Ryosuke Niwa
Comment 4 2011-11-23 17:25:51 PST
(In reply to comment #3) > I suspect this may be as simple as adding the CSS from the HTML5 spec into html.css in WebCore: > http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#bidirectional-text > http://trac.webkit.org/browser/trunk/Source/WebCore/css/html.css > > The important piece is of course the LayoutTests which get landed with any change. I could get rid of all C++ code in HTMLElement.cpp and add html5 spec's style rules but some of the rules might be expensive so I'll just modify the existing code to match the spec.
Ryosuke Niwa
Comment 5 2011-11-23 19:38:41 PST
Created attachment 116470 [details] work in progress
Ryosuke Niwa
Comment 6 2011-11-23 19:40:44 PST
I'm hitting some obscure CSS selector bug here. bdi tests fail only if we test span. e.g. if you comment out all tests for span, then bdi tests magically pass :( It appears that refactoring code around also changes the output of the test. Not sure what's causing this flakiness. If anything this is a pretty serious bug.
Aharon (Vladimir) Lanin
Comment 7 2011-11-23 21:18:18 PST
Hi Ryosuke, I am not sure if it makes sense to fix this bug separately from https://bugs.webkit.org/show_bug.cgi?id=70373 and https://bugs.webkit.org/show_bug.cgi?id=71188, since all three are a result of the default unicode-bidi value being calculated in various places in C++ code instead of the default stylesheet. I am attaching a test case for all three bugs.
Aharon (Vladimir) Lanin
Comment 8 2011-11-23 21:20:07 PST
Ryosuke Niwa
Comment 9 2011-11-23 21:33:47 PST
(In reply to comment #7) > Hi Ryosuke, > > I am not sure if it makes sense to fix this bug separately from https://bugs.webkit.org/show_bug.cgi?id=70373 and https://bugs.webkit.org/show_bug.cgi?id=71188, since all three are a result of the default unicode-bidi value being calculated in various places in C++ code instead of the default stylesheet. > > I am attaching a test case for all three bugs. Yup, I'm fixing them all in this patch :) I just need to figure out what the heck is wrong with css style selector :(
Aharon (Vladimir) Lanin
Comment 10 2011-11-23 22:04:31 PST
Great! Hope my test case can be of use. It does not use WebKit's js test infrastructure, but should be easy enough to convert. BTW, several of the selectors specified in the HTML5 spec use the "i" notation to indicate that they have to be case-insensitive, e.g. "[dir=auto i]". From what I can see WebKit already treats attribute value selectors case insensitively, so just "[dir=auto]" works.
Ryosuke Niwa
Comment 11 2011-11-24 12:41:46 PST
(In reply to comment #10) > Hope my test case can be of use. It does not use WebKit's js test infrastructure, but should be easy enough to convert. In fact, my test contains all those cases. It'll be great if you can verify what's listed in LayoutTests/fast/css/default-bidi-css-rules-expected.txt (bdi and output tests fails due to a css style selector bug that I need to investigate) > BTW, several of the selectors specified in the HTML5 spec use the "i" notation to indicate that they have to be case-insensitive, e.g. "[dir=auto i]". From what I can see WebKit already treats attribute value selectors case insensitively, so just "[dir=auto]" works. Per CSS selectors level 3 spec, all selectors syntax is case-insensitive: http://www.w3.org/TR/selectors/#casesens
Ryosuke Niwa
Comment 12 2011-11-24 17:40:46 PST
So the problem is that CSSStyleSelector::matchAllRules is pulling unicode-bidi: embed out of CSSMappedAttributeDeclaration. My guess is that getMappedAttributeDecl has some bug.
Ryosuke Niwa
Comment 13 2011-11-24 22:45:04 PST
Hm... okay, I think I know what's going on here. The problem is that StyledElement::attributeChanged will memorize the result of parseMappedAttribute, and <span dir=""> creates an universal entry that then applies to both bdo and output elements. The solution to this problem is override mapToEntry to respect bdo and output elements with respect to dir attribute.
Ryosuke Niwa
Comment 14 2011-11-25 00:12:46 PST
Created attachment 116574 [details] Ready for review but can't land until the bug 73116 is fixed
Ryosuke Niwa
Comment 15 2011-11-25 14:24:38 PST
I've confirmed that tests pass with my patch so it's ready for review & land now.
Ryosuke Niwa
Comment 16 2011-11-25 17:28:04 PST
Eric Seidel (no email)
Comment 17 2011-11-25 17:58:29 PST
Seems reasonable to me. You might want to let mitz at least see this.
Ryosuke Niwa
Comment 18 2011-11-25 17:59:55 PST
(In reply to comment #17) > Seems reasonable to me. You might want to let mitz at least see this. Yeah, he's cc-ed on the bug but I guess I'll send him & hyatt an email as well.
Aharon (Vladimir) Lanin
Comment 19 2011-11-26 23:32:38 PST
(In reply to comment #14) > Created an attachment (id=116574) [details] > Ready for review but can't land until the bug 73116 is fixed > Source/WebCore/html/HTMLOutputElement.cpp:67 > + return eBDO; Huh? This is supposed to return a bool. And <output> is like <bdi>, not <bdo>. > LayoutTests/fast/css/default-bidi-css-rules.html:23 > + ['div', {}, 'ltr', 'normal'], Actually, for all dir values (or no dir at all), unicode-bidi should be -webkit-isolate for all "block" elements (except for <pre>, which should get -webkit-plaintext for dir=auto). This is https://bugs.webkit.org/show_bug.cgi?id=65617. Add a comment to this effect for now? > LayoutTests/fast/css/default-bidi-css-rules.html:33 > + ['span', {'dir': ''}, 'ltr', 'embed'], Not so clear that this should not be 'normal'. Empty string is not a valid value for the dir attribute. > LayoutTests/fast/css/default-bidi-css-rules.html:50 > + ['bdo', {'dir': 'auto'}, 'ltr', '-webkit-isolate'], No, should be "bidi-override -webkit-isolate".
Ryosuke Niwa
Comment 20 2011-11-27 00:03:39 PST
(In reply to comment #19) > > Source/WebCore/html/HTMLOutputElement.cpp:67 > > + return eBDO; > > Huh? This is supposed to return a bool. And <output> is like <bdi>, not <bdo>. Oops, I need to fix this. > > LayoutTests/fast/css/default-bidi-css-rules.html:23 > > + ['div', {}, 'ltr', 'normal'], > > Actually, for all dir values (or no dir at all), unicode-bidi should be -webkit-isolate for all "block" elements (except for <pre>, which should get -webkit-plaintext for dir=auto). This is https://bugs.webkit.org/show_bug.cgi?id=65617. Add a comment to this effect for now? I see. Supposedly, you meant http://dev.w3.org/html5/spec/Overview.html#flow-content-1 ? > > LayoutTests/fast/css/default-bidi-css-rules.html:33 > > + ['span', {'dir': ''}, 'ltr', 'embed'], > > Not so clear that this should not be 'normal'. Empty string is not a valid value for the dir attribute. CSS style listed in the spec says [dir] { unicode-bidi: embed } so this must be the case. > > LayoutTests/fast/css/default-bidi-css-rules.html:50 > > + ['bdo', {'dir': 'auto'}, 'ltr', '-webkit-isolate'], > > No, should be "bidi-override -webkit-isolate". Ah, interesting. Not sure if WebKit supports having two values in unicode-bidi yet. I probably need to leave it to either bidi-override or -webkit-isolate for now.
Ryosuke Niwa
Comment 21 2011-11-27 00:19:50 PST
(In reply to comment #19) > Actually, for all dir values (or no dir at all), unicode-bidi should be -webkit-isolate for all "block" elements (except for <pre>, which should get -webkit-plaintext for dir=auto). This is https://bugs.webkit.org/show_bug.cgi?id=65617. Add a comment to this effect for now? As far as I look at HTML5 spec, we have unicode-bidi: isolate on non-phrasing elements only when we don't have dir content attribute unless unicode-bidi is additive like text-decoration. Let me know if this is a mistake in the spec or misinterpretation.
Ryosuke Niwa
Comment 22 2011-11-27 00:26:22 PST
Created attachment 116667 [details] Fixed per Aharon's comment
Aharon (Vladimir) Lanin
Comment 23 2011-11-27 00:45:31 PST
(In reply to comment #20) > (In reply to comment #19) > > > LayoutTests/fast/css/default-bidi-css-rules.html:23 > > > + ['div', {}, 'ltr', 'normal'], > > > > Actually, for all dir values (or no dir at all), unicode-bidi should be -webkit-isolate for all "block" elements (except for <pre>, which should get -webkit-plaintext for dir=auto). This is https://bugs.webkit.org/show_bug.cgi?id=65617. Add a comment to this effect for now? > > I see. Supposedly, you meant http://dev.w3.org/html5/spec/Overview.html#flow-content-1 ? Not sure what you mean by "you meant". What I meant is that the test should continue to require what it currently requires here until https://bugs.webkit.org/show_bug.cgi?id=65617 is addressed (which should not be until we have verified that fixing it will not hurt efficiency too much), but the test should contain a FIXME with a reference to that bug number. Re http://dev.w3.org/html5/spec/Overview.html#flow-content-1 and "block" elements, this is actiually relevant to all of http://dev.w3.org/html5/spec/Overview.html#flow-content-1, http://dev.w3.org/html5/spec/Overview.html#sections-and-headings, http://dev.w3.org/html5/spec/Overview.html#lists, and http://dev.w3.org/html5/spec/Overview.html#tables. Also, see http://www.w3.org/Bugs/Public/show_bug.cgi?id=14850. > > > LayoutTests/fast/css/default-bidi-css-rules.html:33 > > > + ['span', {'dir': ''}, 'ltr', 'embed'], > > > > Not so clear that this should not be 'normal'. Empty string is not a valid value for the dir attribute. > > CSS style listed in the spec says [dir] { unicode-bidi: embed } so this must be the case. Perhaps. I am not enough of a CSS & HTML expert to decide. The CSS Selectors spec (http://www.w3.org/TR/selectors/#attribute-selectors) does say: "[att] represents an element with the att attribute, whatever the value of the attribute." However, the HTML5 spec (http://www.w3.org/TR/html5/common-microsyntaxes.html#keywords-and-enumerated-attributes) says re enumerated attributes: "If the attribute value matches none of the given keywords, but the attribute has an invalid value default, then the attribute represents that state. Otherwise, if the attribute value matches none of the keywords but there is a missing value default state defined, then that is the state represented by the attribute. Otherwise, there is no default, and *invalid values must be ignored*." (Emphasis mine.) The question is *who* must ignore the attribute value - just HTML, or both HTML and CSS. In other words, when CSS talks about an attribute value, is it referring to the attribute value as specified in the source file, or as represented in the DOM (which, if I understand the HTML spec correctly, should completely ignore invalid attribute values). > > > > LayoutTests/fast/css/default-bidi-css-rules.html:50 > > > + ['bdo', {'dir': 'auto'}, 'ltr', '-webkit-isolate'], > > > > No, should be "bidi-override -webkit-isolate". > > Ah, interesting. Not sure if WebKit supports having two values in unicode-bidi yet. I probably need to leave it to either bidi-override or -webkit-isolate for now. Agreed. It would be best to file a bug and refer to it in the test.
Aharon (Vladimir) Lanin
Comment 24 2011-11-27 00:47:12 PST
(In reply to comment #21) > (In reply to comment #19) > > Actually, for all dir values (or no dir at all), unicode-bidi should be -webkit-isolate for all "block" elements (except for <pre>, which should get -webkit-plaintext for dir=auto). This is https://bugs.webkit.org/show_bug.cgi?id=65617. Add a comment to this effect for now? > > As far as I look at HTML5 spec, we have unicode-bidi: isolate on non-phrasing elements only when we don't have dir content attribute unless unicode-bidi is additive like text-decoration. Let me know if this is a mistake in the spec or misinterpretation. You are correct re the spec as it currently stands, but this is a bug in the spec. I have filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=14850 on that.
Aharon (Vladimir) Lanin
Comment 25 2011-11-27 00:57:07 PST
(In reply to comment #22) > Created an attachment (id=116667) [details] > Fixed per Aharon's comment > Source/WebCore/css/html.css:995 > +bdi, output { Actually, does this whole rule do anything now? It seems to be implemented in the C++ code anyway.
Ryosuke Niwa
Comment 26 2011-11-27 01:13:52 PST
(In reply to comment #25) > (In reply to comment #22) > > Created an attachment (id=116667) [details] [details] > > Fixed per Aharon's comment > > > Source/WebCore/css/html.css:995 > > +bdi, output { > > Actually, does this whole rule do anything now? It seems to be implemented in the C++ code anyway. It does when bdi or output element doesn't have dir attribute.
mitz
Comment 27 2011-11-28 10:54:58 PST
Comment on attachment 116667 [details] Fixed per Aharon's comment View in context: https://bugs.webkit.org/attachment.cgi?id=116667&action=review > Source/WebCore/ChangeLog:9 > + http://dev.w3.org/html5/spec/Overview.html#bidirectional-text It might be better to use a permanent URL here, such as <http://dev.w3.org/cvsweb/~checkout~/html5/spec/Overview.html?rev=1.5455;content-type=text%2Fhtml#bidirectional-text>. > Source/WebCore/ChangeLog:11 > + Any element other than bdo, textarea, and pre with dir=auto should use unicode-bidi: -webkit-isolate “Any element other than bdo, textarea, and pre with dir=auto” is ambiguous. How about “Any element with dir=auto other than bdo, textarea, and pre” instead? > Source/WebCore/ChangeLog:21 > + (bdo): bdo should use -webkit-override as the default value for unicode-bidi. I think you mean “bidi-override”. > Source/WebCore/ChangeLog:22 > + * dom/MappedAttributeEntry.h: Add eBDI, which is used by bdi and output elements. Perhaps the name should include “output”? > Source/WebCore/ChangeLog:28 > + in the UA stylesheet now. Also return -webkit-isolate as the default because now that this function is > + called when dir=auto as the name implies. Something’s wrong in this last sentence.
Ryosuke Niwa
Comment 28 2011-11-28 11:12:42 PST
Thanks for the review! (In reply to comment #27) > (From update of attachment 116667 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116667&action=review > > > Source/WebCore/ChangeLog:9 > > + http://dev.w3.org/html5/spec/Overview.html#bidirectional-text > > It might be better to use a permanent URL here, such as <http://dev.w3.org/cvsweb/~checkout~/html5/spec/Overview.html?rev=1.5455;content-type=text%2Fhtml#bidirectional-text>. Hm... I see your point but this URL is quite ugly. I'll stick with the original URL as I'd expect #bidirectional-text to continue to work. > > Source/WebCore/ChangeLog:11 > > + Any element other than bdo, textarea, and pre with dir=auto should use unicode-bidi: -webkit-isolate > > “Any element other than bdo, textarea, and pre with dir=auto” is ambiguous. How about “Any element with dir=auto other than bdo, textarea, and pre” instead? Done. > > Source/WebCore/ChangeLog:21 > > + (bdo): bdo should use -webkit-override as the default value for unicode-bidi. > > I think you mean “bidi-override”. Yes. Fixed. > > Source/WebCore/ChangeLog:22 > > + * dom/MappedAttributeEntry.h: Add eBDI, which is used by bdi and output elements. > > Perhaps the name should include “output”? Hm... like eBDIOutput or eBDIAndOutput? I think eBDI makes sense in that output and bdi elements both use BiDi Isolate. > > Source/WebCore/ChangeLog:28 > > + in the UA stylesheet now. Also return -webkit-isolate as the default because now that this function is > > + called when dir=auto as the name implies. > > Something’s wrong in this last sentence. Revised to: Don't set bidi-override for bdo element since this is done in the UA stylesheet now. Set unicode-bidi to -webkit-isolate for elements other than pre and textarea now that this function is called only when dir=auto.
Ryosuke Niwa
Comment 29 2011-11-28 11:13:32 PST
Comment on attachment 116667 [details] Fixed per Aharon's comment View in context: https://bugs.webkit.org/attachment.cgi?id=116667&action=review > Source/WebCore/html/HTMLElement.cpp:146 > + // For bdo element, dir="auto" should result in "bidi-override isolate" but we don't support having multiple values in unicode-bidi yet. If you're interested, I have a patch on https://bugs.webkit.org/show_bug.cgi?id=73164 to fix this.
Ryosuke Niwa
Comment 30 2011-11-28 11:14:08 PST
(In reply to comment #29) > (From update of attachment 116667 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116667&action=review > > > Source/WebCore/html/HTMLElement.cpp:146 > > + // For bdo element, dir="auto" should result in "bidi-override isolate" but we don't support having multiple values in unicode-bidi yet. > > If you're interested, I have a patch on https://bugs.webkit.org/show_bug.cgi?id=73164 to fix this. Oops, I just realized I don't have FIXME in front of this. I'll add that and the bug number before landing the patch.
Ryosuke Niwa
Comment 31 2011-11-28 11:19:46 PST
Note You need to log in before you can comment on or make changes to this bug.