Implement CSS4 pseudo-class :dir()

Implement CSS4 pseudo-class :dir()

Implement parsing and matching of :dir() pseudo class
spec: https://drafts.csswg.org/selectors-4/#the-dir-pseudo

BUG=576815

[ramya.v, 2016-01-27 06:25:15]

Patchset #2 (id:20001) has been deleted

[ramya.v, 2016-01-27 07:39:20]

ramya.v@samsung.com changed reviewers:
+ timloh@chromium.org

[ramya.v, 2016-01-27 07:39:21]

Hi Tim, 

PTAL!

Do I have to raise an Intent to Implement And Ship first?

Thanks.

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt
(right):

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:2:
FAIL ':dir(rtl)' matches all elements whose directionality is 'rtl'.
assert_array_equals: lengths differ, expected 7 got 6
&#x202E;<span id=span4>WERBEH</span>&#x202C;
As of now is calculated as ltr hence the difference.

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:3:
FAIL ':dir(ltr)' matches all elements whose directionality is 'ltr'.
assert_array_equals: lengths differ, expected 20 got 21
ditto

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:4:
FAIL ':dir(ltr)' doesn't match elements not in the document.
assert_array_equals: lengths differ, expected 14 got 21
This is giving same output as previous test. As per the expectations, it is
supposed to ignore html, head, body, bdi1, bdo5, script4 and the newly created
bdo element. Not sure exactly why it is supposed to ignore them. Can you please
let me know why the specified output is expected?

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir.html
(right):

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir.html:31:
testSelector(":dir(ltr)", ["html", "head", "body", "meta", "title", "link1",
"link2", "script1", "script2", "script3", "style", "log", "bdo2", "bdi1",
"bdi3", "span3", "span6", "bdo3", "bdo5", "script4"], "':dir(ltr)' matches all
elements whose directionality is 'ltr'.");
1)Last script element is ignored. So included it.
2)style id is mispelt so corrected it. 
3)bdi1 element is not included in both ltr and rtl. Included it in ltr section.

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir01-expected.txt
(right):

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir01-expected.txt:2:
FAIL direction doesn't affect :dir() assert_array_equals: lengths differ,
expected 16 got 14
Expected:
html, head, body, meta, title, link, link, script, script, script, div, div,
div, div, span, script.

Actual:
html, head, body, meta, title, link, link, script, script, script, div, div,
div, script.

Here div id=div1 and span id=div2 both specify direction as rtl. However text
inside div id=dir1 is rendered from right to left.

As per expected both dir(ltr) considers both of them as ltr.
As per actual results dir(ltr) considers both of them as rtl and doesn't list in
results.


Should div1 be calculated as ltr or rtl?

[ramya.v, 2016-02-08 03:48:23]

On 2016/01/27 07:39:21, ramya.v wrote:
> Hi Tim, 
> 
> PTAL!
> 
> Do I have to raise an Intent to Implement And Ship first?
> 
> Thanks.
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:2:
> FAIL ':dir(rtl)' matches all elements whose directionality is 'rtl'.
> assert_array_equals: lengths differ, expected 7 got 6
> &#x202E;<span id=span4>WERBEH</span>&#x202C;
> As of now is calculated as ltr hence the difference.
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:3:
> FAIL ':dir(ltr)' matches all elements whose directionality is 'ltr'.
> assert_array_equals: lengths differ, expected 20 got 21
> ditto
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir-expected.txt:4:
> FAIL ':dir(ltr)' doesn't match elements not in the document.
> assert_array_equals: lengths differ, expected 14 got 21
> This is giving same output as previous test. As per the expectations, it is
> supposed to ignore html, head, body, bdi1, bdo5, script4 and the newly created
> bdo element. Not sure exactly why it is supposed to ignore them. Can you
please
> let me know why the specified output is expected?
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir.html
> (right):
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir.html:31:
> testSelector(":dir(ltr)", ["html", "head", "body", "meta", "title", "link1",
> "link2", "script1", "script2", "script3", "style", "log", "bdo2", "bdi1",
> "bdi3", "span3", "span6", "bdo3", "bdo5", "script4"], "':dir(ltr)' matches all
> elements whose directionality is 'ltr'.");
> 1)Last script element is ignored. So included it.
> 2)style id is mispelt so corrected it. 
> 3)bdi1 element is not included in both ltr and rtl. Included it in ltr
section.
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir01-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/web-platform-tests/html/semantics/selectors/pseudo-classes/dir01-expected.txt:2:
> FAIL direction doesn't affect :dir() assert_array_equals: lengths differ,
> expected 16 got 14
> Expected:
> html, head, body, meta, title, link, link, script, script, script, div, div,
> div, div, span, script.
> 
> Actual:
> html, head, body, meta, title, link, link, script, script, script, div, div,
> div, script.
> 
> Here div id=div1 and span id=div2 both specify direction as rtl. However text
> inside div id=dir1 is rendered from right to left.
> 
> As per expected both dir(ltr) considers both of them as ltr.
> As per actual results dir(ltr) considers both of them as rtl and doesn't list
in
> results.
> 
> 
> Should div1 be calculated as ltr or rtl?




Friendly ping!

[Timothy Loh, 2016-02-11 02:53:26]

Yeah this needs an intent to implement and ship

[esprehn, 2016-02-12 05:13:40]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org, rune@opera.com

[esprehn, 2016-02-12 05:13:41]

This approach doesn't work, you need to store it in the DOM. Also I'm not sure
the existing code works correctly either,
Element::selfOrAncestorHasDirAutoAttribute delegates through to LayoutObject,
but what if the style is dirty? This makes me question how
HTMLElement::dirAttributeChanged can work at all... it doesn't update style and
then asks style dependent questions (not that it should update style, it just
seems like something here isn't right).

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:958: dir =
context.elementStyle->direction();
how does this work? The elementStyle is the one being created right now, we
haven't figured out what values apply yet.

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:960: dir =
element.ensureComputedStyle()->direction();
ensureComputedStyle() is non-trivial, always do 

if (ComputedStyle* style = element.ensureComputedStyle()) {
  // use style here
}

I don't think this is right though, you can't compute the style in a detached
subtree, and this would cause a style resolve from inside querySelector for
display none elements, or use stale style for displayed elements giving you back
the wrong value.

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:963: value = "ltr";
we should just atomize these with a DEFINE_STATIC_LOCAL(AtomicString, ltrAtom,
"ltr") and then the check below will usually be very fast since authors will
write dir(ltr)

https://codereview.chromium.org/1614343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:972: if (value.isEmpty()
|| !equalIgnoringCase(value, argument))
should this use ascii case? this file is inconsistent, I'm not sure when we're
supposed to be using ascii these days.

[kojii, 2016-03-17 15:26:31]

ramya@, we had some discussion at style-dev@:
https://groups.google.com/a/chromium.org/d/msg/style-dev/aWaTEQQPo2M/IcRK8zB5...

Are you still motivated to move this forward? That'd be helpful if so.

[kojii, 2016-03-17 15:28:56]

ah, just noticed that the intent doesn't have enough sign-off yet, sorry about
that. I'll reply to the thread.

[ramya.v, 2016-03-18 03:54:21]

On 2016/03/17 15:28:56, kojii wrote:
> ah, just noticed that the intent doesn't have enough sign-off yet, sorry about
> that. I'll reply to the thread.

@kojii I would like to continue work on this.
Waiting for more insights on the thread, 
so that I can get some clarity on how this should be implemented without
affecting performance.

[Timothy Loh, 2016-04-11 04:17:36]

What's the status of this?

[ramya.v, 2016-04-11 04:21:25]

On 2016/04/11 04:17:36, Timothy Loh wrote:
> What's the status of this?

Hi Tim,
As of now I think I'm not having sufficient knowledge to implement :dir() work
for shadow dom. 
It would be better if someone more proficient takes up this implementation of
CSS :dir() class.
so I would like to close this review for now.