Bug 591363 - (in)visible state is not always correct? r=tbsaunde, marcoz
authorDavid Bolter <dbolter@mozilla.com>
Thu, 05 Jan 2012 22:45:11 -0500
changeset 85337 3a47e5f593ad543c7f5a2217228696b75424f569
parent 85336 39acd9b60ebc2090731a3461b25b12454d66bf43
child 85338 648239b7558d601f3d5824a8791308ede33051ee
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstbsaunde, marcoz
bugs591363
milestone12.0a1
Bug 591363 - (in)visible state is not always correct? r=tbsaunde, marcoz Here we depart from relying on layout because we don't want to walk up the ancestor chain all the way past the property page parent since this messes with screen readers virtual buffer updates (see bug).
accessible/src/base/nsAccessible.cpp
accessible/src/base/nsAccessible.h
accessible/tests/mochitest/states/Makefile.in
accessible/tests/mochitest/states/test_visibility.html
--- a/accessible/src/base/nsAccessible.cpp
+++ b/accessible/src/base/nsAccessible.cpp
@@ -585,85 +585,73 @@ nsresult nsAccessible::GetTranslatedStri
   if (!gStringBundle || 
     NS_FAILED(gStringBundle->GetStringFromName(PromiseFlatString(aKey).get(), getter_Copies(xsValue)))) 
     return NS_ERROR_FAILURE;
 
   aStringOut.Assign(xsValue);
   return NS_OK;
 }
 
-bool
-nsAccessible::IsVisible(bool* aIsOffscreen)
+PRUint64
+nsAccessible::VisibilityState()
 {
+  PRUint64 vstates = states::INVISIBLE | states::OFFSCREEN;
+
+  // We need to check the parent chain for visibility.
+  nsAccessible* accessible = this;
+  do {
+    // We don't want background tab page content to be aggressively invisible.
+    // Otherwise this foils screen reader virtual buffer caches.
+	  PRUint32 role = accessible->Role();
+    if (role == nsIAccessibleRole::ROLE_PROPERTYPAGE ||
+        role == nsIAccessibleRole::ROLE_PANE) {
+      break;
+    }
+
+    nsIFrame* frame = accessible->GetFrame();
+    if (!frame)
+      return vstates;
+
+    const nsIView* view = frame->GetView();
+    if (view && view->GetVisibility() == nsViewVisibility_kHide)
+      return vstates;
+    
+  } while (accessible = accessible->Parent());
+
+  nsIFrame* frame = GetFrame();
+  const nsCOMPtr<nsIPresShell> shell(GetPresShell());
+
   // We need to know if at least a kMinPixels around the object is visible,
-  // otherwise it will be marked states::OFFSCREEN. The states::INVISIBLE flag
-  // is for elements which are programmatically hidden.
-
-  *aIsOffscreen = true;
-  if (IsDefunct())
-    return false;
-
+  // otherwise it will be marked states::OFFSCREEN.
   const PRUint16 kMinPixels  = 12;
-   // Set up the variables we need, return false if we can't get at them all
-  nsCOMPtr<nsIPresShell> shell(GetPresShell());
-  if (!shell) 
-    return false;
-
-  nsIFrame *frame = GetFrame();
-  if (!frame) {
-    return false;
-  }
-
-  // If visibility:hidden or visibility:collapsed then mark with STATE_INVISIBLE
-  if (!frame->GetStyleVisibility()->IsVisible())
-  {
-      return false;
-  }
-
-  // We don't use the more accurate GetBoundsRect, because that is more expensive
-  // and the STATE_OFFSCREEN flag that this is used for only needs to be a rough
-  // indicator
-  nsSize frameSize = frame->GetSize();
-  nsRectVisibility rectVisibility =
+  const nsSize frameSize = frame->GetSize();
+  const nsRectVisibility rectVisibility =
     shell->GetRectVisibility(frame, nsRect(nsPoint(0,0), frameSize),
                              nsPresContext::CSSPixelsToAppUnits(kMinPixels));
 
-  if (frame->GetRect().IsEmpty()) {
-    bool isEmpty = true;
-
-    nsIAtom *frameType = frame->GetType();
-    if (frameType == nsGkAtoms::textFrame) {
-      // Zero area rects can occur in the first frame of a multi-frame text flow,
-      // in which case the rendered text is not empty and the frame should not be marked invisible
-      nsAutoString renderedText;
-      frame->GetRenderedText (&renderedText, nsnull, nsnull, 0, 1);
-      isEmpty = renderedText.IsEmpty();
-    }
-    else if (frameType == nsGkAtoms::inlineFrame) {
-      // Yuck. Unfortunately inline frames can contain larger frames inside of them,
-      // so we can't really believe this is a zero area rect without checking more deeply.
-      // GetBounds() will do that for us.
-      PRInt32 x, y, width, height;
-      GetBounds(&x, &y, &width, &height);
-      isEmpty = width == 0 || height == 0;
-    }
-
-    if (isEmpty && !(frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {
-      // Consider zero area objects hidden unless they are absolutely positioned
-      // or floating and may have descendants that have a non-zero size
-      return false;
-    }
+  if (rectVisibility == nsRectVisibility_kVisible)
+    vstates &= ~states::OFFSCREEN;
+
+  // Zero area rects can occur in the first frame of a multi-frame text flow,
+  // in which case the rendered text is not empty and the frame should not be
+  // marked invisible.
+  // XXX Can we just remove this check? Why do we need to mark empty
+  // text invisible?
+  if (frame->GetType() == nsGkAtoms::textFrame &&
+      !(frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
+      frame->GetRect().IsEmpty()) {
+    nsAutoString renderedText;
+    frame->GetRenderedText(&renderedText, nsnull, nsnull, 0, 1);
+    if (renderedText.IsEmpty())
+      return vstates;
+
   }
 
-  // The frame intersects the viewport, but we need to check the parent view chain :(
-  bool isVisible = frame->IsVisibleConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY);
-  if (isVisible && rectVisibility == nsRectVisibility_kVisible) {
-    *aIsOffscreen = false;
-  }
-  return isVisible;
+  // Assume we are visible enough.
+  return vstates &= ~states::INVISIBLE;
 }
 
 PRUint64
 nsAccessible::NativeState()
 {
   PRUint64 state = 0;
 
   nsDocAccessible* document = GetDocAccessible();
@@ -696,25 +684,18 @@ nsAccessible::NativeState()
     nsIFrame* frame = GetFrame();
     if (frame && frame->IsFocusable())
       state |= states::FOCUSABLE;
 
     if (FocusMgr()->IsFocused(this))
       state |= states::FOCUSED;
   }
 
-  // Check if states::INVISIBLE and
-  // states::OFFSCREEN flags should be turned on for this object.
-  bool isOffscreen;
-  if (!IsVisible(&isOffscreen)) {
-    state |= states::INVISIBLE;
-  }
-  if (isOffscreen) {
-    state |= states::OFFSCREEN;
-  }
+  // Gather states::INVISIBLE and states::OFFSCREEN flags for this object.
+  state |= VisibilityState();
 
   nsIFrame *frame = GetFrame();
   if (frame && (frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW))
     state |= states::FLOATING;
 
   // Check if a XUL element has the popup attribute (an attached popup menu).
   if (mContent->IsXUL())
     if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popup))
--- a/accessible/src/base/nsAccessible.h
+++ b/accessible/src/base/nsAccessible.h
@@ -666,17 +666,18 @@ protected:
 
   /**
    * Return ARIA role (helper method).
    */
   PRUint32 ARIARoleInternal();
 
   virtual nsIFrame* GetBoundsFrame();
   virtual void GetBoundsRect(nsRect& aRect, nsIFrame** aRelativeFrame);
-  bool IsVisible(bool *aIsOffscreen); 
+
+  PRUint64 VisibilityState(); 
 
   //////////////////////////////////////////////////////////////////////////////
   // Name helpers
 
   /**
    * Compute the name of HTML node.
    */
   nsresult GetHTMLName(nsAString& aName);
--- a/accessible/tests/mochitest/states/Makefile.in
+++ b/accessible/tests/mochitest/states/Makefile.in
@@ -58,16 +58,17 @@ include $(topsrcdir)/config/rules.mk
 		test_inputs.html \
 		test_inputs.xul \
 		test_link.html \
 		test_popup.xul \
 		test_selects.html \
 		test_stale.html \
 		test_textbox.xul \
 		test_tree.xul \
+		test_visibility.html \
 		z_frames.html \
 		z_frames_article.html \
 		z_frames_checkbox.html \
 		z_frames_textbox.html \
 		z_frames_update.html \
 		$(NULL)
 
 libs:: $(_TEST_FILES)
new file mode 100644
--- /dev/null
+++ b/accessible/tests/mochitest/states/test_visibility.html
@@ -0,0 +1,71 @@
+<html>
+<head>
+  <title>visibility state testing</title>
+
+  <link rel="stylesheet" type="text/css"
+        href="chrome://mochikit/content/tests/SimpleTest/test.css" />
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/MochiKit/packed.js"></script>
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+
+  <script type="application/javascript"
+          src="../common.js"></script>
+  <script type="application/javascript"
+          src="../role.js"></script>
+  <script type="application/javascript"
+          src="../states.js"></script>
+
+  <script type="application/javascript">
+    function doTest()
+    {
+      testStates("div", 0, 0, STATE_INVISIBLE);
+      testStates("div_off", STATE_OFFSCREEN, 0, STATE_INVISIBLE);
+      testStates("div_abschild", 0, 0, STATE_INVISIBLE);
+
+      // Confirm destruction of accessibles.
+      document.getElementById("div").style.visibility = "hidden";
+      document.getElementById("div_off").style.visibility="hidden";
+      document.getElementById("div_abschild").style.visibility="hidden";
+      document.body.clientWidth; // flush layout
+      testAccessibleTree("outer_div", {children:[]});
+
+
+      SimpleTest.finish();
+    }
+
+    SimpleTest.waitForExplicitFinish();
+    addA11yLoadEvent(doTest);
+  </script>
+
+</head>
+
+<body>
+
+  <a target="_blank"
+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=591363"
+     title="(in)visible state is not always correct?">
+    Mozilla Bug 591363
+  </a>
+  <p id="display"></p>
+  <div id="content" style="display: none"></div>
+  <pre id="test">
+  </pre>
+
+  <div id="outer_div">
+
+    <!-- trivial cases -->
+    <div id="div">div</div>
+    <div id="div_off" style="position: absolute; left:-999px; top:-999px">
+      offscreen!
+    </div>
+
+    <!-- edge case: no rect but has out of flow child -->
+    <div id="div_abschild">
+      <p style="position: absolute; left: 120px; top:120px;">absolute</p>
+    </div>
+
+  </div>
+</body>
+</html>