decomtaminate Select Row / Column() on accessible tables

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: capella)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bu][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
similar approach to bug 757504
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 631252 [details] [diff] [review]
Patch (v1)

Asking feedback from mentor ...
Attachment #631252 - Flags: feedback?(trev.saunders)

Comment 2

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

one review is enough for this bug
Attachment #631252 - Flags: feedback?(trev.saunders) → review?(trev.saunders)
(Reporter)

Comment 3

5 years ago
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.
Attachment #631252 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
TRY push
https://tbpl.mozilla.org/?tree=Try&rev=049e05386514
(Reporter)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Yah ok, stuck in the mud again. Let me focus on something else for a bit.
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Comment 9

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4c34e1a4cbb5
Target Milestone: --- → mozilla16
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
Sorry, believe that was actually caused by bug 539356 looking at the graphs for other platforms.
https://hg.mozilla.org/mozilla-central/rev/4c34e1a4cbb5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.