Closed Bug 1001634 Opened 8 years ago Closed 7 years ago

Add MathMLAccessible class

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jwei, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Add a class representing presentation MathML elements.  This will replace the current usage of a generic HyperTextAccessible.
Blocks: 1001635
Blocks: 1001637
Blocks: 1001641
Attached patch MathMLAccessible WIP (obsolete) — Splinter Review
Added MathMLAccessible. Currently has a base class with static methods providing an implementation to be shared by MathMLAccessible as well as the MathMLTableAccessible added in bug 1001637 which will be inheriting from HTMLTableAccessible, which isn't ideal. With regards to bug 916419 comment 26, all roles are currently still included. In addition, the mfenced operators are still unexposed in the accessible tree, and further investigation needs to be done to determine exactly how to do this.
Comment on attachment 8412901 [details] [diff] [review]
MathMLAccessible WIP

I couldn't find a better solution than having a common base class with static methods that MathMLAccessible and the table-related MathML accessibles inherit from, and was wondering if you could think of a better solution for this.
Attachment #8412901 - Flags: feedback?(surkov.alexander)
> I couldn't find a better solution than having a common base class with
> static methods that MathMLAccessible and the table-related MathML
> accessibles inherit from, and was wondering if you could think of a better
> solution for this.


I forget if I asked before, but can we just have MathMLUtils? its certainly not OOish, but imho that's because OO is incapable of handling this.  either that or the "base class" seem kind of fine on further reflection or atleast good enough I wouldn't worry too much about.
if we don't have nicer than static methods then I agree that utils class might be nicer than inheritance. Could you file a patch that makes the need of the hierarchy evident (mathml table patch iirc)?
Comment on attachment 8412901 [details] [diff] [review]
MathMLAccessible WIP

cancelling feedback until comment is addressed
Attachment #8412901 - Flags: feedback?(surkov.alexander)
Attached patch MathMLAccessible Implementation (obsolete) — Splinter Review
Switched to utils class for common MathML implementations.
Attachment #8412901 - Attachment is obsolete: true
Attachment #8434640 - Flags: review?(surkov.alexander)
Comment on attachment 8434640 [details] [diff] [review]
MathMLAccessible Implementation

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

related bugs for complete overview are bug 1001635 and bug 1001637

Neil, probably you have some ideas how to implement that nicer
Attachment #8434640 - Flags: superreview?(neil)
Comment on attachment 8434640 [details] [diff] [review]
MathMLAccessible Implementation

The only way I could think of improving this would be use tables of atoms. The only built-in way I can think of to achieve this is using a static atom table, but maybe you are already using lists of atoms for some other reason and can copy that code.

>+void
>+MathMLUtils::GetMathMLNativeAttributes(Accessible* aAccessible,
>+                                       nsIPersistentProperties* aAttributes)
>+{
>+  nsAutoString value;
>+  nsIContent* content = aAccessible->GetContent();
>+  role accRole = GetMathMLRole(aAccessible);
>+
>+  // accent
>+  if (content->GetAttr(kNameSpaceID_None, nsGkAtoms::accent_, value) &&
>+      (accRole == roles::MATHML_STYLE ||
>+       accRole == roles::MATHML_OPERATOR ||
>+       accRole == roles::MATHML_OVER ||
>+       accRole == roles::MATHML_UNDER_OVER))
>+    nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::accent_, value);
At the very least you should test the role(s) before the attribute.
A possible idea I have for simplifying this is to have a table of bitmasks i.e.
#define ROLEBIT(role) (1ULL << (role & 63))
for (size_t index = 0; index <= ArrayLength(sMaskList); index++)
  if ((sMaskList[index] & ROLEBIT(accRole)) &&
      content->GetAttr(kNameSpaceID_None, sAttributeList[index], value))
    nsAccUtils::SetAccAttr(aAttributes, sAttributeList[index], value);

>+void
>+MathMLUtils::GetMathMLRelationByType(Accessible* aAccessible,
>+                                     Relation* aRelation,
>+                                     RelationType aType)
>+{
>+}
[Not sure what this is achieving.]

>+bool
>+MathMLUtils::IsMathMLPresentationTag(const nsIAtom* aTag)
>+{
...
>+}
>+
>+role
>+MathMLUtils::GetMathMLRole(Accessible* aAccessible)
>+{
...
>+}
A table lookup might help here too. If you have a 1-1 mapping of presentation tags into roles then a simple table of static atoms in role order starting with <math:math> and ending with <math:msline> would work. Otherwise you would need a list of MathML presentation tags with the appropriate role, or NOTHING if there is a presentation tag with no specific role.
Attachment #8434640 - Flags: superreview?(neil)
Comment on attachment 8434640 [details] [diff] [review]
MathMLAccessible Implementation

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

Jonathan, do you have any time to finish it?

::: accessible/src/base/RoleMap.h
@@ +1060,5 @@
> +     "math",
> +     ATK_ROLE_UNKNOWN,
> +     NSAccessibilityUnknownRole,
> +     0,
> +     IA2_ROLE_UNKNOWN,

that should be equation role

@@ +1068,5 @@
> +     "mathml identifier",
> +     ATK_ROLE_UNKNOWN,
> +     NSAccessibilityUnknownRole,
> +     0,
> +     IA2_ROLE_UNKNOWN,

until we have a good mapping it makes sense to map into generic hypertext role, i.e. do what we currently do (we create HyperTextAccessible for them)

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1035,5 @@
>          newAcc = new EnumRoleAccessible(content, document, roles::DIAGRAM);
>        }
>      } else if (content->IsMathML()){
> +      if (MathMLUtils::IsMathMLPresentationTag(content->Tag()))
> +        newAcc = new MathMLAccessible(content, document);

we could use EnumRole here as well if we are ok to spend extra byte for it, that will save us from long if/else to calculate accessible role

::: accessible/src/generic/Accessible.cpp
@@ +2485,5 @@
>  // Accessible protected
>  ENameValueFlag
>  Accessible::NativeName(nsString& aName)
>  {
> +  if (mContent->IsHTML() || mContent->IsMathML()) {

MathML content can be labelled by HTML?

::: accessible/src/generic/MathMLAccessible.cpp
@@ +19,5 @@
> +
> +MathMLAccessible::MathMLAccessible(nsIContent* aNode, DocAccessible* aDoc) :
> +  HyperTextAccessibleWrap(aNode, aDoc)
> +{
> +  mType = eMathMLType;

this one sounds like a quite generic type

@@ +28,5 @@
> +{
> +  nsCOMPtr<nsIPersistentProperties> attributes =
> +    HyperTextAccessibleWrap::NativeAttributes();
> +
> +  MathMLUtils::GetMathMLNativeAttributes(this, attributes);

rather SetMathMLAttrs() to stay closer to nsAccUtils pattern

@@ +36,5 @@
> +
> +role
> +MathMLAccessible::NativeRole()
> +{
> +  return MathMLUtils::GetMathMLRole(this);

maybe MathMLRoleFor(nsIContent* aElm)?

@@ +43,5 @@
> +Relation
> +MathMLAccessible::RelationByType(RelationType aType)
> +{
> +  Relation rel = HyperTextAccessibleWrap::RelationByType(aType);
> +  MathMLUtils::GetMathMLRelationByType(this, &rel, aType);

maybe SetMathMLRelations()

::: accessible/src/generic/MathMLUtils.h
@@ +11,5 @@
> +
> +/**
> +  * Utility class for an accessible representing a MathML node.
> +  */
> +class MathMLUtils

it doesn't seem to be a big class so it makes sense to merge it with nsAccUtils

::: accessible/tests/mochitest/name/test_mathml.html
@@ +16,5 @@
> +
> +  <script type="application/javascript">
> +    function doTest()
> +    {
> +      testTokenValue("mi", "y");

i'm not sure they have to have subtree, I think AT is supposed to use text interface, like paragraphs, after all they are closer to paragraphs rather than buttons
Attachment #8434640 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #10)

> Jonathan, do you have any time to finish it?

I'll revisit this again tomorrow and apply suggestions from comments. There's a bunch of refactoring to do as code has moved around since I wrote this and the other patches - which is good since I need to re-familiarise myself with the code base.
Thanks for doing this! Sorry it took so long.

Mostly stuff can be easily fixed by removing "src/" from your patches. Also I guess it'd be nice to keep MathMLAccessible classes inside html folder since mathml is part of html5. And pls address Neil's comments.
Attached patch MathMLAccessible Implementation (obsolete) — Splinter Review
Just unbitrotting the patch...
(In reply to Frédéric Wang (:fredw) from comment #13)
> Just unbitrotting the patch...

Thanks for the refresh on this and the other patches.

(In reply to alexander :surkov from comment #10)
> we could use EnumRole here as well if we are ok to spend extra byte for it,
> that will save us from long if/else to calculate accessible role

I'll probably set up a map from the atom to the role enum value. It'll simplify this and MathMLRoleFor. For SetMathMLAttrs, I'll put together a bitmask table for roles to attributes as suggested by Neil.

> MathML content can be labelled by HTML?

Sorry, what do you mean? 

> rather SetMathMLAttrs() to stay closer to nsAccUtils pattern
> maybe MathMLRoleFor(nsIContent* aElm)?
> maybe SetMathMLRelations()

Renamed.

> it doesn't seem to be a big class so it makes sense to merge it with
> nsAccUtils

True. Moved to nsAccUtils.

> i'm not sure they have to have subtree, I think AT is supposed to use text
> interface, like paragraphs, after all they are closer to paragraphs rather
> than buttons

Okay. So would getText() be a better way to test these?
@Jonathan: what is the rationale for exposing the following elements to the accessibility tree?

<mspace> <mphantom> <mpadded> <maligngroup> <malignmark>

After discussion with Joanie, these will probably not be needed for ATK as they are only involved in visual stuff. I see that you are converting them to NSAccessibilityGroupRole role in attachment 8532959 [details] [diff] [review]. Is it something required by the current WebKit Mac implementation? At least <mphantom> is visibility=hidden in WebKit and I've opened bug 1108378 to do the same in Gecko.
Flags: needinfo?(jwei)
(In reply to Frédéric Wang (:fredw) from comment #15)
> @Jonathan: what is the rationale for exposing the following elements to the
> accessibility tree?

let's move discussion to bug 1108378
(In reply to Jonathan Wei [:jwei] from comment #14)

> > MathML content can be labelled by HTML?
> 
> Sorry, what do you mean? 

If I read you right the path you touched now allows to use HTML label for MathML elements, something like
<mo id="m"></mo><label for="m">label</label>

> > i'm not sure they have to have subtree, I think AT is supposed to use text
> > interface, like paragraphs, after all they are closer to paragraphs rather
> > than buttons
> 
> Okay. So would getText() be a better way to test these?

text interface is must have I think, I'm not sure about name, maybe Jamie has answer.
(In reply to Frédéric Wang (:fredw) from comment #15)
> @Jonathan: what is the rationale for exposing the following elements to the
> accessibility tree?

I had included them in case ATs would like to infer author intent from the visual elements, but I guess that's difficult to do generically, and the author shouldn't be using visual layout to change semantic meaning anyway. I'm totally fine with having them removed from the list of MathML presentation elements unless there was a particular reason to keep them.

> I see that you are converting them
> to NSAccessibilityGroupRole role in attachment 8532959 [details] [diff] [review]
> Is it something required by the current WebKit Mac implementation?

That was done for all the MathML accessibles since that's how WebKit exposes them on OS X.
Flags: needinfo?(jwei)
(In reply to alexander :surkov from comment #17)
> If I read you right the path you touched now allows to use HTML label for
> MathML elements, something like
> <mo id="m"></mo><label for="m">label</label>

Hmm. Yeah, that'd be a bit weird. Maybe only call GetNameFromSubtree? If it makes sense to calculate names for the container types based on their children.
(In reply to Jonathan Wei [:jwei] from comment #18)
> I had included them in case ATs would like to infer author intent from the
> visual elements, but I guess that's difficult to do generically, and the
> author shouldn't be using visual layout to change semantic meaning anyway.
> I'm totally fine with having them removed from the list of MathML
> presentation elements unless there was a particular reason to keep them.

OK, would it be possible to modify the accessibility code to not expose these elements at all? Adding visibility=hidden as in bug 1108378 will not work for mspace/mpadded because many reftests rely on the fact that they can have a background color applied.
Flags: needinfo?(jwei)
Blocks: 1109022
(In reply to Frédéric Wang (:fredw) from comment #20)

> OK, would it be possible to modify the accessibility code to not expose
> these elements at all? Adding visibility=hidden as in bug 1108378 will not
> work for mspace/mpadded because many reftests rely on the fact that they can
> have a background color applied.

Yes. They can just not have accessibles created for them at the same point we decide how to construct the accessible.
Flags: needinfo?(jwei)
(In reply to Jonathan Wei [:jwei] from comment #21)
> Yes. They can just not have accessibles created for them at the same point
> we decide how to construct the accessible.

Thank you. I actually did some testing and figured that out yesterday. I think the only special case is for <mphantom> where we also want the descendants not to have any accessible exposed. The patch from bug 1108378 will work for that.
Depends on: 1108378
Here is the idea, but that needs to be properly merged into the original patch.
Attachment #8434640 - Attachment is obsolete: true
Attachment #8534246 - Attachment is obsolete: true
Addressed most of the comments. Added static arrays for mapping content tag to role, and bit-mask table for mapping roles to each attribute. Don't create accessibles for mspace, mphantom, mpadded, malignmentgroup, and malignmark.

TODO: fix mochitests.
Attachment #8532910 - Attachment is obsolete: true
Attachment #8534888 - Attachment is obsolete: true
Attached patch MathMLAccessible Implementation (obsolete) — Splinter Review
unbirot
No longer blocks: 1001637, 1001641, 1109022
Hi Jonathan. I refreshed your patches again (I have not tested yet, so I hope nothing is broken). I saw that you wanted to fix the Mochitest here. Can you indicate the status on your work and when you plan to update the patch? Thanks.
Flags: needinfo?(jwei)
I'll take it if John doesn't mind
Assignee: jwei → surkov.alexander
Flags: needinfo?(jwei)
Attached patch patchSplinter Review
basically this is Jonathan patch, just updated to current approaches. Many mappings can be questionable but that should be ok as first step on the way. I think I'll file follow ups to sort things out.
Attachment #8574240 - Flags: review?(mzehe)
(In reply to Frédéric Wang (:fredw) from comment #27)
> Hi Jonathan. I refreshed your patches again (I have not tested yet, so I
> hope nothing is broken). I saw that you wanted to fix the Mochitest here.
> Can you indicate the status on your work and when you plan to update the
> patch? Thanks.

(In reply to alexander :surkov from comment #28)
> I'll take it if John doesn't mind

Not at all. I had hoped to get this and the other changes finished much earlier, but unfortunately didn't have the chance and won't be able to do so until around April. Apologies again for all the delays.
(In reply to Jonathan Wei [:jwei] from comment #30)
> Not at all. I had hoped to get this and the other changes finished much
> earlier, but unfortunately didn't have the chance and won't be able to do so
> until around April. Apologies again for all the delays.

no worries, I just felt like we need finish the important work you did. You're always welcome back.
Comment on attachment 8574240 [details] [diff] [review]
patch

OK, get this baby off the ground, then!
Attachment #8574240 - Flags: review?(mzehe) → review+
Attachment #8557465 - Attachment is obsolete: true
Attachment #8557465 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/b6e5a79d25ab
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Blocks: 1109022
No longer blocks: 1001635
Blocks: 1001637
Attachment #8557465 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.