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

@@ -590,75 +590,63 @@ nsresult nsAccessible::GetTranslatedString(const nsAString& aKey, nsAString& aSt
   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;
+  if (rectVisibility == nsRectVisibility_kVisible)
+    vstates &= ~states::OFFSCREEN;
 
-    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;
-    }
+  // 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;
 
-    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;
-    }
   }
 
-  // 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
@@ -701,15 +689,8 @@ nsAccessible::NativeState()
       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))

accessible/src/base/nsAccessible.h

@@ -671,7 +671,8 @@ protected:
 
   virtual nsIFrame* GetBoundsFrame();
   virtual void GetBoundsRect(nsRect& aRect, nsIFrame** aRelativeFrame);
-  bool IsVisible(bool *aIsOffscreen); 
+
+  PRUint64 VisibilityState(); 
 
   //////////////////////////////////////////////////////////////////////////////
   // Name helpers

accessible/tests/mochitest/states/Makefile.in

@@ -63,6 +63,7 @@ _TEST_FILES =\
 		test_stale.html \
 		test_textbox.xul \
 		test_tree.xul \
+		test_visibility.html \
 		z_frames.html \
 		z_frames_article.html \
 		z_frames_checkbox.html \

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>