Bug 64861 - Need support for :dir() pseudo-class
Summary: Need support for :dir() pseudo-class
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Sylvain Galineau
URL: https://drafts.csswg.org/selectors-4/...
Keywords: InRadar
Depends on:
Blocks: 159753
  Show dependency treegraph
 
Reported: 2011-07-20 04:18 PDT by Aharon (Vladimir) Lanin
Modified: 2016-10-13 13:25 PDT (History)
15 users (show)

See Also:


Attachments
Patch (14.69 KB, patch)
2014-12-07 18:24 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (14.90 KB, patch)
2014-12-08 08:50 PST, Sukolsak Sakshuwong
benjamin: review-
Details | Formatted Diff | Diff
Work in progress (9.08 KB, patch)
2015-03-05 17:50 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff
Updated WIP (9.40 KB, application/octet-stream)
2015-03-07 10:53 PST, Sylvain Galineau
no flags Details
Simple static test (1.38 KB, text/html)
2015-03-07 10:54 PST, Sylvain Galineau
no flags Details
Updated WIP (9.25 KB, patch)
2015-03-07 11:13 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff
Work In Progress (9.66 KB, patch)
2015-03-08 18:13 PDT, Sylvain Galineau
no flags Details | Formatted Diff | Diff
Work In Progress (13.08 KB, patch)
2015-04-28 17:48 PDT, Sylvain Galineau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aharon (Vladimir) Lanin 2011-07-20 04:18:16 PDT
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.
Comment 1 Aharon (Vladimir) Lanin 2012-08-23 09:25:06 PDT
FYI, Mozilla has implemented this. See https://bugzilla.mozilla.org/show_bug.cgi?id=562169.
Comment 2 Ryosuke Niwa 2013-02-03 14:02:47 PST
(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;
}
?
Comment 3 Aharon (Vladimir) Lanin 2013-02-04 06:07:06 PST
(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
Comment 4 Sukolsak Sakshuwong 2014-12-07 18:24:25 PST
Created attachment 242777 [details]
Patch
Comment 5 Sukolsak Sakshuwong 2014-12-08 08:50:12 PST
Created attachment 242815 [details]
Patch
Comment 6 Benjamin Poulain 2014-12-08 15:05:33 PST
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
Comment 7 Sylvain Galineau 2015-03-05 17:50:17 PST
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 8 Benjamin Poulain 2015-03-06 12:40:05 PST
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();
Comment 9 Sylvain Galineau 2015-03-07 10:53:04 PST
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.
Comment 10 Sylvain Galineau 2015-03-07 10:54:20 PST
Created attachment 248160 [details]
Simple static test

A very basic static test file to check the core logic.
Comment 11 Sylvain Galineau 2015-03-07 11:13:09 PST
Created attachment 248164 [details]
Updated WIP

Updating WIP to fix bug uncovered by ASSERT_NOT_REACHED().
Comment 12 Ryosuke Niwa 2015-03-07 16:55:56 PST
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 13 Benjamin Poulain 2015-03-08 17:04:13 PDT
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
Comment 14 Sylvain Galineau 2015-03-08 18:13:21 PDT
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.
Comment 15 Sylvain Galineau 2015-04-28 17:48:14 PDT
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
Comment 16 Ramya 2016-02-11 19:41:42 PST
May I know when can this feature be up?
Comment 17 Radar WebKit Bug Importer 2016-10-13 13:25:27 PDT
<rdar://problem/28761567>