[Webkit-unassigned] [Bug 64861] Need support for :dir() pseudo-class

Mon Dec 8 15:05:33 PST 2014

https://bugs.webkit.org/show_bug.cgi?id=64861

Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #242815|review?                     |review-
              Flags|                            |

--- Comment #6 from Benjamin Poulain <benjamin at webkit.org> ---
Comment on attachment 242815
  --> https://bugs.webkit.org/attachment.cgi?id=242815
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141208/f97fcaf8/attachment-0002.html>