Last Comment Bug 835121 - ARIA grid should be editable by default
: ARIA grid should be editable by default
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: alexander :surkov
:
Mentors:
Depends on: 857936
Blocks: aria
  Show dependency treegraph
 
Reported: 2013-01-27 02:13 PST by alexander :surkov
Modified: 2013-04-10 18:25 PDT (History)
4 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.81 KB, patch)
2013-02-02 02:34 PST, alexander :surkov
no flags Details | Diff | Review
patch2 (9.61 KB, patch)
2013-02-11 20:23 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description alexander :surkov 2013-01-27 02:13:30 PST
Spec says (http://www.w3.org/TR/wai-aria/roles#grid):

"A grid is considered editable unless otherwise specified. To make a grid read-only, set the aria-readonly attribute of the grid to true. The value of the grid element's aria-readonly attribute is implicitly propagated to all of its owned gridcell elements, and will be exposed through the accessibility API. An author may override an individual gridcell element's propagated aria-readonly value by setting the aria-readonly attribute on the gridcell."

I wonder how useful the editable state on grid elements is, in other words I wonder whether editable state on cells is enough.

Jamie, what will you say?
Comment 1 James Teh [:Jamie] 2013-01-28 18:50:02 PST
I don't really have a preference here. For the most part, it doesn't really matter. My gut feeling is that the grid probably should have the editable state, but I can't really justify that. I guess the grid having the editable state suggests that the grid itself is editable (i.e. you can add cells to it, etc.; e.g. a spreadsheet), whereas the editable state on a cell only suggests that particular cell is editable (i.e. its content).
Comment 2 alexander :surkov 2013-01-28 19:22:21 PST
Oh, that makes sense. 

Do you think should editable state be propagated to each cell from a grid until the cell overrides it?
Comment 3 James Teh [:Jamie] 2013-01-28 19:23:09 PST
(In reply to alexander :surkov from comment #2)
> Do you think should editable state be propagated to each cell from a grid
> until the cell overrides it?
Yes.
Comment 4 alexander :surkov 2013-02-02 02:34:46 PST
Created attachment 709366 [details] [diff] [review]
patch
Comment 5 alexander :surkov 2013-02-02 02:41:01 PST
related ARIA failure tracking URL: https://wiki.mozilla.org/Accessibility/ARIA1.0TestSuiteFailures#182
Comment 6 steve faulkner 2013-02-02 03:14:36 PST
Hi alex, does this mean that when role=grid is used as it sometimes is for creating read onlt data tables out of arbitrary elements, the cells will be editbale by default?
Comment 7 alexander :surkov 2013-02-02 03:52:45 PST
(In reply to steve faulkner from comment #6)
> Hi alex, does this mean that when role=grid is used as it sometimes is for
> creating read onlt data tables out of arbitrary elements, the cells will be
> editbale by default?

yes, is that not expected?
Comment 8 Trevor Saunders (:tbsaunde) 2013-02-02 06:01:22 PST
Comment on attachment 709366 [details] [diff] [review]
patch

>+  if (mRoleMapEntry->Is(nsGkAtoms::gridcell) &&
>+      !(*aState & (states::READONLY | states::EDITABLE))) {
>+    TableCellAccessible* cell = const_cast<Accessible*>(this)->AsTableCell();

why not add const TablecellAccessible* AsTableCell() const; ?
Comment 9 alexander :surkov 2013-02-02 06:19:29 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Comment on attachment 709366 [details] [diff] [review]
> patch
> 
> >+  if (mRoleMapEntry->Is(nsGkAtoms::gridcell) &&
> >+      !(*aState & (states::READONLY | states::EDITABLE))) {
> >+    TableCellAccessible* cell = const_cast<Accessible*>(this)->AsTableCell();
> 
> why not add const TablecellAccessible* AsTableCell() const; ?

would that work? The problem here is this is const because we are in ApplyARIAState() but AsTableCell() is not const.
Comment 10 Trevor Saunders (:tbsaunde) 2013-02-02 06:41:13 PST
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > Comment on attachment 709366 [details] [diff] [review]
> > patch
> > 
> > >+  if (mRoleMapEntry->Is(nsGkAtoms::gridcell) &&
> > >+      !(*aState & (states::READONLY | states::EDITABLE))) {
> > >+    TableCellAccessible* cell = const_cast<Accessible*>(this)->AsTableCell();
> > 
> > why not add const TablecellAccessible* AsTableCell() const; ?
> 
> would that work? The problem here is this is const because we are in
> ApplyARIAState() but AsTableCell() is not const.

I'm suggesting adding a AsTableCell() that is const which you should be able to do so long as it returns a const TableCellAccessible*
Comment 11 alexander :surkov 2013-02-02 08:02:15 PST
I see, misread you
Comment 12 steve faulkner 2013-02-02 08:31:32 PST
(In reply to alexander :surkov from comment #7)
> (In reply to steve faulkner from comment #6)
> > Hi alex, does this mean that when role=grid is used as it sometimes is for
> > creating read onlt data tables out of arbitrary elements, the cells will be
> > editbale by default?
> 
> yes, is that not expected?

ignore me :-) , I pinged Rich offline to clarify.
Comment 13 alexander :surkov 2013-02-02 19:28:56 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> why not add const TablecellAccessible* AsTableCell() const; ?

that means two independent implementations of AsTableCell() or are you fine to have a second version as inline thing of the first one using const_cast again?
Comment 14 alexander :surkov 2013-02-08 00:52:08 PST
ping?
Comment 15 Trevor Saunders (:tbsaunde) 2013-02-11 13:32:01 PST
(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > why not add const TablecellAccessible* AsTableCell() const; ?
> 
> that means two independent implementations of AsTableCell() or are you fine
> to have a second version as inline thing of the first one using const_cast
> again?

personally I'd probably just provide seperate impl afterall its a trivial method, but either's fine with me I think.
Comment 16 alexander :surkov 2013-02-11 20:23:01 PST
Created attachment 712791 [details] [diff] [review]
patch2
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-02-14 14:24:42 PST
https://hg.mozilla.org/mozilla-central/rev/55a453689fb1

Note You need to log in before you can comment on or make changes to this bug.