Closed Bug 417929 Opened 13 years ago Closed 11 years ago

nsIAccessiblTable selectRows does not unselect previously selected rows

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: bernd_mozilla, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(2 files, 5 obsolete files)

and while I am at it selectColumn also does not work as advertised
Assignee: aaronleventhal → surkov.alexander
Blocks: tablea11y
Duplicate of this bug: 502143
Attached patch patch (obsolete) — Splinter Review
Attachment #390415 - Flags: superreview?(roc)
Attachment #390415 - Flags: review?(marco.zehe)
Attachment #390415 - Flags: review?(bolterbugz)
Attachment #390415 - Flags: review?(Olli.Pettay)
Status: NEW → ASSIGNED
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?
Attachment #390415 - Flags: review?(marco.zehe) → review+
(In reply to comment #5)

> Is it safe to not preinitialize these at all?

sure, we don't use them
Comment on attachment 390415 [details] [diff] [review]
patch

Ginn, do you have time to review this?
Attachment #390415 - Flags: review?(ginn.chen)
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.
Attachment #390415 - Flags: review?(bolterbugz)
(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?
(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.
>+  PRInt32 startRowIdx = 0, startColIdx = 0,
>+    rowSpan, colSpan, actualRowSpan, actualColSpan;

It's not necessary to initialize startRowIdx, startColIdx, either.
(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.
OK, then I guess we can add aIsOutsideOf for
UnselectRow
UnselectColumn
(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.
Attached patch patch2 (obsolete) — Splinter Review
with David and Ginn comments addressed excepting the one which is about frameSelection::remove... vs nsIAccessibleTable::Unselect...
Attachment #390415 - Attachment is obsolete: true
Attachment #391293 - Flags: superreview?(roc)
Attachment #391293 - Flags: review?(ginn.chen)
Attachment #391293 - Flags: review?(bolterbugz)
Attachment #391293 - Flags: review?(Olli.Pettay)
Attachment #390415 - Flags: superreview?(roc)
Attachment #390415 - Flags: review?(ginn.chen)
Attachment #390415 - Flags: review?(Olli.Pettay)
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.
(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
Attached patch patch3 (obsolete) — Splinter Review
Attachment #391293 - Attachment is obsolete: true
Attachment #391304 - Flags: superreview?(roc)
Attachment #391304 - Flags: review?(ginn.chen)
Attachment #391304 - Flags: review?(bolterbugz)
Attachment #391304 - Flags: review?(Olli.Pettay)
Attachment #391293 - Flags: superreview?(roc)
Attachment #391293 - Flags: review?(ginn.chen)
Attachment #391293 - Flags: review?(bolterbugz)
Attachment #391293 - Flags: review?(Olli.Pettay)
Comment on attachment 391304 [details] [diff] [review]
patch3

r=me for the accessible part
Attachment #391304 - Flags: review?(ginn.chen) → review+
Comment on attachment 391304 [details] [diff] [review]
patch3

Thanks. r=me for the a11y part
Attachment #391304 - Flags: review?(bolterbugz) → review+
Why you need nsITableSelection? You don't even QI to it ever.
Attachment #391304 - Flags: review?(Olli.Pettay) → review-
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 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.
(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.
(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.
(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.
(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.
Attached patch patch4 (obsolete) — Splinter Review
Attachment #391304 - Attachment is obsolete: true
Attachment #394810 - Flags: superreview?(roc)
Attachment #394810 - Flags: review?(Olli.Pettay)
Attachment #391304 - Flags: superreview?(roc)
Attachment #394810 - Flags: review?(Olli.Pettay) → review+
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.
(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.
Attached patch patch5 (obsolete) — Splinter Review
make SelectCellElement virtual
Attachment #394810 - Attachment is obsolete: true
Attachment #395301 - Flags: superreview?(roc)
Attachment #394810 - Flags: superreview?(roc)
Robert, any chance to review please?
Roc, friendly ping.
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?
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"?
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?
(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.)
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.
Attached patch patch6Splinter Review
Attachment #395301 - Attachment is obsolete: true
Attachment #399032 - Flags: superreview?(roc)
Attachment #395301 - Flags: superreview?(roc)
Robert, does this patch correspond to an idea you keep in mind?
Attachment #399032 - Flags: superreview?(roc) → superreview+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/b270bdb573f6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
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.
Attachment #399032 - Flags: approval1.9.2?
Attachment #399032 - Flags: approval1.9.2? → approval1.9.2+
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)
Added a mention of this fix to Firefox 3.6 for developers.
You need to log in before you can comment on or make changes to this bug.