Move WalkDescendantsClearAncestorDirAuto from BeforeSetAttr to OnSetDirAttr to simplify the test for elements that had dir=auto but no longer do, especially <bdi>. Bug 845093, r=ehsan
authorSimon Montagu <smontagu@smontagu.org>
Thu, 07 Mar 2013 22:53:08 +0200
changeset 124152 9f15cf555746e337b15e12b118913a5c194c65bd
parent 124151 ac291d349daaaa6dfd6eca8a2e099c3fdc380e3f
child 124153 563213431c9fb7f996c44e1598c5a975b72c25fc
push id24408
push userryanvm@gmail.com
push dateFri, 08 Mar 2013 04:58:11 +0000
treeherdermozilla-central@cb432984d5ce [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs845093
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Move WalkDescendantsClearAncestorDirAuto from BeforeSetAttr to OnSetDirAttr to simplify the test for elements that had dir=auto but no longer do, especially <bdi>. Bug 845093, r=ehsan
content/base/public/DirectionalityUtils.h
content/base/src/DirectionalityUtils.cpp
content/base/src/Element.cpp
content/html/content/src/nsGenericHTMLElement.cpp
content/html/content/src/nsGenericHTMLElement.h
--- a/content/base/public/DirectionalityUtils.h
+++ b/content/base/public/DirectionalityUtils.h
@@ -112,16 +112,17 @@ void SetDirectionalityFromValue(mozilla:
  * in AfterSetAttr we don't know the old value, so we can't identify all cases
  * where we need to walk up or down the document tree and reset the direction;
  * and in BeforeSetAttr we can't do the walk because this element hasn't had the
  * value set yet so the results will be wrong.
  */
 void OnSetDirAttr(mozilla::dom::Element* aElement,
                   const nsAttrValue* aNewValue,
                   bool hadValidDir,
+                  bool hadDirAuto,
                   bool aNotify);
 
 /**
  * Called when binding a new element to the tree, to set the
  * NodeAncestorHasDirAuto flag and set the direction of the element and its
  * ancestors if necessary
  */
 void SetDirOnBind(mozilla::dom::Element* aElement, nsIContent* aParent);
--- a/content/base/src/DirectionalityUtils.cpp
+++ b/content/base/src/DirectionalityUtils.cpp
@@ -857,17 +857,17 @@ SetDirectionalityFromValue(Element* aEle
     dir = eDir_LTR;
   }
 
   aElement->SetDirectionality(dir, aNotify);
 }
 
 void
 OnSetDirAttr(Element* aElement, const nsAttrValue* aNewValue,
-             bool hadValidDir, bool aNotify)
+             bool hadValidDir, bool hadDirAuto, bool aNotify)
 {
   if (aElement->IsHTML(nsGkAtoms::input)) {
     return;
   }
 
   if (aElement->AncestorHasDirAuto()) {
     if (!hadValidDir) {
       // The element is a descendant of an element with dir = auto, is
@@ -878,16 +878,30 @@ OnSetDirAttr(Element* aElement, const ns
       WalkDescendantsResetAutoDirection(aElement);
     } else if (!aElement->HasValidDir()) {
       // The element is a descendant of an element with dir = auto and is
       // having its dir attribute removed or set to an invalid value.
       // Reset the direction of any of its ancestors whose direction is
       // determined by a text node descendant
       WalkAncestorsResetAutoDirection(aElement, aNotify);
     }
+  } else if (hadDirAuto && !aElement->HasDirAuto()) {
+    // The element isn't a descendant of an element with dir = auto, and is
+    // having its dir attribute set to something other than auto.
+    // Walk the descendant tree and clear the AncestorHasDirAuto flag.
+    //
+    // N.B: For elements other than <bdi> it would be enough to test that the
+    //      current value of dir was "auto" in BeforeSetAttr to know that we
+    //      were unsetting dir="auto". For <bdi> things are more complicated,
+    //      since it behaves like dir="auto" whenever the dir attribute is
+    //      empty or invalid, so we would have to check whether the old value
+    //      was not either "ltr" or "rtl", and the new value was either "ltr"
+    //      or "rtl". Element::HasDirAuto() encapsulates all that, so doing it
+    //      here is simpler.
+    WalkDescendantsClearAncestorDirAuto(aElement);
   }
 
   if (aElement->HasDirAuto()) {
     WalkDescendantsSetDirAuto(aElement, aNotify);
   } else {
     SetDirectionalityOnDescendants(aElement,
                                    RecomputeDirectionality(aElement, aNotify),
                                    aNotify);
--- a/content/base/src/Element.cpp
+++ b/content/base/src/Element.cpp
@@ -1849,20 +1849,22 @@ Element::SetAttrAndNotify(int32_t aNames
   // Copy aParsedValue for later use since it will be lost when we call
   // SetAndTakeMappedAttr below
   nsAttrValue aValueForAfterSetAttr;
   if (aCallAfterSetAttr) {
     aValueForAfterSetAttr.SetTo(aParsedValue);
   }
 
   bool hadValidDir = false;
+  bool hadDirAuto = false;
 
   if (aNamespaceID == kNameSpaceID_None) {
     if (aName == nsGkAtoms::dir) {
       hadValidDir = HasValidDir() || IsHTML(nsGkAtoms::bdi);
+      hadDirAuto = HasDirAuto(); // already takes bdi into account
     }
 
     // XXXbz Perhaps we should push up the attribute mapping function
     // stuff to Element?
     if (!IsAttributeMapped(aName) ||
         !SetMappedAttribute(document, aName, aParsedValue, &rv)) {
       rv = mAttrsAndChildren.SetAndTakeAttr(aName, aParsedValue);
     }
@@ -1892,17 +1894,18 @@ Element::SetAttrAndNotify(int32_t aNames
     nsNodeUtils::AttributeChanged(this, aNamespaceID, aName, aModType);
   }
 
   if (aCallAfterSetAttr) {
     rv = AfterSetAttr(aNamespaceID, aName, &aValueForAfterSetAttr, aNotify);
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::dir) {
-      OnSetDirAttr(this, &aValueForAfterSetAttr, hadValidDir, aNotify);
+      OnSetDirAttr(this, &aValueForAfterSetAttr,
+                   hadValidDir, hadDirAuto, aNotify);
     }
   }
 
   if (aFireMutation) {
     nsMutationEvent mutation(true, NS_MUTATION_ATTRMODIFIED);
 
     nsAutoString ns;
     nsContentUtils::NameSpaceManager()->GetNameSpaceURI(aNamespaceID, ns);
@@ -2047,19 +2050,21 @@ Element::UnsetAttr(int32_t aNameSpaceID,
     slots->mAttributeMap->DropAttribute(aNameSpaceID, aName);
   }
 
   // The id-handling code, and in the future possibly other code, need to
   // react to unexpected attribute changes.
   nsMutationGuard::DidMutate();
 
   bool hadValidDir = false;
+  bool hadDirAuto = false;
 
   if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::dir) {
     hadValidDir = HasValidDir() || IsHTML(nsGkAtoms::bdi);
+    hadDirAuto = HasDirAuto(); // already takes bdi into account
   }
 
   nsAttrValue oldValue;
   rv = mAttrsAndChildren.RemoveAttrAt(index, oldValue);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (document || HasFlag(NODE_FORCE_XBL_BINDINGS)) {
     nsRefPtr<nsXBLBinding> binding =
@@ -2075,17 +2080,17 @@ Element::UnsetAttr(int32_t aNameSpaceID,
     nsNodeUtils::AttributeChanged(this, aNameSpaceID, aName,
                                   nsIDOMMutationEvent::REMOVAL);
   }
 
   rv = AfterSetAttr(aNameSpaceID, aName, nullptr, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::dir) {
-    OnSetDirAttr(this, nullptr, hadValidDir, aNotify);
+    OnSetDirAttr(this, nullptr, hadValidDir, hadDirAuto, aNotify);
   }
 
   if (hasMutationListeners) {
     nsCOMPtr<nsIDOMEventTarget> node = do_QueryObject(this);
     nsMutationEvent mutation(true, NS_MUTATION_ATTRMODIFIED);
 
     mutation.mRelatedNode = attrNode;
     mutation.mAttrName = aName;
--- a/content/html/content/src/nsGenericHTMLElement.cpp
+++ b/content/html/content/src/nsGenericHTMLElement.cpp
@@ -770,35 +770,16 @@ nsGenericHTMLElement::GetHrefURIForAncho
   // We use the nsAttrValue's copy of the URI string to avoid copying.
   nsCOMPtr<nsIURI> uri;
   GetURIAttr(nsGkAtoms::href, nullptr, getter_AddRefs(uri));
 
   return uri.forget();
 }
 
 nsresult
-nsGenericHTMLElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
-                                    const nsAttrValueOrString* aValue,
-                                    bool aNotify)
-{
-  if (aNamespaceID == kNameSpaceID_None &&
-      aName == nsGkAtoms::dir &&
-      HasDirAuto() && !AncestorHasDirAuto()) {
-    // When setting dir on an element that currently has dir=auto, we walk the
-    // descendant tree and clear the AncestorHasDirAuto flag; unless this
-    // element itself has the AncestorHasDirAuto flag
-    WalkDescendantsClearAncestorDirAuto(this);
-    SetHasDirAuto();
-  }
-
-  return nsGenericHTMLElementBase::BeforeSetAttr(aNamespaceID, aName,
-                                                 aValue, aNotify);
-}
-
-nsresult
 nsGenericHTMLElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                    const nsAttrValue* aValue, bool aNotify)
 {
   if (aNamespaceID == kNameSpaceID_None) {
     if (IsEventAttributeName(aName) && aValue) {
       NS_ABORT_IF_FALSE(aValue->Type() == nsAttrValue::eString,
         "Expected string value for script body");
       nsresult rv = SetEventHandler(aName, aValue->GetStringValue());
--- a/content/html/content/src/nsGenericHTMLElement.h
+++ b/content/html/content/src/nsGenericHTMLElement.h
@@ -755,20 +755,16 @@ protected:
       RegUnRegAccessKey(false);
     }
   }
 
 private:
   void RegUnRegAccessKey(bool aDoReg);
 
 protected:
-  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
-                                 const nsAttrValueOrString* aValue,
-                                 bool aNotify);
-
   virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue, bool aNotify);
 
   virtual nsEventListenerManager*
     GetEventListenerManagerForAttr(nsIAtom* aAttrName, bool* aDefer);
 
   virtual const nsAttrName* InternalGetExistingAttrNameFromQName(const nsAString& aStr) const;