Resolve direction correctly when dir attribute is not in a defined state

Source/core/html/HTMLElement.cpp

Index: Source/core/html/HTMLElement.cpp
diff --git a/Source/core/html/HTMLElement.cpp b/Source/core/html/HTMLElement.cpp
index 8eb8c19b2410213c17b090ca9e38f3d5d6fed2ce..efb44044516f3b1dacd2a72a29e508c4f0787018 100644
--- a/Source/core/html/HTMLElement.cpp
+++ b/Source/core/html/HTMLElement.cpp
@@ -211,10 +211,12 @@ void HTMLElement::collectStyleForPresentationAttribute(const QualifiedName& name
         if (equalIgnoringCase(value, "auto"))
             addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, unicodeBidiAttributeForDirAuto(this));
         else {
-            if (isValidDirAttribute(value))
+            if (isValidDirAttribute(value)) {
                 addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, value);
-            else
-                addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "ltr");

[Sunil Ratnu, 2014/11/21 12:21:08]

Hi tkent, We need to keep this else check here because removing it breaks
fast/dom/Attr/direction-attribute-set-and-cleared.html test. The parent lookup
fails here and keeps the direction "rtl" even after clearing it to "" as
mentioned in the test. So, we need to make sure that we consider parent
element's directionality and resolve accordingly. The mentioned test fails no
more now.

[tkent, 2014/11/25 01:27:24]

I don't think parent lookup is a right solution.
How about
    style->removeProperty(CSSPropertyDirection);
?

[Sunil Ratnu, 2014/11/25 05:43:25]

The above approach does not work here. The above mentioned test still fails.
Since this is the only place where we are checking for invalid dirAttr or
clearing of dirAttr and finally adding the direction to CSSPropertyDirection, we
need parent lookup so sa to resolve the direction and that seems reasonable.

[tkent, 2014/11/25 05:56:09]

Please investigate why it didn't work, and find another solution without parent
lookup.  Adding parent lookup is not acceptable.  Style resolution already
contains parent lookup, and adding DOM parent lookup will introduce
inconsistency and confusion.

+            } else {
+                const AtomicString& dirValue = determineDirection() == RTL ? "rtl" : "ltr";
+                addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, dirValue);
+            }
             if (!hasTagName(bdiTag) && !hasTagName(bdoTag) && !hasTagName(outputTag))
                 addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, CSSValueEmbed);
         }
@@ -715,6 +717,30 @@ TextDirection HTMLElement::directionalityIfhasDirAutoAttribute(bool& isAuto) con
     return directionality();
 }
 
+// FIXME: Implement directionality for input type='tel'.
+// Spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#the-directionality.
+TextDirection HTMLElement::determineDirection() const

[Sunil Ratnu, 2014/11/21 12:21:08]

Instead of moving it here, we can also define a new function
effectiveDirAttributeValue (as suggested by you in case of input type='tel') but
that also has to use a lot of same code here. Hence, I moved it from
HTMLTextFormControlElement.cpp in order to reduce the duplicated code.

+{
+    for (const HTMLElement* element = this; element; element = Traversal<HTMLElement>::firstAncestor(*element)) {
+        const AtomicString& dirAttributeValue = element->fastGetAttribute(dirAttr);
+        if (dirAttributeValue.isNull())
+            continue;
+
+        if (equalIgnoringCase(dirAttributeValue, "rtl"))
+            return RTL;
+
+        if (equalIgnoringCase(dirAttributeValue, "ltr"))
+            return LTR;
+
+        if (equalIgnoringCase(dirAttributeValue, "auto")) {
+            bool isAuto;
+            return element->directionalityIfhasDirAutoAttribute(isAuto);
+        }
+    }
+
+    return LTR;
+}
+
 TextDirection HTMLElement::directionality(Node** strongDirectionalityTextNode) const
 {
     if (isHTMLInputElement(*this)) {