dir=auto needs to work on value of input and textarea elements

• Component: DOM
• Status: RESOLVED FIXED

[Aharon (Vladimir) Lanin]

dir=auto as currently implemented in WebKit does not do anything useful for input elements, and only works on a textarea's initial value, without changing as the user modifies it. This is because dir=auto currently looks at the element's descendant text nodes. The HTML5 spec (http://dev.w3.org/html5/spec/Overview.html#the-dir-attribute) needs to be changed in this regard. Filed bug http://www.w3.org/Bugs/Public/show_bug.cgi?id=13482 on HTML5, asking that the spec be changed such that:

- For input and textarea elements with dir=auto, the directionality should be determined not by the element's descendants, but by the element's current value, and should change as the latter changes.

- For other elements, the text node descendants under a descendant textarea element should be excluded, just like those in script and style elements.

[Yael]

Created attachment 102584 [details]
Patch.

Changed the directionality algorithm to evaluate the value of input elements and textarea elements when they have the attribute dir-auto Set.
Also skip these elements when evaluating the directionality of their ancestors.

HTMLTextAreaElement::childrenChanged is not called when a user types into the textarea, so call calculateAndAdjustDirectionality() explicitly from HTMLTextAreaElement::subtreeHasChanged().

[Ryosuke Niwa]

Comment on attachment 102584 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=102584&action=review

> Source/WebCore/html/HTMLElement.cpp:887
> +        if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag) || (node->isElementNode() && toElement(node)->isTextFormControl())) {

Maybe split into two lines?

> Source/WebCore/html/HTMLTextAreaElement.cpp:259
> +    // When typing in a textarea, childrenChanged is not called, so we need to force the directionality check.
> +    calculateAndAdjustDirectionality();

Should we also call this in setValue and when "dir" attribute is changed?  e.g. dir attribute is removed and we need to figure out new directionality.

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto-text-form-control-expected.txt:9
> +PASS document.defaultView.getComputedStyle(el, null).getPropertyValue('border-right-color') is 'rgb(255, 0, 0)'

This is a very clever way of testing!

[Yael]

(In reply to comment #2)
> (From update of attachment 102584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102584&action=review
> 
Thank you for the review :)
> > Source/WebCore/html/HTMLElement.cpp:887
> > +        if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag) || (node->isElementNode() && toElement(node)->isTextFormControl())) {
> 
> Maybe split into two lines?
> 
ok

> > Source/WebCore/html/HTMLTextAreaElement.cpp:259
> > +    // When typing in a textarea, childrenChanged is not called, so we need to force the directionality check.
> > +    calculateAndAdjustDirectionality();
> 
> Should we also call this in setValue and when "dir" attribute is changed?  e.g. dir attribute is removed and we need to figure out new directionality.
> 
We already handle the case where dir attribute changes in HTMLTextAreaElement, like we handle it for any other HTMLElement so there is no need to explicitly call calculateAndAdjustDirectionality()

> > LayoutTests/fast/dom/HTMLElement/attr-dir-auto-text-form-control-expected.txt:9
> > +PASS document.defaultView.getComputedStyle(el, null).getPropertyValue('border-right-color') is 'rgb(255, 0, 0)'
> 
> This is a very clever way of testing!
thanks :)

[Yael]

Created attachment 102705 [details]
Patch.

Added a test for dynamic changes to dir attribute in text form controls, and added explicit call to calculateAndAdjustDirectionality() from HTMLInputElement::subtreeHasChanged(), with a check that it is called only for text fields.

Also broke the line as suggested in comment #2.

[Darin Adler]

Comment on attachment 102705 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=102705&action=review

> Source/WebCore/html/HTMLElement.cpp:876
> +    HTMLTextFormControlElement* textElement = toTextFormControl(const_cast<HTMLElement*>(this));
> +    if (textElement) {

Would be elegant to put this definition inside the if so it gets scoped.

> Source/WebCore/html/HTMLElement.cpp:881
> +        return (textDirection == WTF::Unicode::LeftToRight) ? LTR : RTL;

Is the WTF:: needed here?

> Source/WebCore/html/HTMLElement.cpp:888
>          // Skip bdi, script and style elements
> -        if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag)) {
> +        if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag) 
> +            || (node->isElementNode() && toElement(node)->isTextFormControl())) {

The comment no longer matches the code.

> Source/WebCore/html/HTMLInputElement.cpp:666
> +    // When typing in a input field, childrenChanged is not called, so we need to force the directionality check.

should be "an input field"

[Yael]

rniwa, based on Hixie's comment at http://www.w3.org/Bugs/Public/show_bug.cgi?id=13482#c1, are you ok with committing this?

[Ryosuke Niwa]

(In reply to comment #6)
> rniwa, based on Hixie's comment at http://www.w3.org/Bugs/Public/show_bug.cgi?id=13482#c1, are you ok with committing this?

Yes.  After all, Darin has already r+ed it :)

[Yael]

Created attachment 103069 [details]
Patch.

Addressing comment #5.

[WebKit Review Bot]

Comment on attachment 103069 [details]
Patch.

Clearing flags on attachment: 103069

Committed r92480: <http://trac.webkit.org/changeset/92480>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

Seems like it broke the Mac and the 64bit bot on the Szeged master.

[Balazs Kelemen]

(In reply to comment #11)
> Seems like it broke the Mac and the 64bit bot on the Szeged master.

After another build it's more likely that it is flakiness.
One test is still failing on the Mac bot but it's not one of the 2 failure that was appearing first time after this change.