Closed
Bug 1001637
Opened 11 years ago
Closed 10 years ago
Make math tables implement the nsIAccessibleTable interface
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
(deleted),
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
MathML table elements can be traversed using the same interface as HTMLTableAccessibles. Support should be added to allow this in the public interface.
Reporter | ||
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•10 years ago
|
||
Switched to utils class for common MathML implementations.
Attachment #8412916 -
Attachment is obsolete: true
Attachment #8434645 -
Flags: review?(surkov.alexander)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
unbitrotting....
Assignee | ||
Updated•10 years ago
|
Attachment #8532958 -
Attachment is patch: true
Assignee | ||
Comment 6•10 years ago
|
||
Trying to unbirot again, but we even more deviated from the original patch. The tests are ignored since mathml.js is no longer here.
Assignee | ||
Updated•10 years ago
|
Attachment #8557467 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8574900 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8576318 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
I suggest to reuse HTML table classes directly (no MathML specific classes)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8577145 -
Attachment is obsolete: true
Attachment #8577145 -
Flags: review?(surkov.alexander)
Attachment #8577333 -
Flags: review?(surkov.alexander)
Comment 13•10 years ago
|
||
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()?
Assignee | ||
Comment 14•10 years ago
|
||
(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...
Comment 15•10 years ago
|
||
(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 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
(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
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Summary: Add MathMLTableAccessible and related classes → Make math tables implement the nsIAccessible interface
Assignee | ||
Comment 19•10 years ago
|
||
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
Comment 20•10 years ago
|
||
yep, sounds good
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8577333 -
Attachment is obsolete: true
Attachment #8577333 -
Flags: review?(surkov.alexander)
Comment 22•10 years ago
|
||
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+
Assignee | ||
Comment 23•10 years ago
|
||
(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)
Reporter | ||
Comment 24•10 years ago
|
||
(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)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8579518 -
Attachment is obsolete: true
Attachment #8579866 -
Flags: review?(surkov.alexander)
Comment 26•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8434645 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Keywords: checkin-needed
Comment 28•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•