Closed Bug 1001637 Opened 8 years ago Closed 7 years ago

Make math tables implement the nsIAccessibleTable interface

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jwei, Assigned: fredw)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 10 obsolete files)

MathML table elements can be traversed using the same interface as HTMLTableAccessibles.  Support should be added to allow this in the public interface.
Attached patch MathMLTableAccessible WIP (obsolete) — Splinter Review
Added MathMLTableAccessible, MathMLTableRowAccessible, and MathMLTableCellAccessible classes. Inherits from HTMLTableAccessible, HTMLTableRowAccessible, and HTMLTableCellAccessible respectively. Also inherits from MathMLAccessibleBase for the shared functionality - ideally, this should be replaced when there's a better solution.
Switched to utils class for common MathML implementations.
Attachment #8412916 - Attachment is obsolete: true
Attachment #8434645 - Flags: review?(surkov.alexander)
Comment on attachment 8434645 [details] [diff] [review]
MathMLTableAccessible Implementation

Review of attachment 8434645 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

::: accessible/src/generic/MathMLTableAccessible.cpp
@@ +56,5 @@
> +
> +role
> +MathMLTableRowAccessible::NativeRole()
> +{
> +  return MathMLUtils::GetMathMLRole(this);

I guess you can just return a role here, avoiding generic computation (and below)

@@ +100,5 @@
> +uint32_t
> +MathMLTableAccessible::RowCount()
> +{ // If mtable contains elements that aren't mtr or mlabeledtr, it's an error
> +  // and the DOM breaks the rest of the markup off from the <math> element.
> +  return mChildren.Length();

so HTMLTableAccessible::RowCount() is only one thing that doesn't work?

::: accessible/src/generic/MathMLTableAccessible.h
@@ +67,5 @@
> +  virtual mozilla::a11y::role NativeRole() MOZ_OVERRIDE;
> +  virtual Relation RelationByType(RelationType aType) MOZ_OVERRIDE;
> +
> +  // TableAccessible
> +  virtual uint32_t RowCount();

MOZ_OVERRIDE

::: accessible/src/html/HTMLTableAccessible.cpp
@@ +153,5 @@
>    Accessible* parent = const_cast<HTMLTableCellAccessible*>(this);
>    while ((parent = parent->Parent())) {
>      roles::Role role = parent->Role();
> +    if (role == roles::TABLE || role == roles::TREE_TABLE ||
> +        role == roles::MATHML_TABLE)

shouldn't we switch to IsTable()
Attachment #8434645 - Flags: review?(surkov.alexander) → review+
unbitrotting....
Attachment #8532958 - Attachment is patch: true
No longer depends on: 1001634
Depends on: 1001635
Blocks: 1001641
Unbitrot
Attachment #8532958 - Attachment is obsolete: true
Blocks: 1109022
Trying to unbirot again, but we even more deviated from the original patch. The tests are ignored since mathml.js is no longer here.
No longer blocks: 1109022
Attachment #8557467 - Attachment is obsolete: true
Attachment #8574900 - Attachment is obsolete: true
Blocks: 1001635
No longer depends on: 1001635
Assignee: jwei → fred.wang
No longer blocks: 1001641
Depends on: 1001634
Attachment #8576318 - Attachment is obsolete: true
OK, I've extracted what I think is the minimal patch to pass test_MathMLSpec.html added in bug 1001634, Jonathan's mochitest for mtable attached to this bug and what is needed for ATK/Orca to navigate in mtable.
Attachment #8576962 - Attachment is obsolete: true
Attachment #8577145 - Flags: review?(surkov.alexander)
I suggest to reuse HTML table classes directly (no MathML specific classes)
Attachment #8577145 - Attachment is obsolete: true
Attachment #8577145 - Flags: review?(surkov.alexander)
Attachment #8577333 - Flags: review?(surkov.alexander)
Comment on attachment 8577333 [details] [diff] [review]
MathMLTableAccessible Implementation

Review of attachment 8577333 [details] [diff] [review]:
-----------------------------------------------------------------

about testing, why you didn't use table.js (it looks like testTableIndexes and testTableStruct would make a trick)

::: accessible/base/nsAccessibilityService.cpp
@@ +201,5 @@
>    { return new HTMLProgressMeterAccessible(aContent, aContext->Document()); }
>  
>  Accessible*
> +New_HTMLTableAccessible(nsIContent* aContent, Accessible* aContext)
> +  { return new HTMLTableAccessible(aContent, aContext->Document()); }

pls make these static

::: accessible/html/HTMLTableAccessible.cpp
@@ +61,5 @@
>  HTMLTableCellAccessible::NativeRole()
>  {
> +  if (mContent->IsMathMLElement(nsGkAtoms::mtd_)) {
> +    return roles::MATHML_CELL;
> +  }

ok, hopefully, we do something nicer one day

@@ +430,5 @@
>  {
>    nsCOMPtr<nsIPersistentProperties> attributes =
>      AccessibleWrap::NativeAttributes();
> +
> +  if (mContent->IsMathMLElement(nsGkAtoms::mtable_) && HasOwnContent()) {

you don't really need HasOwnContent()?
(In reply to alexander :surkov from comment #13)
> about testing, why you didn't use table.js (it looks like testTableIndexes
> and testTableStruct would make a trick)
> 

I actually just copied Jonathan's tests as they were and didn't try to modify them. I'll see if this can be simplified a bit.

> @@ +430,5 @@
> >  {
> >    nsCOMPtr<nsIPersistentProperties> attributes =
> >      AccessibleWrap::NativeAttributes();
> > +
> > +  if (mContent->IsMathMLElement(nsGkAtoms::mtable_) && HasOwnContent()) {
> 
> you don't really need HasOwnContent()?

I think I copied that from HyperTextAccessible::NativeAttributes but was not really sure whether it's necessary...
(In reply to Frédéric Wang (:fredw) from comment #14)
> (In reply to alexander :surkov from comment #13)
> > about testing, why you didn't use table.js (it looks like testTableIndexes
> > and testTableStruct would make a trick)
> > 
> 
> I actually just copied Jonathan's tests as they were and didn't try to
> modify them. I'll see if this can be simplified a bit.

thanks, you might need to change those methods to work with mathml roles but otherwise it should simplify things

> > @@ +430,5 @@
> > 
> > you don't really need HasOwnContent()?
> 
> I think I copied that from HyperTextAccessible::NativeAttributes but was not
> really sure whether it's necessary...

I think that code is shared with document accessible which may not have mContent, tables should always have content
Comment on attachment 8577333 [details] [diff] [review]
MathMLTableAccessible Implementation

> HTMLTableRowAccessible::NativeRole()
> {
>+  if (mContent->IsAnyOfMathMLElements(nsGkAtoms::mtr_,
>+                                      nsGkAtoms::mlabeledtr_)) {
>+    return GetAccService()->MarkupRole(mContent);
>+  }

it'd be much easier to understand if you just returned the right role. not to mention faster.

> HTMLTableAccessible::NativeAttributes()
> {
>   nsCOMPtr<nsIPersistentProperties> attributes =
>     AccessibleWrap::NativeAttributes();
>+
>+  if (mContent->IsMathMLElement(nsGkAtoms::mtable_) && HasOwnContent()) {
>+    GetAccService()->MarkupAttributes(mContent, attributes);
>+  }

same idea here.
(In reply to Trevor Saunders (:tbsaunde) from comment #16)

> >+  if (mContent->IsAnyOfMathMLElements(nsGkAtoms::mtr_,
> >+                                      nsGkAtoms::mlabeledtr_)) {
> >+    return GetAccService()->MarkupRole(mContent);
> >+  }
> 
> it'd be much easier to understand if you just returned the right role. not
> to mention faster.

yes but that keeps us a bit closer to markup maps usage, it's up to you I'd say

> > HTMLTableAccessible::NativeAttributes()

> >+  if (mContent->IsMathMLElement(nsGkAtoms::mtable_) && HasOwnContent()) {
> >+    GetAccService()->MarkupAttributes(mContent, attributes);
> >+  }
> 
> same idea here.

that'd be unnecessary dupe of markup map, so please don't do this
(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> 
> > >+  if (mContent->IsAnyOfMathMLElements(nsGkAtoms::mtr_,
> > >+                                      nsGkAtoms::mlabeledtr_)) {
> > >+    return GetAccService()->MarkupRole(mContent);
> > >+  }
> > 
> > it'd be much easier to understand if you just returned the right role. not
> > to mention faster.
> 
> yes but that keeps us a bit closer to markup maps usage, it's up to you I'd
> say

not using the map is a *good* thing.

> > > HTMLTableAccessible::NativeAttributes()
> 
> > >+  if (mContent->IsMathMLElement(nsGkAtoms::mtable_) && HasOwnContent()) {
> > >+    GetAccService()->MarkupAttributes(mContent, attributes);
> > >+  }
> > 
> > same idea here.
> 
> that'd be unnecessary dupe of markup map, so please don't do this

I'm pretty sure you could then remove it from the map which'd be nice.
Summary: Add MathMLTableAccessible and related classes → Make math tables implement the nsIAccessible interface
After bug 1001634, the MathML elements use a map so this does not make thing worse anyway. I'll change HTMLTableRowAccessible::NativeRole but it does not seem wise to duplicate nsAccessibilityService::MarkupAttributes. For now, my main concern is to have the math tables implement the nsIAccessibleTable interface so that Orca can use it. We'll see later whether we really need to expose these mtable attributes but I just want to do the minimal changes without breaking the current tests.
Summary: Make math tables implement the nsIAccessible interface → Make math tables implement the nsIAccessibleTable interface
yep, sounds good
Attachment #8577333 - Attachment is obsolete: true
Attachment #8577333 - Flags: review?(surkov.alexander)
Comment on attachment 8579518 [details] [diff] [review]
Make math tables implement the nsIAccessibleTable interface

Review of attachment 8579518 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/html/HTMLTableAccessible.cpp
@@ +489,5 @@
>  {
> +  if (mContent->IsMathMLElement(nsGkAtoms::mtable_)) {
> +    // If mtable contains elements that aren't mtr or mlabeledtr, it's an error
> +    // and the DOM breaks the rest of the markup off from the <math> element.
> +    return mChildren.Length();

I'm curious if we need this and if we need then if it's enough. It's not covered by tests. Maybe we should drop this for now.

::: accessible/tests/mochitest/table.js
@@ +53,5 @@
>  
>    // Test table accessible tree.
>    var tableObj = {
> +    role: (aMathRowsArray ? ROLE_MATHML_TABLE :
> +           (aIsTreeTable ? ROLE_TREE_TABLE : ROLE_TABLE)),

I would do something like
kTable = 0;
kTreeTable = 1;
kMathTable = 2;
and renamed aIsTreeTable to aTableType

and then you can renamve aMathRowsArray to aRowRoles

::: accessible/tests/mochitest/table/a11y.ini
@@ +8,5 @@
>  [test_indexes_listbox.xul]
>  [test_indexes_table.html]
>  [test_indexes_tree.xul]
>  [test_layoutguess.html]
> +[test_mathml.html]

I prefer to have it split into the test structure like test_indexes_mtable.html and test_struct_mtable.html but I'm ok either way

::: accessible/tests/mochitest/table/test_mathml.html
@@ +47,5 @@
> +        ROLE_MATHML_TABLE_ROW,
> +        ROLE_MATHML_TABLE_ROW,
> +        ROLE_MATHML_TABLE_ROW
> +      ];
> +      testTableIndexes("complex", idxes);

nit: pls move it after indexes variable
Attachment #8579518 - Flags: review+
(In reply to alexander :surkov from comment #22)
> @@ +489,5 @@
> >  {
> > +  if (mContent->IsMathMLElement(nsGkAtoms::mtable_)) {
> > +    // If mtable contains elements that aren't mtr or mlabeledtr, it's an error
> > +    // and the DOM breaks the rest of the markup off from the <math> element.
> > +    return mChildren.Length();
> 
> I'm curious if we need this and if we need then if it's enough. It's not
> covered by tests. Maybe we should drop this for now.

@Jonathan: do you remember what was the rationale for this?
Flags: needinfo?(jwei)
(In reply to Frédéric Wang (:fredw) from comment #23)
> (In reply to alexander :surkov from comment #22)
> > @@ +489,5 @@
> > >  {
> > > +  if (mContent->IsMathMLElement(nsGkAtoms::mtable_)) {
> > > +    // If mtable contains elements that aren't mtr or mlabeledtr, it's an error
> > > +    // and the DOM breaks the rest of the markup off from the <math> element.
> > > +    return mChildren.Length();
> > 
> > I'm curious if we need this and if we need then if it's enough. It's not
> > covered by tests. Maybe we should drop this for now.
> 
> @Jonathan: do you remember what was the rationale for this?

This should probably be removed. It was meant to be a quick short-circuit to avoid extra computation in the case that the associated table was an <mtable>, but the majority of calls to RowCount will be regular ones anyway, leaving in an extra call any time the function is called. It doesn't appear to be exactly correct, either, since regular <table> elements (<td>, <tbody>, etc.) appear to be accepted in an <mtable>, with only HTML elements invalid in that scope (say, <p>) being omitted.
Flags: needinfo?(jwei)
Attachment #8579518 - Attachment is obsolete: true
Attachment #8579866 - Flags: review?(surkov.alexander)
Comment on attachment 8579866 [details] [diff] [review]
Make math tables implement the nsIAccessibleTable interface

Review of attachment 8579866 [details] [diff] [review]:
-----------------------------------------------------------------

it looks comments were addressed, so r+ from previous patch
Attachment #8579866 - Flags: review?(surkov.alexander) → review+
Attachment #8434645 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/be4f8924a7ce
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.