Bug 501659 - HTML table's isRowSelected/isColumnSelected shouldn't fail if row or column has cell holes, r=davidb
authorAlexander Surkov <surkov.alexander@gmail.com>
Sun, 18 Oct 2009 10:38:27 +0800
changeset 34235 b6ce00fade5eb344996142e69fe0dad4bb51bf6d
parent 34234 425726d54dc9766f92dfee3b641d3c77ae8e2613
child 34236 33c41598d5ced2596cf5ee2b020bfb0d4d3bef74
push id115
push userbmcbride@mozilla.com
push dateMon, 19 Oct 2009 23:27:00 +0000
reviewersdavidb
bugs501659
milestone1.9.3a1pre
Bug 501659 - HTML table's isRowSelected/isColumnSelected shouldn't fail if row or column has cell holes, r=davidb
accessible/src/html/nsHTMLTableAccessible.cpp
accessible/src/html/nsHTMLTableAccessible.h
accessible/tests/mochitest/test_table_sels.html
--- a/accessible/src/html/nsHTMLTableAccessible.cpp
+++ b/accessible/src/html/nsHTMLTableAccessible.cpp
@@ -919,40 +919,30 @@ nsHTMLTableAccessible::GetSelectedRowInd
   *aRows = outArray;
   return rv;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetCellAt(PRInt32 aRow, PRInt32 aColumn,
                                  nsIAccessible **aTableCellAccessible)
 {
-  NS_ENSURE_TRUE(IsValidRow(aRow) && IsValidColumn(aColumn), NS_ERROR_INVALID_ARG);
-
-  nsresult rv = NS_OK;
-
   nsCOMPtr<nsIDOMElement> cellElement;
-  rv = GetCellAt(aRow, aColumn, *getter_AddRefs(cellElement));
+  nsresult rv = GetCellAt(aRow, aColumn, *getter_AddRefs(cellElement));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  nsCOMPtr<nsIAccessibilityService>
-    accService(do_GetService("@mozilla.org/accessibilityService;1"));
-  NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE);
-
-  return accService->GetAccessibleInWeakShell(cellElement, mWeakShell,
-                                              aTableCellAccessible);
+  return GetAccService()->GetAccessibleInWeakShell(cellElement, mWeakShell,
+                                                   aTableCellAccessible);
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetCellIndexAt(PRInt32 aRow, PRInt32 aColumn,
                                       PRInt32 *aIndex)
 {
   NS_ENSURE_ARG_POINTER(aIndex);
 
-  NS_ENSURE_TRUE(IsValidRow(aRow) && IsValidColumn(aColumn), NS_ERROR_INVALID_ARG);
-
   nsITableLayout *tableLayout = GetTableLayout();
   NS_ENSURE_STATE(tableLayout);
 
   nsresult rv = tableLayout->GetIndexByRowAndColumn(aRow, aColumn, aIndex);
   if (rv == NS_TABLELAYOUT_CELL_NOT_FOUND)
     return NS_ERROR_INVALID_ARG;
 
   return NS_OK;
@@ -965,145 +955,158 @@ nsHTMLTableAccessible::GetColumnIndexAt(
 
   if (IsDefunct())
     return NS_ERROR_FAILURE;
 
   nsITableLayout *tableLayout = GetTableLayout();
   NS_ENSURE_STATE(tableLayout);
 
   PRInt32 row;
-  return tableLayout->GetRowAndColumnByIndex(aIndex, &row, aColumn);
+  nsresult rv = tableLayout->GetRowAndColumnByIndex(aIndex, &row, aColumn);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return (row == -1 || *aColumn == -1) ? NS_ERROR_INVALID_ARG : NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetRowIndexAt(PRInt32 aIndex, PRInt32 *aRow)
 {
   NS_ENSURE_ARG_POINTER(aRow);
 
   if (IsDefunct())
     return NS_ERROR_FAILURE;
 
   nsITableLayout *tableLayout = GetTableLayout();
   NS_ENSURE_STATE(tableLayout);
 
   PRInt32 column;
-  return tableLayout->GetRowAndColumnByIndex(aIndex, aRow, &column);
+  nsresult rv = tableLayout->GetRowAndColumnByIndex(aIndex, aRow, &column);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return (*aRow == -1 || column == -1) ? NS_ERROR_INVALID_ARG : NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetColumnExtentAt(PRInt32 aRowIndex,
                                          PRInt32 aColumnIndex,
                                          PRInt32 *aExtentCount)
 {
-  NS_ENSURE_TRUE(IsValidRow(aRowIndex) && IsValidColumn(aColumnIndex),
-                 NS_ERROR_INVALID_ARG);
-
   nsITableLayout *tableLayout = GetTableLayout();
   NS_ENSURE_STATE(tableLayout);
 
   nsCOMPtr<nsIDOMElement> domElement;
   PRInt32 startRowIndex, startColIndex, rowSpan, colSpan, actualRowSpan;
   PRBool isSelected;
 
-  return tableLayout->
+  nsresult rv = tableLayout->
     GetCellDataAt(aRowIndex, aColumnIndex, *getter_AddRefs(domElement),
                   startRowIndex, startColIndex, rowSpan, colSpan,
                   actualRowSpan, *aExtentCount, isSelected);
+
+  return (rv == NS_TABLELAYOUT_CELL_NOT_FOUND) ? NS_ERROR_INVALID_ARG : NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetRowExtentAt(PRInt32 aRowIndex, PRInt32 aColumnIndex,
                                       PRInt32 *aExtentCount)
 {
-  NS_ENSURE_TRUE(IsValidRow(aRowIndex) && IsValidColumn(aColumnIndex),
-                 NS_ERROR_INVALID_ARG);
-
   nsITableLayout *tableLayout = GetTableLayout();
   NS_ENSURE_STATE(tableLayout);
 
   nsCOMPtr<nsIDOMElement> domElement;
   PRInt32 startRowIndex, startColIndex, rowSpan, colSpan, actualColSpan;
   PRBool isSelected;
 
-  return tableLayout->
+  nsresult rv = tableLayout->
     GetCellDataAt(aRowIndex, aColumnIndex, *getter_AddRefs(domElement),
                   startRowIndex, startColIndex, rowSpan, colSpan,
                   *aExtentCount, actualColSpan, isSelected);
+
+  return (rv == NS_TABLELAYOUT_CELL_NOT_FOUND) ? NS_ERROR_INVALID_ARG : NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetColumnDescription(PRInt32 aColumn, nsAString &_retval)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::GetRowDescription(PRInt32 aRow, nsAString &_retval)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
-nsHTMLTableAccessible::IsColumnSelected(PRInt32 aColumn, PRBool *_retval)
+nsHTMLTableAccessible::IsColumnSelected(PRInt32 aColumn, PRBool *aIsSelected)
 {
-  NS_ENSURE_ARG_POINTER(_retval);
+  NS_ENSURE_ARG_POINTER(aIsSelected);
+  *aIsSelected = PR_FALSE;
 
-  NS_ENSURE_TRUE(IsValidColumn(aColumn), NS_ERROR_INVALID_ARG);
-
-  nsresult rv = NS_OK;
-
-  PRInt32 rows;
-  rv = GetRowCount(&rows);
+  PRInt32 colCount = 0;
+  nsresult rv = GetColumnCount(&colCount);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  for (PRInt32 index = 0; index < rows; index++) {
-    rv = IsCellSelected(index, aColumn, _retval);
-    NS_ENSURE_SUCCESS(rv, rv);
-    if (!*_retval) {
-      break;
+  if (aColumn < 0 || aColumn >= colCount)
+    return NS_ERROR_INVALID_ARG;
+
+  PRInt32 rowCount = 0;
+  rv = GetRowCount(&rowCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  for (PRInt32 rowIdx = 0; rowIdx < rowCount; rowIdx++) {
+    PRBool isSelected = PR_FALSE;
+    rv = IsCellSelected(rowIdx, aColumn, &isSelected);
+    if (NS_SUCCEEDED(rv)) {
+      *aIsSelected = isSelected;
+      if (!isSelected)
+        break;
     }
   }
 
-  return rv;
+  return NS_OK;
 }
 
 NS_IMETHODIMP
-nsHTMLTableAccessible::IsRowSelected(PRInt32 aRow, PRBool *_retval)
+nsHTMLTableAccessible::IsRowSelected(PRInt32 aRow, PRBool *aIsSelected)
 {
-  NS_ENSURE_ARG_POINTER(_retval);
+  NS_ENSURE_ARG_POINTER(aIsSelected);
+  *aIsSelected = PR_FALSE;
 
-  NS_ENSURE_TRUE(IsValidRow(aRow), NS_ERROR_INVALID_ARG);
-
-  nsresult rv = NS_OK;
-
-  PRInt32 columns;
-  rv = GetColumnCount(&columns);
+  PRInt32 rowCount = 0;
+  nsresult rv = GetRowCount(&rowCount);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  for (PRInt32 index = 0; index < columns; index++) {
-    rv = IsCellSelected(aRow, index, _retval);
-    NS_ENSURE_SUCCESS(rv, rv);
-    if (!*_retval) {
-      break;
+  if (aRow < 0 || aRow >= rowCount)
+    return NS_ERROR_INVALID_ARG;
+
+  PRInt32 colCount = 0;
+  rv = GetColumnCount(&colCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  for (PRInt32 colIdx = 0; colIdx < colCount; colIdx++) {
+    PRBool isSelected = PR_FALSE;
+    rv = IsCellSelected(aRow, colIdx, &isSelected);
+    if (NS_SUCCEEDED(rv)) {
+      *aIsSelected = isSelected;
+      if (!isSelected)
+        break;
     }
   }
 
-  return rv;
+  return NS_OK;
 }
 
 NS_IMETHODIMP
 nsHTMLTableAccessible::IsCellSelected(PRInt32 aRow, PRInt32 aColumn,
                                       PRBool *aIsSelected)
 {
   NS_ENSURE_ARG_POINTER(aIsSelected);
   *aIsSelected = PR_FALSE;
 
-  NS_ENSURE_TRUE(IsValidRow(aRow) && IsValidColumn(aColumn),
-                 NS_ERROR_INVALID_ARG);
-
   nsITableLayout *tableLayout = GetTableLayout();
   NS_ENSURE_STATE(tableLayout);
 
   nsCOMPtr<nsIDOMElement> domElement;
   PRInt32 startRowIndex = 0, startColIndex = 0,
           rowSpan, colSpan, actualRowSpan, actualColSpan;
 
   nsresult rv = tableLayout->
@@ -1111,32 +1114,16 @@ nsHTMLTableAccessible::IsCellSelected(PR
                   startRowIndex, startColIndex, rowSpan, colSpan,
                   actualRowSpan, actualColSpan, *aIsSelected);
 
   if (rv == NS_TABLELAYOUT_CELL_NOT_FOUND)
     return NS_ERROR_INVALID_ARG;
   return rv;
 }
 
-PRBool
-nsHTMLTableAccessible::IsValidColumn(PRInt32 aColumn)
-{
-  PRInt32 colCount = 0;
-  nsresult rv = GetColumnCount(&colCount);
-  return NS_SUCCEEDED(rv) && (aColumn >= 0) && (aColumn < colCount);
-}
-
-PRBool
-nsHTMLTableAccessible::IsValidRow(PRInt32 aRow)
-{
-  PRInt32 rowCount = 0;
-  nsresult rv = GetRowCount(&rowCount);
-  return NS_SUCCEEDED(rv) && (aRow >= 0) && (aRow < rowCount);
-}
-
 NS_IMETHODIMP
 nsHTMLTableAccessible::SelectRow(PRInt32 aRow)
 {
   if (IsDefunct())
     return NS_ERROR_FAILURE;
 
   nsresult rv =
     RemoveRowsOrColumnsFromSelection(aRow,
--- a/accessible/src/html/nsHTMLTableAccessible.h
+++ b/accessible/src/html/nsHTMLTableAccessible.h
@@ -139,30 +139,16 @@ public:
   virtual nsresult GetNameInternal(nsAString& aName);
   virtual nsresult GetRoleInternal(PRUint32 *aRole);
   virtual nsresult GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState);
   virtual nsresult GetAttributesInternal(nsIPersistentProperties *aAttributes);
 
   // nsHTMLTableAccessible
 
   /**
-    * Returns true if the column index is in the valid column range.
-    *
-    * @param aColumn  The index to check for validity.
-    */
-  PRBool IsValidColumn(PRInt32 aColumn);
-
-  /**
-    * Returns true if the given index is in the valid row range.
-    *
-    * @param aRow  The index to check for validity.
-    */
-  PRBool IsValidRow(PRInt32 aRow);
-
-  /**
    * Retun cell element at the given row and column index.
    */
   nsresult GetCellAt(PRInt32 aRowIndex, PRInt32 aColIndex,
                      nsIDOMElement* &aCell);
 
   /**
    * Return nsITableLayout for the frame of the accessible table.
    */
--- a/accessible/tests/mochitest/test_table_sels.html
+++ b/accessible/tests/mochitest/test_table_sels.html
@@ -17,16 +17,19 @@
           src="chrome://mochikit/content/a11y/accessible/states.js"></script>
   <script type="application/javascript"
           src="chrome://mochikit/content/a11y/accessible/table.js"></script>
 
   <script type="text/javascript">
 
     function doTest()
     {
+      //////////////////////////////////////////////////////////////////////////
+      // table
+
       var cellsArray =
       [
         [false, false,       false,       kColSpanned, false, false,       false, false],
         [false, false,       false,       false,       false, false,       false, kRowSpanned],
         [false, false,       kColSpanned, false,       false, false,       false, kRowSpanned],
         [false, kRowSpanned, kSpanned,    false,       false, kRowSpanned, false, kRowSpanned]
       ];
 
@@ -48,16 +51,30 @@
       for (var colIdx = 0; colIdx < columsCount; colIdx++) {
         testSelectTableColumn("table", colIdx, cellsArray);
         testUnselectTableColumn("table", colIdx, cellsArray);
       }
 
       var accTable = getAccessible("table", [nsIAccessibleTable]);
       ok(!accTable.isProbablyForLayout(), "table is not for layout");
 
+      //////////////////////////////////////////////////////////////////////////
+      // table instane
+
+      cellsArray =
+      [
+        [false,       false, false,       -1,          -1],
+        [false,       false, false,       -1,          -1],
+        [false,       false, kColSpanned, kColSpanned, -1],
+        [kRowSpanned, false, false,       -1,          -1],
+        [kRowSpanned, false, kRowSpanned, false,       false]
+      ];
+
+      testTableSelection("tableinsane", cellsArray);
+
       SimpleTest.finish();
     }
 
     SimpleTest.waitForExplicitFinish();
     addA11yLoadEvent(doTest);
   </script>
  </head>
  <body>
@@ -72,16 +89,21 @@
      title="nsHTMLTableAccessible::GetSelectedCells contains index duplicates for spanned rows or columns">
     Mozilla Bug 501635
   </a>
   <a target="_blank"
      href="https://bugzilla.mozilla.org/show_bug.cgi?id=417929"
      title="nsIAccessiblTable selectRows does not unselect previously selected rows">
     Mozilla Bug 417929
   </a>
+  <a target="_blank"
+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=501659"
+     title="HTML table's isRowSelected/isColumnSelected shouldn't fail if row or column has cell holes">
+    Mozilla Bug 501659
+  </a>
 
   <p id="display"></p>
   <div id="content" style="display: none"></div>
   <pre id="test">
   </pre>
 
   <!-- Test Table -->
   <br><br><b> Testing Table:</b><br><br>
@@ -117,11 +139,42 @@
      <tr>
       <td><br></td>
       <td><br></td>
       <td><br></td>
       <td><br></td>
      </tr>
     </tbody>
    </table>
+
+   <table border="1" id="tableinsane">
+    <thead>
+     <tr>
+      <th>col1</th>
+      <th>col2</th>
+      <th>col3</th>
+     </tr>
+    </thead>
+     <tbody>
+      <tr>
+       <td>1</td>
+       <td>2</td>
+       <td>3</td>
+      </tr>
+      <tr>
+       <td rowspan="3">4</td>
+       <td colspan="4">5</td>
+      </tr>
+      <tr>
+       <td>6</td>
+       <td rowspan="2">7</td>
+      </tr>
+      <tr>
+       <td>8</td>
+       <td>9</td>
+       <td>10</td>
+      </tr>
+     </tbody>
+    </table>
+
   </center>
  </body>
 </html>