Bug 50916 – Add support for dir=auto

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jeremy Moskovich]

From http://dev.w3.org/html5/spec/Overview.html#the-dir-attribute:

The auto keyword, which maps to the auto state
Indicates that the contents of the element are explicitly embedded text, but that the direction is to be determined programmatically using the contents of the element (as described below).

The heuristic used by this state is very crude (it just looks at the first character with a strong directionality, in a manner analogous to the Paragraph Level determination in the bidirectional algorithm). Authors are urged to only use this value as a last resort when the direction of the text is truly unknown and no better server-side heuristic can be applied.

...

If the element's dir attribute is in the auto state
If the element is a bdi element and the dir attribute is not in a defined state (i.e. it is not present or has an invalid value)
Find the first character in tree order that matches the following criteria:

The character is from a text node that is a descendant of the element whose directionality is being determined.

The character is of bidirectional character type L, AL, or R. [BIDI]

The character is not in a text node that has an ancestor element that is a descendant of the element whose directionality is being determined and that is either:

A bdi element.
A script element.
A style element.
An element with a dir attribute in a defined state.
If such a character is found and it is of bidirectional character type AL or R, the directionality of the element is 'rtl'.

Otherwise, the directionality of the element is 'ltr'.

[Yael]

Created attachment 80700 [details]
Patch.

This patch adds support for dir="auto" by calling defaultWritingMode when we encounter an element with dir="auto".
A flag was added to indicate that a node or one of its ancestors has this attribute and value, so that we can detect DOM changes quickly and adjust the directionality accordingly.
The flag is one bit in an existing flag, so it does not consume extra memory.

This patch will fail to build on Windows, I will fix that in a future version of the patch.

This patch will fail the style check. I don't think we should fix these style errors.
(one error for consistency with the rest of the file, and the second error is wrong).

[Yael]

(In reply to comment #1)
> Created an attachment (id=80700) [details]
> Patch.
> 
> This patch will fail to build on Windows, I will fix that in a future version of the patch.
> 
I thought I needed to update Windows' def files but I guess not :)

Comment on attachment 80700 [details]
Patch.

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

> Source/WebCore/html/HTMLElement.cpp:162
> +        if (equalIgnoringCase(attr->value(), "auto"))
> +            setHasDirAutoFlagRecursively(true);
> +        else 
> +            setHasDirAutoFlagRecursively(false);

It’s not clear to me how this won’t result in clearing the flag on descendants that have dir="auto" specified when the ancestor changes from "auto" to non-"auto".

[WebKit Review Bot]

Attachment 80700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/Node.cpp:2998:  Extra space after ( in if  [whitespace/parens] [5]
Source/WebCore/css/CSSStyleSelector.cpp:3382:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/dom/Node.h:711:  The parameter name "flag" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSParser.cpp:734:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

(In reply to comment #3)
> (From update of attachment 80700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80700&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:162
> > +        if (equalIgnoringCase(attr->value(), "auto"))
> > +            setHasDirAutoFlagRecursively(true);
> > +        else 
> > +            setHasDirAutoFlagRecursively(false);
> 
> It’s not clear to me how this won’t result in clearing the flag on descendants that have dir="auto" specified when the ancestor changes from "auto" to non-"auto".

You are right, we will lose the flag in that case. I'll update the patch to correct that.
thanks!

[Yael]

Created attachment 80835 [details]
Patch.

Address comment #3. Make sure not to clear the flag if a descendant element also has dir=auto attribute.
Modified one of the new layout tests to test for this situation.

[WebKit Review Bot]

Attachment 80835 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:3384:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/css/CSSParser.cpp:734:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

(In reply to comment #7)
> Attachment 80835 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> 
> Source/WebCore/css/CSSStyleSelector.cpp:3384:  One line control clauses should not use braces.  [whitespace/braces] [4]
> Source/WebCore/css/CSSParser.cpp:734:  One space before end of line comments  [whitespace/comments] [5]
> Total errors found: 2 in 24 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

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

Further thoughts: I don’t think using the style system in this manner is appropriate. You are allowing CSS to specify 'direction: auto', which will get resolved to 'ltr' or 'rtl' at (an) arbitrary time(s). We shouldn’t expose this ability through CSS, and definitely not via a standard value like 'auto'. (Of course, a CSS value of 'auto' for the 'direction' property might be useful, and unlike this HTML construct, could take generated content into account when resolving and ignore non-rendered content, but that’s outside the scope of this bug).

The way you use setHasDirAutoFlagRecursively() is inconsistent: when 'dir' is changed from 'auto' to 'ltr', the element will lose its “self or ancestor has dir=auto” flag even if it does in fact have a dir=auto ancestor. Maybe that’s okay (and the flag is just poorly named), but then again, at other times, the flag could be set on that element. Is this a good idea?

[Yael]

Created attachment 80951 [details]
Patch.

Address comment #9.
dir=auto is resolved in the dom now, not in the css, and more checks were added when removing the flag SelfOrAncestorHasDirAutoFlag.
If you think the flag's name is inappropriate, could you please suggest how to change it ? thanks.

[Dave Hyatt]

Comment on attachment 80951 [details]
Patch.

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

> Source/WebCore/css/CSSValueKeywords.in:371
> +# auto

You don't need this any longer.

> Source/WebCore/html/HTMLElement.h:112
> +    TextDirection directionality();

This can be const, right?

[Yael]

(In reply to comment #11)
thanks for reviewing,
> (From update of attachment 80951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80951&action=review
> 
> > Source/WebCore/css/CSSValueKeywords.in:371
> > +# auto
> 
> You don't need this any longer.
> 
Right, I forgot to remove it.

> > Source/WebCore/html/HTMLElement.h:112
> > +    TextDirection directionality();
> 
> This can be const, right?
right, it should be const.

[Yael]

Created attachment 80968 [details]
Patch.

Address comment #11.

Comment on attachment 80968 [details]
Patch.

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

> Source/JavaScriptCore/ChangeLog:14
> +        * JavaScriptCore.exp:
> +        * JavaScriptCore.order:
> +        * wtf/text/StringImpl.cpp:
> +        (WTF::StringImpl::defaultWritingDirection):
> +        * wtf/text/StringImpl.h:
> +        * wtf/text/WTFString.h:
> +        (WTF::String::defaultWritingDirection):

What happened to the change log comments?

> Source/WebCore/ChangeLog:29
> +        * css/CSSStyleSelector.cpp:
> +        (WebCore::CSSStyleSelector::canShareStyleWithElement):
> +        * dom/Element.cpp:
> +        (WebCore::Element::adjustElementDirectionalityIfNeededAfterChildrenChanged):
> +        (WebCore::Element::childrenChanged):
> +        * dom/Element.h:
> +        * dom/Node.cpp:
> +        (WebCore::Node::attach):
> +        (WebCore::Node::setHasDirAutoFlagRecursively):
> +        * dom/Node.h:
> +        (WebCore::Node::selfOrAncestorHasDirAutoAttribute):
> +        * html/HTMLElement.cpp:
> +        (WebCore::HTMLElement::mapToEntry):
> +        (WebCore::HTMLElement::parseMappedAttribute):
> +        (WebCore::HTMLElement::directionality):
> +        (WebCore::HTMLElement::adjustDirectionalityAfterChildrenChanged):
> +        * html/HTMLElement.h:

Same here.

> Source/WebCore/dom/Node.cpp:1255
> +    if (parentNode() && parentNode()->getFlag(SelfOrAncestorHasDirAutoFlag))
> +        setFlag(SelfOrAncestorHasDirAutoFlag);
> +

It’s strange that inheriting this bit from the parent happens in attach() and not when actually being added to the parent in the DOM tree. It is also not cleared when attaching under a parent that doesn’t have the flag set, so it seems as if you can move a subtree from under a dir="auto" parent to a dir="ltr" parent and the subtree will maintain the flag.

I am still fuzzy on whether this patch (a) handles all dynamic cases correctly and (b) does not scan descendants multiple times as the tree is constructed. I guess that depends on when childrenChanged() is called during parsing.

> Source/WebCore/html/HTMLElement.cpp:905
> +    setAttribute(dirAttr, textDirection == LTR ? "ltr" : " rtl");

Did you mean to set the attribute here?

[WebKit Review Bot]

Attachment 80968 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7687739

[WebKit Review Bot]

Attachment 80968 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7689411

[Jeremy Moskovich]

Comment on attachment 80968 [details]
Patch.

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

Disclaimer: I'm not a WebKit reviewer, feel free to ignore my comments.

My comments are mainly targeted at making things a little easier to understand for non-native language speakers

> Source/WebCore/html/HTMLElement.cpp:868
> +    ASSERT(equalIgnoringCase(fastGetAttribute(dirAttr), "auto"));

It's only legal to call this method on an element which has dir=auto set?
Could you add a comment on why that is?

> Source/WebCore/html/HTMLElement.cpp:895
> +    return direction;

It looks like you don't modify direction in the function body, can you just return LTR here?

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto-change-child-node.html:19
> +description('Test that when an element has dir attribute with value "auto", if its first child changes, the element\'s directionality is re-evaluated.');

How about:
Test that directionality is re-evaluated when an element with dir=auto set, has it's first child modified.

The current description was a little hard for me to parse :) , same comment for other tests below.

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto.html:17
> +<div id="ltr" dir="auto" class="testDiv">Test test test</div>

Perhaps give this an id of left-to-right or similar so someone looking at the test doesn't get confused with the dir attribute?

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto.html:19
> +<div id="rtl1" dir="auto" class="testDiv">×ק×ר ××©× ×¢×ר×ת</div>

How about putting an "!" at the end of the string e.g. 
FOO!

For RTL the ! will be on the right side and for LTR it'll be on the left side.

This way you can phrase the pass/fail text above as:
"!" should be on the right side of the string.

This makes it easier for non-RTL speakers to tell that the test is passing when doing a visual inspection.

[Yael]

(In reply to comment #14)
Thank you for your review,
> (From update of attachment 80968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80968&action=review
> 
> What happened to the change log comments?
> 
Sorry, forgot to add them back after re-genrating the changelog :(

> > Source/WebCore/ChangeLog:29
> 
> Same here.
> 
Yup, forgot them here too :(

> > Source/WebCore/dom/Node.cpp:1255
> > +    if (parentNode() && parentNode()->getFlag(SelfOrAncestorHasDirAutoFlag))
> > +        setFlag(SelfOrAncestorHasDirAutoFlag);
> > +
> 
> It’s strange that inheriting this bit from the parent happens in attach() and not when actually being added to the parent in the DOM tree. It is also not cleared when attaching under a parent that doesn’t have the flag set, so it seems as if you can move a subtree from under a dir="auto" parent to a dir="ltr" parent and the subtree will maintain the flag.
> 
I am not sure I understand the difference between "Node::attach" and adding to the parent in the DOM tree? I was wondering if I should do something in detach(), and I guess I should. However, is detach() not the same as removing a node from the DOM ?

> I am still fuzzy on whether this patch (a) handles all dynamic cases correctly and (b) does not scan descendants multiple times as the tree is constructed. I guess that depends on when childrenChanged() is called during parsing.
> 
You are right, there are multiple scanning during construction. I'll see if I can mark only the part of the tree up to the first relevant text node.

> > Source/WebCore/html/HTMLElement.cpp:905
> > +    setAttribute(dirAttr, textDirection == LTR ? "ltr" : " rtl");
> 
> Did you mean to set the attribute here?
No, I wanted to change the css direction on the existing attribute. sorry about that.

[Yael]

(In reply to comment #17)
Thanks for your comments,
> (From update of attachment 80968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80968&action=review
> > Source/WebCore/html/HTMLElement.cpp:868
> > +    ASSERT(equalIgnoringCase(fastGetAttribute(dirAttr), "auto"));
> 
> It's only legal to call this method on an element which has dir=auto set?
> Could you add a comment on why that is?
> 
I am not sure what is the use for calling this function unless the element has the attribute dir="auto" ?

> > Source/WebCore/html/HTMLElement.cpp:895
> > +    return direction;
> 
> It looks like you don't modify direction in the function body, can you just return LTR here?
> 
True, direction is redundant here and can be removed.

> > LayoutTests/fast/dom/HTMLElement/attr-dir-auto-change-child-node.html:19
> > +description('Test that when an element has dir attribute with value "auto", if its first child changes, the element\'s directionality is re-evaluated.');
> 
> How about:
> Test that directionality is re-evaluated when an element with dir=auto set, has it's first child modified.
> 
ok

> The current description was a little hard for me to parse :) , same comment for other tests below.
> 
> > LayoutTests/fast/dom/HTMLElement/attr-dir-auto.html:17
> > +<div id="ltr" dir="auto" class="testDiv">Test test test</div>
> 
> Perhaps give this an id of left-to-right or similar so someone looking at the test doesn't get confused with the dir attribute?
> 
ok

> > LayoutTests/fast/dom/HTMLElement/attr-dir-auto.html:19
> > +<div id="rtl1" dir="auto" class="testDiv">×ק×ר ××©× ×¢×ר×ת</div>
> 
> How about putting an "!" at the end of the string e.g. 
> FOO!
> 
> For RTL the ! will be on the right side and for LTR it'll be on the left side.
> 
> This way you can phrase the pass/fail text above as:
> "!" should be on the right side of the string.
> 
> This makes it easier for non-RTL speakers to tell that the test is passing when doing a visual inspection.
The Hebrew text looks like garbage in the "expected results" text file, so I would not want to keep it in the expected results. Other tests that I saw also remove the text  from the expected results. 
How about if I add a comment explaining that a green border indicates right-to-left and red border indicates left-to-right ?
Of course, if there is a convention for right to left testing, I will be happy to follow that.

[Yael]

Created attachment 81377 [details]
Patch.

When an element has dir attribute with value "auto", call defaultWritingMode to find its directionality.
Added a flag SelfOrAncestorHasDirAutoFlag, and added hooks in the DOM to set and check this flag. This flag is set on every node between an element with dir=auto attribute and its first text node. Changes in the DOM between those elements will trigger re-evaluating the directionality, but changed not between those element do not need to be concerned.
The DOM hooks were added to childrenChanged, and to parseMappedAttribute. The directionality is evaluated when children are added, but not when they are removed. Once they are re-added, the directionality will be evaluated.

Added 2 static CSSMutableStyleDeclarations to be used for elements with dir=auto.
We cannot use the mapped declaration, because it can take only one value.

[WebKit Review Bot]

Attachment 81377 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7702455

[Jeremy Moskovich]

(In reply to comment #19)
> (In reply to comment #17)
> Thanks for your comments,
> > (From update of attachment 80968 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80968&action=review
> > > Source/WebCore/html/HTMLElement.cpp:868
> > > +    ASSERT(equalIgnoringCase(fastGetAttribute(dirAttr), "auto"));
> > 
> > It's only legal to call this method on an element which has dir=auto set?
> > Could you add a comment on why that is?
> > 
> I am not sure what is the use for calling this function unless the element has the attribute dir="auto" ?

My thinking is that to someone reading this code without the context of this bug, the reason for the assert isn't clear.  If you could add a comment as to why or change the name of the function [e.g. resolveDirectionality()] it may be easier to understand this.

The point I find confusing is that it looks like this is the canonical way to get the directionality of an HTMLElement, whereas it's only useful for dir=auto.

[Yael]

(In reply to comment #22)
> (In reply to comment #19)
> My thinking is that to someone reading this code without the context of this bug, the reason for the assert isn't clear.  If you could add a comment as to why or change the name of the function [e.g. resolveDirectionality()] it may be easier to understand this.
> 
> The point I find confusing is that it looks like this is the canonical way to get the directionality of an HTMLElement, whereas it's only useful for dir=auto.

You are right:) I renamed this method in the latest patch so it is more clear that it only handles dir=auto.

[Yael]

Created attachment 81401 [details]
Patch.

Fix the mac build.

[Yael]

Hyatt, Dan, I noticed that HTMLTableElement creates additionalAttributeStyleDecls.
Would you prefer if I add the direction declaration there over the way I am doing it as a static declaration in CSSStyleSelector?

[Dave Hyatt]

Comment on attachment 81401 [details]
Patch.

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

> Source/WebCore/dom/Element.cpp:1226
> +    if (isHTMLElement())
> +        adjustDirectionalityIfNeededAfterChildrenChanged(beforeChange, childCountDelta);

You should just make HTMLElement::childrenChanged and put this code there.  That way non-HTML elements don't have to test this.

> Source/WebCore/dom/Element.cpp:1827
> +TextDirection Element::directionalityIfhasDirAutoAttribute(bool& isAuto) const

Shouldn't the next 130 lines of code (all these functions) be in HTMLElement?  dir only applies to HTML, right, so it seems weird to have all this code sitting in Element.

> Source/WebCore/dom/Element.h:367
> +    void setHasDirAutoFlagRecursively(bool, Node* = 0);
> +    void dirAttributeChanged(Attribute*);
> +    void adjustDirectionalityIfNeededAfterChildAttributeChanged(Element* child);
> +    void calculateAndAdjustDirectionality();
> +    void adjustDirectionalityIfNeededAfterChildrenChanged(Node* beforeChange, int childCountDelta);

See my previous comment.  Why is all this in Element?  If auto is an HTML-specific feature only, does this have to be here?

[Yael]

Created attachment 81792 [details]
Patch.

Move the functionality to HTMLElement.cpp, as suggested in comment #26.
Created a static helper for setHasDirAutoFlagRecursively(), as suggested on IRC.

[Yael]

Created attachment 81841 [details]
Patch.

Replaced casting to HTMLElement with toHTMLElement() .

[Yael]

(In reply to comment #14)
> (From update of attachment 80968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80968&action=review
>
> I am still fuzzy on whether this patch (a) handles all dynamic cases correctly and (b) does not scan descendants multiple times as the tree is constructed. I guess that depends on when childrenChanged() is called during parsing.
> 
a) Initially, I did not address nested levels of dir="auto", but this is fixed now.
When adding a subtree to the tree, I adjust both the parent of the node that was added (and ancestors if needed) and the node itself (and descendants if needed) to reflect the change.
b) I changed the logic to mark children only between the element with dir="auto" and its first text node. This should limit the scanning to only a small number of nodes.

Could you please review this patch? thanks:)

[Dave Hyatt]

Comment on attachment 81841 [details]
Patch.

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

Much better in HTMLElement.  Just one concern:

> Source/WebCore/html/HTMLElement.cpp:1005
> +    if (childCountDelta > 0) {
> +        Node* node = beforeChange ? beforeChange->traverseNextSibling() : 0;
> +        for (int counter = 0; node && counter < childCountDelta; counter++, node = node->traverseNextSibling()) {
> +            if (node->isElementNode() && toElement(node)->hasAttribute(dirAttr))
> +                continue;
> +
> +            setHasDirAutoFlagRecursively(node, false);
> +        }
> +    }

This code concerns me.  It seems like it's going to run on every childrenChanged call?  This seems like an obvious performance hit.  Does this code really have to run all the time, or did you mean to put it below the if check?

[Yael]

Created attachment 82795 [details]
Patch.

Clear the directionality flag when nodes are removed, not when they are re-added.
The flag is not cleared if the document does not have a renderer, which indicates that the document is being destroyed.
The flag is also cleared on nodes that previously had the flag, if a new text node is added before those nodes. After the addition, those nodes no longer participate in determining the  directionality.

[Dave Hyatt]

Comment on attachment 82795 [details]
Patch.

r=me

[Yael]

Committed r79024.  <http://trac.webkit.org/changeset/79024>