| Summary: | dir=auto should imply unicode-bidi:isolate by default | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Aharon (Vladimir) Lanin <aharon> |
| Component: | CSS | Assignee: | Ryosuke Niwa <rniwa> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | aharon, amir.aharoni, eric, macpherson, mitz, playmobil, rniwa, shanestephens, yael |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 50912, 73116, 73250 | ||
| Bug Blocks: | 50910, 70373, 71188 | ||
| Attachments: | |||
|
Description
Aharon (Vladimir) Lanin
2011-07-04 06:26:41 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. Please note related bug https://bugs.webkit.org/show_bug.cgi?id=68773 (BDI element should have dir=auto by default) 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. (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. Created attachment 116470 [details]
work in progress
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. 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. Created attachment 116479 [details] 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 (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 :( 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. (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 So the problem is that CSSStyleSelector::matchAllRules is pulling unicode-bidi: embed out of CSSMappedAttributeDeclaration. My guess is that getMappedAttributeDecl has some bug. 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. Created attachment 116574 [details] Ready for review but can't land until the bug 73116 is fixed I've confirmed that tests pass with my patch so it's ready for review & land now. Note my patch also fixes https://bugs.webkit.org/show_bug.cgi?id=68773 and https://bugs.webkit.org/show_bug.cgi?id=70373. Seems reasonable to me. You might want to let mitz at least see this. (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. (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". (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. (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. Created attachment 116667 [details]
Fixed per Aharon's comment
(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. (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. (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. (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. 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. 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. 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. (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. Committed r101268: <http://trac.webkit.org/changeset/101268> |