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'.

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).

(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".

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.

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.

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.

(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?

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.

(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.

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.

(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.

(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.

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?

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?

(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:)

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.