Bug 64861 – Implement :dir() pseudo class

• Component: CSS
• Status: RESOLVED FIXED

[Aharon (Vladimir) Lanin]

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.

[Aharon (Vladimir) Lanin]

FYI, Mozilla has implemented this. See https://bugzilla.mozilla.org/show_bug.cgi?id=562169.

[Ryosuke Niwa]

(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;
}
?

[Aharon (Vladimir) Lanin]

(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

[Sukolsak Sakshuwong]

Created attachment 242777 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 242815 [details]
Patch

[Benjamin Poulain]

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

[Sylvain Galineau]

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

[Benjamin Poulain]

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();

[Sylvain Galineau]

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.

[Sylvain Galineau]

Created attachment 248160 [details]
Simple static test

A very basic static test file to check the core logic.

[Sylvain Galineau]

Created attachment 248164 [details]
Updated WIP

Updating WIP to fix bug uncovered by ASSERT_NOT_REACHED().

[Ryosuke Niwa]

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.

[Benjamin Poulain]

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

[Sylvain Galineau]

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.

[Sylvain Galineau]

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

[Ramya]

May I know when can this feature be up?

[Radar WebKit Bug Importer]

<rdar://problem/28761567>

[Yuri Guller]

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.

[Westbrook]

+1

I can't see a star or like option in the UI, so my apologies if this is improper.

[Ryosuke Niwa]

(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.

[Greg Whitworth]

@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.

[Ryosuke Niwa]

(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

[Westbrook]

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!

[Ryosuke Niwa]

(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

[r12a]

Any movement on this?

This bug report is currently being tracked by the W3C at https://github.com/w3c/alreq/issues/256.

[Ryosuke Niwa]

This is feature complete.

[Shahar Or]

Thank you!

[r12a]

+1