760881 - decomtaminate Select Row / Column() on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

similar approach to bug 757504

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Asking feedback from mentor ...

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 631252 [details] [diff] [review]
Patch (v1)

one review is enough for this bug

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 631252 [details] [diff] [review]
Patch (v1)

>+  for (PRInt32 rowIdx = 0; (row = rowIter.Next()); rowIdx++)
>+    SetARIASelected(row, rowIdx == aRowIdx);

I don't think that should fail under normal cases, so I'd rather not get rid of the warning all together.  I'm not sure what the best replacement is though, maybe NS_ASSERTION(NS_SUCCeeDED(rv), "blah shouldn't fail!"); ?


>-    nsresult rv = SetARIASelected(row, false);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    SetARIASelected(row, false);

same

>+  RemoveRowsOrColumnsFromSelection(aRowIdx,
>+                                   nsISelectionPrivate::TABLESELECTION_ROW,
>+                                   true);
> 
>-  nsresult rv =
>-    RemoveRowsOrColumnsFromSelection(aRow,
>-                                     nsISelectionPrivate::TABLESELECTION_ROW,
>-                                     true);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  return AddRowOrColumnToSelection(aRow,
>-                                   nsISelectionPrivate::TABLESELECTION_ROW);
>+  AddRowOrColumnToSelection(aRowIdx, nsISelectionPrivate::TABLESELECTION_ROW);

same for these functions.

>   nsCOMPtr<nsITreeSelection> selection;
>   mTreeView->GetSelection(getter_AddRefs(selection));
>-  NS_ENSURE_STATE(selection);

same here

>+  if (control) {

I don't think you need to add this check, it wasn't here before, and we really shouldn't create this type of accessible when the content is of the different type.

>+    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>+    control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
>+    control->SelectItem(item);

on the other hand we used to check item wasn't null but I'm not sure that's really necesary now since we assume the row index is valid.

Comment 4

[Mark Capella [:capella]]

re: "I don't think that should fail under normal cases" ...

I patterned these SelectRow() / SelectCol() methods on the existing UnselectRow () / UnselectCol() methods, which don't have warnings. If you lean towards my keeping warnings in Select methods, should I retro-fit them back into the Unselect methods?

re: if (control) { ... I don't think you need to add this check ...

Again, this is how the Unselect() function currently is coded, so I sought uniformity.

If you are flexible with your thoughts here, I'd like to leave the patch as I now have it. If you strongly disagree, then I'd feel better at least making consistent changes for both the Select() and Unselect() methods.

Comment 5

[Mark Capella [:capella]]

TRY push
https://tbpl.mozilla.org/?tree=Try&rev=049e05386514

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #4)
> re: "I don't think that should fail under normal cases" ...
> 
> I patterned these SelectRow() / SelectCol() methods on the existing
> UnselectRow () / UnselectCol() methods, which don't have warnings. If you
> lean towards my keeping warnings in Select methods, should I retro-fit them
> back into the Unselect methods?

I'd probably prefer that happen some day, but fixing them doesn't seem terribly urgent.

> re: if (control) { ... I don't think you need to add this check ...
> 
> Again, this is how the Unselect() function currently is coded, so I sought
> uniformity.

consistantly wrong is worse than sometimes right and inconsnistant.

> If you are flexible with your thoughts here, I'd like to leave the patch as
> I now have it. If you strongly disagree, then I'd feel better at least
> making consistent changes for both the Select() and Unselect() methods.

why? what's wrong with just fixing the current issue the best way possiblew?  THere are certainly inconsistant things, but if you try and fix them all at once you'll never get it done.  If you really can't stand the thought of a little bit of inconsistancy I guess I don't mind fixing them now since its small, and sort of maybe a little related, but that seems like enough change I'd probably get it reviewed again, were as if you just change these methods I wouldn't bother.

Comment 7

[Mark Capella [:capella]]

Yah ok, stuck in the mud again. Let me focus on something else for a bit.

Comment 8

[Mark Capella [:capella]]

Ok, got the build problems w/ bug 763212 fixed, so I'm able to build / test again ... changed / tested just those methods, and pushing to TRY again to be safe

https://tbpl.mozilla.org/?tree=Try&rev=4f92a8fead85

Comment 9

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4c34e1a4cbb5

Comment 10

[Ed Morley [:emorley]]

Unfortunately this appears to have caused:

Regression :( SVG increase 26.9% on Linux x64 Mozilla-Inbound-Non-PGO
---------------------------------------------------------------------
    Previous: avg 3573.097 stddev 6.190 of 30 runs up to revision 61fd66629c4f
    New     : avg 4535.128 stddev 12.532 of 5 runs since revision 4c34e1a4cbb5
    Change  : +962.031 (26.9% / z=155.408)
    Graph   : http://mzl.la/N2bejZ 

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/QuabFfbyLAo

Comment 11

[Ed Morley [:emorley]]

Sorry, believe that was actually caused by bug 539356 looking at the graphs for other platforms.

Comment 12

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/4c34e1a4cbb5