Selectors Level 4 (http://dev.w3.org/csswg/selectors4/#dir-pseudo) specifies the :dir() pseudo-class (:dir(ltr) and :dir(rtl)), which allows the author to write selectors that represent an element based on its HTML5 directionality (see http://dev.w3.org/html5/spec/Overview.html#the-directionality). This is a very useful capability for bidi support, even in implementing WebKit-based browsers' own controls (e.g. see http://code.google.com/p/chromium/issues/detail?id=87045). Note: this capability was originally proposed as :ltr and :rtl. This was eventually changed to the :dir(ltr|rtl) syntax.
FYI, Mozilla has implemented this. See https://bugzilla.mozilla.org/show_bug.cgi?id=562169.
(In reply to comment #0) > Selectors Level 4 (http://dev.w3.org/csswg/selectors4/#dir-pseudo) specifies the :dir() pseudo-class (:dir(ltr) and :dir(rtl)), which allows the author to write selectors that represent an element based on its HTML5 directionality (see http://dev.w3.org/html5/spec/Overview.html#the-directionality). What happens if you added a style rule like: :dir(ltr) { direction: rtl; } ?
(In reply to comment #2) > (In reply to comment #0) > > Selectors Level 4 (http://dev.w3.org/csswg/selectors4/#dir-pseudo) specifies the :dir() pseudo-class (:dir(ltr) and :dir(rtl)), which allows the author to write selectors that represent an element based on its HTML5 directionality (see http://dev.w3.org/html5/spec/Overview.html#the-directionality). > > What happens if you added a style rule like: > :dir(ltr) { > direction: rtl; > } > ? Nothing special. :dir() is not affected by the element's direction style. It works off its HTML directionality (1), which is independent of CSS. (1) http://www.w3.org/html/wg/drafts/html/master/dom.html#the-directionality
Created attachment 242777 [details] Patch
Created attachment 242815 [details] Patch
Comment on attachment 242815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242815&action=review First round is promising. The code looks correct but it is hard to tell if everything is covered because the testing is still a bit basic. I would like to see much heavier testing. The pseudo class :dir() has one of the most complex definition of any pseudo-class we support, we should test the hell out of it. > Source/WebCore/ChangeLog:9 > + http://dev.w3.org/csswg/selectors4/#the-dir-pseudo I would also mention https://html.spec.whatwg.org/multipage/dom.html#the-directionality for the definition of directionality. > Source/WebCore/css/SelectorCheckerTestFunctions.h:124 > +inline bool matchesDirPseudoClass(const Element* element, const AtomicString& direction) I don't think we want to use this function from the JIT. SelectorCheckerTestFunctions.h is only for functions that are shared between SelectorChecker and the CSS JIT. In the CSS JIT, we should be able to call element->computeInheritedDirectionality(), and compare the value to what wee expect, LTR/RTL. > Source/WebCore/dom/Element.cpp:2270 > +TextDirection Element::computeInheritedDirectionality() const This function should make JITing really easy. > Source/WebCore/dom/Element.cpp:2274 > + if (is<HTMLElement>(*node)) { This is suspicious. Why are you restricting the directionality to HTMLElements? https://html.spec.whatwg.org/multipage/dom.html#the-directionality says "The directionality of an element (any element, not just an HTML element) is either 'ltr' or 'rtl'", so I would assume :dir() should work with SVG elements if their ancestor defines a direction. > Source/WebCore/dom/Element.cpp:2282 > + TextDirection directionalityIfAuto = element.directionalityIfhasDirAutoAttribute(isAuto); I am not sure that is good enough to match the spec. The spec restricts the directionality to Text, Search, Telephone, URL, or E-mail. It looks to me like HTMLElement::directionality() has no such restriction (<input type=button> would work). This may be a bug in HTMLElement::directionality(). > Source/WebCore/dom/Element.cpp:2285 > + if (is<HTMLInputElement>(element) && equalIgnoringCase(element.fastGetAttribute(typeAttr), "tel")) Instead of "equalIgnoringCase(element.fastGetAttribute(typeAttr), "tel")", you can use downcast<HTMLInputElement>(element)->isTelephoneField(), which is the computed internal input type of the HTMLInputElement. > LayoutTests/ChangeLog:13 > + * fast/selectors/dir-inheritance-expected.html: Added. > + * fast/selectors/dir-inheritance.html: Added. I would like to also see -style-sharing and -style-update type tests. I am not saying it is incorrect, but that is very frequent cause or problem. Better having it tested upfront. I actually suspect style update is not working correctly. > LayoutTests/fast/selectors/dir-basics-expected.html:7 > +#rtl p, #rtl bdi, #rtl input, #rtl textarea { background: green } You can use :matches() to simplify selectors. For example #rtl :matches(p, bdi, input, textarea) { ... } > LayoutTests/fast/selectors/dir-basics.html:11 > +<body> It is good to have a little description in your test page. That way you can run the test manually in your browser and understand what is tested here and what is expected from the test. > LayoutTests/fast/selectors/dir-inheritance.html:11 > +<div dir="rtl"> I would also test ltr. More importantly, I would look into complex inheritance. Multiple level, each overwriting the previous, cases with bdi, etc
Created attachment 248025 [details] Work in progress A first pass; aims to conform to the specs [1][2] Major caveats: - No tests yet - We now have directionality code in both Element and HTMLElement; will attempt to refactor the latter out once this works correctly. [1] http://dev.w3.org/csswg/selectors-4/#the-dir-pseudo [2] https://html.spec.whatwg.org/multipage/dom.html#the-directionality
Comment on attachment 248025 [details] Work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=248025&action=review Quick review > Source/WebCore/css/CSSSelector.cpp:339 > bool CSSSelector::operator==(const CSSSelector& other) const Missing blank line between the methods. > Source/WebCore/css/SelectorChecker.cpp:58 > + #include "TextDirection.h" Extra space before the # > Source/WebCore/css/SelectorChecker.cpp:1012 > // FIXME: Implement :dir() selector. Don't forget to remove the comment. :) > Source/WebCore/css/SelectorChecker.cpp:1014 > + { This bracket should be on the previous line. > Source/WebCore/css/SelectorChecker.cpp:1018 > + if (!CSSSelector::parseDirection(selector->argument(), dir)) { > + return false; > + } The WebKit coding style is against brackets for single conditional statements. > Source/WebCore/css/SelectorChecker.cpp:1019 > + return (element->computeInheritedDirection() == dir); No need for the parenthesis here. > Source/WebCore/dom/Element.cpp:2424 > + return LTR; // Must ASSERT we don't get here ASSERT_NOT_REACHED() > Source/WebCore/dom/Element.h:384 > + enum DirAttributeState { DirLTR, DirRTL, DirAuto, DirUnknown }; > + DirAttributeState dir() const; > + TextDirection computeInheritedDirection() const; For the enum, you can use a enum class. Shouldn't dir() be private? Maybe it is personal taste, but I am not a big fan of the abbreviated "dir". Maybe?: enum class SelfDirectionality { LTR, RTL, Auto, Unknown } selfDirectionality() computeInheritedDirectionality();
Created attachment 248159 [details] Updated WIP All stylistic fixes applied. A few comments below. >> Source/WebCore/css/SelectorChecker.cpp:1014 >> + { >This bracket should be on the previous line. I matched the style of all the other case statements in the function e.g. PseudoClassLang, PseudoClassScope...; do they all need fixing? >> Source/WebCore/dom/Element.cpp:2424 >> + return LTR; // Must ASSERT we don't get here >ASSERT_NOT_REACHED() This was worth trying because it does get reached. Will look into this more...Commented out for now.
Created attachment 248160 [details] Simple static test A very basic static test file to check the core logic.
Created attachment 248164 [details] Updated WIP Updating WIP to fix bug uncovered by ASSERT_NOT_REACHED().
Comment on attachment 248164 [details] Updated WIP View in context: https://bugs.webkit.org/attachment.cgi?id=248164&action=review > Source/WebCore/css/CSSSelector.cpp:332 > + if (equalIgnoringCase(name, "rtl")) { else if? name can't match both "ltr" and "rtl", right? > Source/WebCore/dom/Element.cpp:2426 > +bool Element::evalContentDirectionality(TextDirection& direction) const { How is this algorithm different from HTMLElement::directionality? It seems like we should at least try to share some code.
Comment on attachment 248164 [details] Updated WIP View in context: https://bugs.webkit.org/attachment.cgi?id=248164&action=review Review: second round. I suggest you add some tests for the next round. That way we can also discuss the test cases. This feature is gonna be fun. > Source/WebCore/ChangeLog:7 > + Let's add links to the relevant specs: -http://dev.w3.org/csswg/selectors-4/#the-dir-pseudo -https://html.spec.whatwg.org/multipage/scripting.html#selectors -https://html.spec.whatwg.org/multipage/dom.html#the-directionality >> Source/WebCore/css/CSSSelector.cpp:332 >> + if (equalIgnoringCase(name, "rtl")) { > > else if? name can't match both "ltr" and "rtl", right? Right, or just early return in each branch and "valid" can go away. > Source/WebCore/css/CSSSelector.h:216 > + static bool parseDirection(const String&, TextDirection& dir); Note that the argument is an AtomicString. You are forcing a conversion by using String here. I would explore an alternate design here: 1) Define an enumeration with the possible direction: enum class SelectorDirectionality { Invalid, RTL, LTR } 2) Have a instance method that assert() on the pseudo class type and return the parsed argument as SelectorDirectionality. > Source/WebCore/dom/Element.cpp:2387 > + SelfDirectionality d = selfDirectionality(); Do not abbreviate variable names: d -> selfDirectionality td -> textDirection > Source/WebCore/dom/Element.cpp:2407 > + if (d == SelfDirectionality::DirUnknown && input.isTelephoneField()) > + return LTR; > + if (d == SelfDirectionality::DirAuto && > + (input.isTextField() || > + input.isSearchField() || > + input.isTelephoneField() || > + input.isURLField() || > + input.isEmailField())) { > + if (evalContentDirectionality(td)) > + return td; It is inelegant that Element needs to know so much about HTMLInputElement and HTMLTextAreaElement. Could we delegate that to those types somehow? > Source/WebCore/dom/Element.cpp:2414 > + } else if (d == SelfDirectionality::DirAuto || (d == SelfDirectionality::DirUnknown && equalIgnoringCase(nodeName(), "bdi"))) { equalIgnoringCase(nodeName(), "bdi")) could match a bdi element in an other namespace. This should probably be hasTagName(dirTag) > Source/WebCore/dom/Element.cpp:2417 > + if (evalContentDirectionality(td)) { > + return td; > + } No need for brackets for single line conditional statements. > Source/WebCore/dom/Element.cpp:2421 > + if (parentElement()) > + return parentElement()->computeInheritedDirectionality(); Should be: if (Element* parentElement = this->parentElement()) return parentElement->computeInheritedDirectionality(); otherwise we rely on the compiler's CSE to remove the duplicated computation. >> Source/WebCore/dom/Element.cpp:2426 >> +bool Element::evalContentDirectionality(TextDirection& direction) const { > > How is this algorithm different from HTMLElement::directionality? > It seems like we should at least try to share some code. It indeed looks like this could be unified with HTMLElement::directionality(). Looks to me that HTMLElement::directionality() is buggy when checking for the "bdi" element
Created attachment 248222 [details] Work In Progress > I suggest you add some tests for the next round. That way we can also discuss the test cases. Yes. I will try to get to this later this week, I made all suggested changes. Comments/questions on those not yet below. >> Source/WebCore/css/CSSSelector.h:216 >> + static bool parseDirection(const String&, TextDirection& dir); > Note that the argument is an AtomicString. You are forcing a conversion by using String here. > I would explore an alternate design here: > 1) Define an enumeration with the possible direction: > enum class SelectorDirectionality { > Invalid, > RTL, > LTR > } > 2) Have a instance method that assert() on the pseudo class type and return the parsed argument as > SelectorDirectionality. That would work; the idea was to some a simple == comparison in SelectorChecker.cpp using an enum that cause Element to depend on CSSSelector or vice-versa. FWIW I tried to add an Invalid state to TextDirection but that results in a number of switch/cases missing a branch in cases where it wouldn't happen. Still, I agree this can and should be improved. >> Source/WebCore/dom/Element.cpp:2387 >> + SelfDirectionality d = selfDirectionality(); > Do not abbreviate variable names: > d -> selfDirectionality Note: that is also a method name so I just went for directionality. >> Source/WebCore/dom/Element.cpp:2407 >> + if (d == SelfDirectionality::DirUnknown && input.isTelephoneField()) >> + return LTR; >> + if (d == SelfDirectionality::DirAuto && >> + (input.isTextField() || >> + input.isSearchField() || [snip] > It is inelegant that Element needs to know so much about HTMLInputElement and HTMLTextAreaElement. Yes; HTMLElement's directionality logic does this but it certainly looks way more awkward up here. > Could we delegate that to those types somehow? I did consider this could be virtual with Element handling the general case. I wasn't sure on the first attempt whether things would be checked in the right order this way. I'd like to make sure the logic is solid before trying to break it across multiple objects. > It indeed looks like this could be unified with HTMLElement::directionality(). Yes; I do not think there should be two. > Looks to me that HTMLElement::directionality() is buggy when checking for the "bdi" element I think so too.
Created attachment 251902 [details] Work In Progress Update previous patch. Main change: - Address feedback from previous code review. - Delegate directionality evaluation to proper subclasses of Element. - Fix some bugs in the previous version. Next: - Invalidation - Tests, tests, tests
May I know when can this feature be up?
<rdar://problem/28761567>
Any chance to urge the implementation of this or raise it's priority? In the thread below I've layed out some issue that may be solved here and otherwise kinda stacked: https://stackoverflow.com/questions/59876111/css-pseudo-dir-host-context-and-directionality-based-styling In essence: given :dir() is not implemented and :host-context() is not going to be implemented (in WebKit only) - there is no CSS based way to style contents of shadow DOM based on the directionality.
+1 I can't see a star or like option in the UI, so my apologies if this is improper.
(In reply to Yuri Guller from comment #18) > In essence: given :dir() is not implemented and :host-context() is not going > to be implemented (in WebKit only) Note that while Gecko does support :dir, Gecko has no plan or has not implemented :host-context. In fact, :host-context is likely going to be removed from the spec soon since there are no independent multiple implementations of this feature. Having said that, I don't think there is any reason why we don't want to support :dir, and all your feedback that this is an important feature for web components is noted. I'll go talk with my colleagues.
@Ryosuke Niwa Likewise, we would really like to move off our synthetic shadow polyfill and this is one aspect we'll need in order to accomplish it. So if this can be prioritized we would greatly appreciate it so we can use native shadow DOM on our core platform.
(In reply to Greg Whitworth from comment #21) > @Ryosuke Niwa > > Likewise, we would really like to move off our synthetic shadow polyfill and > this is one aspect we'll need in order to accomplish it. So if this can be > prioritized we would greatly appreciate it so we can use native shadow DOM > on our core platform. For that purpose, we need to first resolve: https://github.com/whatwg/html/issues/3699
Ryosuke, as per your comment here: https://github.com/whatwg/html/issues/3699#issuecomment-603345811 and Blink's flip to "fixed" here: https://bugs.chromium.org/p/chromium/issues/detail?id=576815#c30 are you able to share any information on when we might be able to see this added to Webkit? Thanks in advance!
(In reply to Westbrook from comment #23) > Ryosuke, as per your comment here: > https://github.com/whatwg/html/issues/3699#issuecomment-603345811 and > Blink's flip to "fixed" here: > https://bugs.chromium.org/p/chromium/issues/detail?id=576815#c30 are you > able to share any information on when we might be able to see this added to > Webkit? I don't understand what Chromium implemented given the relevant CSS spec hasn't even been updated yet. I guess I'm gonna ask about it in https://github.com/whatwg/html/issues/3699
Any movement on this? This bug report is currently being tracked by the W3C at https://github.com/w3c/alreq/issues/256.
This is feature complete.
Thank you!
+1