Jonathan Wei [:jwei] Reporter
	Description • 10 years ago

Add a class representing presentation MathML elements.  This will replace the current usage of a generic HyperTextAccessible.

	Jonathan Wei [:jwei] Reporter
	Comment 1 • 10 years ago

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.

	Jonathan Wei [:jwei] Reporter
	Comment 2 • 10 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 3 • 10 years ago

> 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.

	alexander :surkov (:asurkov) Assignee
	Comment 4 • 10 years ago

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)?

	alexander :surkov (:asurkov) Assignee
	Comment 5 • 10 years ago

Comment on attachment 8412901 [details] [diff] [review]
MathMLAccessible WIP

cancelling feedback until comment is addressed

	Jonathan Wei [:jwei] Reporter
	Comment 6 • 10 years ago

Switched to utils class for common MathML implementations.

	alexander :surkov (:asurkov) Assignee
	Comment 7 • 10 years ago

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

	neil@parkwaycc.co.uk
	Comment 8 • 10 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 9 • 10 years ago

What's next here? :)

	alexander :surkov (:asurkov) Assignee
	Comment 10 • 10 years ago

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

	Jonathan Wei [:jwei] Reporter
	Comment 11 • 10 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 12 • 10 years ago

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.

	Frédéric Wang (:fredw)
	Comment 13 • 10 years ago

Just unbitrotting the patch...

	Jonathan Wei [:jwei] Reporter
	Comment 14 • 10 years ago

(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?

	Frédéric Wang (:fredw)
	Comment 15 • 10 years ago

@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.

	alexander :surkov (:asurkov) Assignee
	Comment 16 • 10 years ago

(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

	alexander :surkov (:asurkov) Assignee
	Comment 17 • 10 years ago

(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.

	Jonathan Wei [:jwei] Reporter
	Comment 18 • 10 years ago

(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.

	Jonathan Wei [:jwei] Reporter
	Comment 19 • 10 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 20 • 10 years ago

(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.

	Jonathan Wei [:jwei] Reporter
	Comment 21 • 10 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 22 • 10 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 23 • 10 years ago

Here is the idea, but that needs to be properly merged into the original patch.

	Jonathan Wei [:jwei] Reporter
	Comment 25 • 10 years ago

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.

	Frédéric Wang (:fredw)
	Comment 26 • 9 years ago

unbirot

	Frédéric Wang (:fredw)
	Comment 27 • 9 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 28 • 9 years ago

I'll take it if John doesn't mind

	alexander :surkov (:asurkov) Assignee
	Comment 29 • 9 years ago

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.

	Jonathan Wei [:jwei] Reporter
	Comment 30 • 9 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 31 • 9 years ago

(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.

	Marco Zehe (:MarcoZ)
	Comment 32 • 9 years ago

Comment on attachment 8574240 [details] [diff] [review]
patch

OK, get this baby off the ground, then!

	alexander :surkov (:asurkov) Assignee
	Comment 33 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e5a79d25ab

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 34 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b6e5a79d25ab