417929 - nsIAccessiblTable selectRows does not unselect previously selected rows

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Bernd]

The idl says it should, as it is outlined at http://accessibility.freestandards.org/a11yspecs/ia2/docs/html/interface_i_accessible_table.html

However the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/html/nsHTMLTableAccessible.cpp&rev=1.59&mark=700-705#690

does certainly only select a row

Comment 1

[Bernd]

Attachment: testcase (run ,locally it uses xpconnect)

Comment 2

[Bernd]

and while I am at it selectColumn also does not work as advertised

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 390415 [details] [diff] [review]
patch

r=me for the test part. It's good to see those "todo" items going away! :)

One question from the nsHTMLTableAccessible part:
>+  PRInt32 startRowIdx = 0, startColIdx = 0,
>+    rowSpan, colSpan, actualRowSpan, actualColSpan;

Is it safe to not preinitialize these at all?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> Is it safe to not preinitialize these at all?

sure, we don't use them

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 390415 [details] [diff] [review]
patch

Ginn, do you have time to review this?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 390415 [details] [diff] [review]
patch

Sorry for delay.

Thanks. My super quick review (warning: I'm fighting a cold and drugged up):

The code where you deselect selected cells
> NS_IMETHODIMP
> nsHTMLTableAccessible::SelectRow(PRInt32 aRow)
> {
>-  return SelectRowOrColumn(aRow, nsISelectionPrivate::TABLESELECTION_ROW,
>-                           PR_TRUE);
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+

Starting here,

>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+
>+  PRInt32 columnsCount = 0;
>+  nsresult rv = GetColumns(&columnsCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIPresShell> presShell(GetPresShell());
>+  nsRefPtr<nsFrameSelection> tableSelection =
>+    const_cast<nsFrameSelection*>(presShell->ConstFrameSelection());
>+  rv = tableSelection->RemoveCellsFromSelection(content, aRow, 0,
>+                                                aRow, columnsCount - 1,
>+                                                PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv, rv);

and ending here. How about just calling you new improved UnselectRow(aRow)? (And similarly elsewhere)


> /**
>+ * Constants used to describe cells states array.
>+ */
>+const kRowSpanned = 2;
>+const kColSpanned = 4;
>+const kSpanned = kRowSpanned | kColSpanned;

Please add comments for each constant and mention that 0 and 1 are reserved for the boolean selected state.

I'll save my r+ as I'd like to see the next patch -- canceling review.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> and ending here. How about just calling you new improved UnselectRow(aRow)?
> (And similarly elsewhere)

It's not performant because I should call UnselectRow for every row, so I should run through all selection ranges for every row.

> Please add comments for each constant and mention that 0 and 1 are reserved for
> the boolean selected state.

sure, I will. Do you need new patch for this?

Comment 10

[Ginn Chen]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > and ending here. How about just calling you new improved UnselectRow(aRow)?
> > (And similarly elsewhere)
> 
> It's not performant because I should call UnselectRow for every row, so I
> should run through all selection ranges for every row.

I don't understand.
It looks like SelectRow is just doing the same thing as UnselectRow and then add the row to selection.
Did I mis-read?

+  void addCellsToSelection(in nsIContent aTable,
+                           in long aStartRowIndex, in long aStartColumnIndex,
+                           in long aEndColumnIndex, in long aEndRowIndex);

+  void removeCellsFromSelection(in nsIContent aTable,
+                                in long aStartRowIndex,
+                                in long aStartColumnIndex,
+                                in long aEndColumnIndex,
+                                in long aEndRowIndex,
+                                in boolean aIsOutsideOf);

aEndColumnIndex is ahead of aEndRowIndex, looks like a typo.

Comment 11

[Ginn Chen]

>+  PRInt32 startRowIdx = 0, startColIdx = 0,
>+    rowSpan, colSpan, actualRowSpan, actualColSpan;

It's not necessary to initialize startRowIdx, startColIdx, either.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #10)
 
> > It's not performant because I should call UnselectRow for every row, so I
> > should run through all selection ranges for every row.
> 
> I don't understand.
> It looks like SelectRow is just doing the same thing as UnselectRow and then
> add the row to selection.
> Did I mis-read?

The different in PR_TRUE or PR_FALSE. UnselectRow unselects the given row only. SelectRow unselects all rows and select the given row.

Comment 13

[Ginn Chen]

OK, then I guess we can add aIsOutsideOf for
UnselectRow
UnselectColumn

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> OK, then I guess we can add aIsOutsideOf for
> UnselectRow
> UnselectColumn

There is no much win if we'll change nsIAccesibleTable interface and I'm not sure this change might be useful. We could create internal method. But it's not big deal to obtain frame selection and call removeCellsFromSelection. I'm not sure complex UnselectRow will improve code readability.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch2

with David and Ginn comments addressed excepting the one which is about frameSelection::remove... vs nsIAccessibleTable::Unselect...

Comment 16

[Ginn Chen]

I think we can use internal method.
I think it will improve code readability for SelectRow() and SelectColumn().
I don't like copying a dozen lines of code around, it's harder to maintain.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> I think we can use internal method.
> I think it will improve code readability for SelectRow() and SelectColumn().
> I don't like copying a dozen lines of code around, it's harder to maintain.

ok

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 19

[Ginn Chen]

Comment on attachment 391304 [details] [diff] [review]
patch3

r=me for the accessible part

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 391304 [details] [diff] [review]
patch3

Thanks. r=me for the a11y part

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Why you need nsITableSelection? You don't even QI to it ever.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 391304 [details] [diff] [review]
patch3

Could you fix that and update the patch. Kick me, if I don't review the new patch soon.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 391304 [details] [diff] [review]
patch3

>+  nsRefPtr<nsFrameSelection> tableSelection =
>+    const_cast<nsFrameSelection*>(presShell->ConstFrameSelection());
Why not
nsRefPtr<nsFrameSelection> tableSelection = presShell->FrameSelection();
You cast away the constness anyway.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> Why you need nsITableSelection? You don't even QI to it ever.

Actually I need virtual function members to have an access from a11y module. I decided to introduce new interface to keep table specific methods together (I took this idea from http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#2424 and http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionPrivate.idl#86). I thought if once we'll introduce this interface any way then I could introduce it now.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #24)
> (In reply to comment #21)
> > Why you need nsITableSelection? You don't even QI to it ever.
> 
> Actually I need virtual function members to have an access from a11y module.
Well, you can access other virtual nsFrameSelection methods already, right?
Why not just add the methods there?

> I thought if once we'll introduce this interface any way then I could
> introduce it now.
The comments about nsITableSelection are 9 years old. So I'd prefer if we could
just remove them. We don't need more interfaces, we need less.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #21)
> > > Why you need nsITableSelection? You don't even QI to it ever.
> > 
> > Actually I need virtual function members to have an access from a11y module.
> Well, you can access other virtual nsFrameSelection methods already, right?
> Why not just add the methods there?

Sure, it works.

> > I thought if once we'll introduce this interface any way then I could
> > introduce it now.
> The comments about nsITableSelection are 9 years old. So I'd prefer if we could
> just remove them. We don't need more interfaces, we need less.

Ok. I'll change.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (From update of attachment 391304 [details] [diff] [review])
> >+  nsRefPtr<nsFrameSelection> tableSelection =
> >+    const_cast<nsFrameSelection*>(presShell->ConstFrameSelection());
> Why not
> nsRefPtr<nsFrameSelection> tableSelection = presShell->FrameSelection();
> You cast away the constness anyway.

It's not virtual.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 394810 [details] [diff] [review]
patch4

> nsHTMLTableAccessible::SelectRow(PRInt32 aRow)
> {
>-  return SelectRowOrColumn(aRow, nsISelectionPrivate::TABLESELECTION_ROW,
>-                           PR_TRUE);
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsresult rv =
>+    RemoveRowsOrColumnsFromSelection(aRow,
>+                                     nsISelectionPrivate::TABLESELECTION_ROW,
>+                                     PR_TRUE);
Just wondering if you could move IsDefunct() call to RemoveRowsOrColumnsFromSelection

>+nsresult
>+nsFrameSelection::AddCellToSelection(nsIContent *aCell)
>+{
>+  return SelectCellElement(aCell);
> }
This is a bit strange. Would it be possible to just make
SelectCellElement virtual?

Could you either remove the comments about nsITableSelection or file a followup bug to do that.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> (From update of attachment 394810 [details] [diff] [review])
> > nsHTMLTableAccessible::SelectRow(PRInt32 aRow)
> > {
> >-  return SelectRowOrColumn(aRow, nsISelectionPrivate::TABLESELECTION_ROW,
> >-                           PR_TRUE);
> >+  if (IsDefunct())
> >+    return NS_ERROR_FAILURE;
> >+
> >+  nsresult rv =
> >+    RemoveRowsOrColumnsFromSelection(aRow,
> >+                                     nsISelectionPrivate::TABLESELECTION_ROW,
> >+                                     PR_TRUE);
> Just wondering if you could move IsDefunct() call to
> RemoveRowsOrColumnsFromSelection

I could. But code readability might suffer I think. I prefer to put IsDefunct() to the begin of every interface method implementation and keep internal methods without IsDefunct() checking. So if won't insist then I would prefer to save current approach.

> This is a bit strange. Would it be possible to just make
> SelectCellElement virtual?

sure, I will if you like.

> 
> Could you either remove the comments about nsITableSelection or file a followup
> bug to do that.

I wouldn't like to remove these comments in this bug. Partially because I'm not sure nsITableSelection is not needed at all. In the meantime, table specific methods and constants are sprayed on several files. Also possibly this interface might be found useful for JS authors. I'll file a bug.

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch5

make SelectCellElement virtual

Comment 32

[alexander :surkov (:asurkov)]

Robert, any chance to review please?

Comment 33

[alexander :surkov (:asurkov)]

Roc, friendly ping.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry about the delay.

+   * @param  aIsOutsideOf       [in] points if unselected cells should be
+   *                             outside or inside of the given cells range

This seems unclear. Can you explain more clearly what this parameter means?

Comment 35

[alexander :surkov (:asurkov)]

If true then cells will be unselected outside of the range pointed by aStartRowIndex, aStartColIndex, aEndRowIndex and aEndColIndex arguments. Otherwise cells will be unselected inside this range.

For example, if start point of the range is (1, 1), end point is (1, 2)

|-----------------|
| oc | oc | oc | oc | oc  |
|-----------------|
| oc | ic  | ic   | oc | oc |
|-----------------|
| oc | oc | oc | oc | oc  |
|-----------------|

then aIsOutsideOf = true will unselect all 'oc' cells, otherwise it will unselect all 'ic' cells.

Probably better wording should be like

"[in] specifies whether cells should be unselected outside or inside of the given range"?

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we remove this parameter so that we always remove the cells inside the range from the selection, and have the caller make three RemoveFromRange calls to unselect the outside of a rectangular range?

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> Can we remove this parameter so that we always remove the cells inside the
> range from the selection, and have the caller make three RemoveFromRange calls
> to unselect the outside of a rectangular range?

It would be less performant because we need to traverse ranges each time, get cell indexes and etc. As well we get bigger code what will reduce code readability :) I think even additional argument doesn't make method syntax very nice but it makes the method more powerful and universal.

(Btw, I don't get how to get three calls but four ones only.)

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK. Instead of having one function which does two quite different things, I think it would be better to have two functions, one named RestrictSelectionToCells and another named RemoveCellsFromSelection, and no aIsOutsideOfCellRange parameter. If you want to have a shared internal function that implements both, that's OK, but the aIsOutsideOfCellRange parameter should probably be renamed to aRemoveOutsideOfCellRange.

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 40

[alexander :surkov (:asurkov)]

Robert, does this patch correspond to an idea you keep in mind?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 399032 [details] [diff] [review]
patch6

Yes, that's better, thanks.

Comment 42

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/b270bdb573f6

Comment 43

[Marco Zehe (:MarcoZ)]

Comment on attachment 399032 [details] [diff] [review]
patch6

Needed for completing table-related a11y support for 3.6. Baked on m-c for 4 weeks without causing regressions. See https://wiki.mozilla.org/Accessibility/Remaining_Mozilla-1.9.2_Nominations for more info.

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 399032 [details] [diff] [review]
patch6

a192=beltzner

Comment 45

[alexander :surkov (:asurkov)]

landed on 1.9.2 - http://hg.mozilla.org/releases/mozilla-1.9.2/rev/596e73cced38

Comment 46

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b2pre) Gecko/20091029 Namoroka/3.6b2pre (.NET CLR 3.5.30729)

Comment 47

[Eric Shepherd [:sheppy]]

Added a mention of this fix to Firefox 3.6 for developers.