1001641 - Provide equivalent support for MathML as WebKit for NSAccessibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jonathan Wei [:jwei]]

Use the existing WebKit accessibility code as a guide to determine exactly how VoiceOver expects the MathML to be laid out, and convert the generic MathML accessibility code to output expected values.  The end result should have equivalent VoiceOver functionality for MathML on both Safari and Firefox, barring rendering engine differences.

Comment 1

[Jonathan Wei [:jwei]]

Attachment: MathML NSAccessibility WIP

Added most of the equivalent functionality as WebKit for OS X accessibility. Still need to add mmultiscripts support as well as mfenced open/close/separator operators (once those are added to the accessible tree).

Comment 2

[Frédéric Wang (:fredw)]

You might want to take a look at

http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/accessibility/

for some MathML accessibility tests.

Comment 3

[Frédéric Wang (:fredw)]

Attachment: MathML NSAccessibility WIP

Unbitrotting...

Comment 4

[Frédéric Wang (:fredw)]

Attachment: MathML NSAccessibility WIP

Unbitrot

Comment 5

[Frédéric Wang (:fredw)]

Note: I made this bug blocking 1109022 since that's the order current patches apply. I believe potential merge conflicts are only for accessible/base/RoleMap.h, though. As I see this patch is still WIP, so if we end up taking the ATK implementation first, then we could refresh the patches / update the bug dependencies.

Comment 6

[Frédéric Wang (:fredw)]

@Jonathan: I'm also curious to know what are you plans for the NSAccessibility. In particular, if the work is unlikely to be finished soon, we should consider taking the ATK patches first, as said in comment 5.

Comment 7

[Jonathan Wei [:jwei]]

(In reply to Frédéric Wang (:fredw) from comment #6)
> @Jonathan: I'm also curious to know what are you plans for the
> NSAccessibility. In particular, if the work is unlikely to be finished soon,
> we should consider taking the ATK patches first, as said in comment 5.

It probably makes sense to merge the patch for bug 1109022 first since it's already ready to go. As mentioned in bug 1001634 comment 30, it'll be a while until I can properly devote time to this series of patches.

With regards to overall plans, a test suite will need to be added before it's ready. In addition, missing sections in the functionality will need to be implemented or have placeholders to be filled in at a future time - the tests linked in comment 2 will help quite a bit for determining those sections.

Comment 8

[Frédéric Wang (:fredw)]

Attachment: Patch

This adds the Mac attributes without relying on MathML relations.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 10

[Frédéric Wang (:fredw)]

Attachment: Patch V3

Some comments:
- VoiceOver seems to read the munder/mover/munderover elements incorrectly, so that's not an issue with this patch.
- The accessible tree exposed for <mfenced> and <mo> support is still not exactly the same as in WebKit mac, but this seems broken in Apple's anyway and I don't think it will be so important.
- Apple's latest features (mmultiscripts and NSAccessibilityMathLineThicknessAttribute) are not provided by this patch either.

Comment 11

[Frédéric Wang (:fredw)]

So I basically tested on Alex's mac and adding the attributes from this patch seems enough to provide basic support. The roots, fraction, and sub/sup scripts seem to be read correctly by VoiceOver. The under/over scripts are exposed correctly but VoiceOver does not seem to read it correctly in Gecko or WebKit anyway.

@Marco: Can you please try again https://bug1175269.bugzilla.mozilla.org/attachment.cgi?id=8624236 with the following try build and tell if you get better results (in particular do you still get a crash?):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3b6ad156790

Comment 12

[Marco Zehe (:MarcoZ)]

This reads much much better with VoiceOver now! I'd say we're on par with Safari. The only thing remaining is a crash in VoiceOver when I get to the last item. Number 23, where fenced is mentioned, as soon as I VoiceOver-arrow onto the MathML part of that, VoiceOver crashes and restarts itself.

Comment 13

[Frédéric Wang (:fredw)]

(In reply to Marco Zehe (:MarcoZ) from comment #12)
> The only thing remaining is a crash in VoiceOver when I get to the
> last item. Number 23, where fenced is mentioned, as soon as I
> VoiceOver-arrow onto the MathML part of that, VoiceOver crashes and restarts
> itself.

Thanks. So as discussed with Alex yesterday, my suspicion is that exposing <mfence> as subrole AXMathFence without implementing the whole associated AXMathSeparatorOperator/AXMathFenceOperator/AXMathAXMathFencedOpen/AXMathFencedClose interface makes VoiceOver crash (not Gecko). For some reason, we could not reproduce the crash on Alex's mac yesterday. I guess for now it is best to just expose <mfenced> as AXMathRow until we align on what VoiceOver expects (or until Apple fixes the crash).

So just to be sure, I submitted two try builds:

1) https://treeherder.mozilla.org/#/jobs?repo=try&revision=55a3dd35cac8 is the same as the first patch of bug 1175269, but this time it includes Alex's patch for bug 1150510.

2) https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e65f422018e is the same, but with subrole AXMathFence replaced with subrole AXMathRow.

Can you please try 1) and 2) when they are ready and tell how they behave for item 23?

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Marco Zehe (:MarcoZ) from comment #12)
> This reads much much better with VoiceOver now! I'd say we're on par with
> Safari.

Yes, I guess more or less at least for the most important MathML constructs. During the Whistler work week, I plan to open follow-up bugs describing what is missing here. Also, I plan to write more MathML testcases and compare how Firefox Nightly reads them using VoiceOver, NVDA+MathPlayer and Orca (with Joanie's improvements), so that we can continue improving things...

Comment 15

[Marco Zehe (:MarcoZ)]

Try build 1 causes VoiceOver to crash, Build 2 does not.

Comment 16

[Frédéric Wang (:fredw)]

Attachment: Patch V4

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8624967 [details] [diff] [review]
Patch V4

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

r=me with comments addressed

::: accessible/mac/mozAccessible.mm
@@ +136,5 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NO);
>  }
>  
> +- (NSArray*)additionalAccessibilityAttributeNames

shoudln't it be in header?

@@ +138,5 @@
>  }
>  
> +- (NSArray*)additionalAccessibilityAttributeNames
> +{
> +  NSMutableArray *additional = [NSMutableArray array];

nit: type* name, here and below

@@ +144,5 @@
> +    case roles::MATHML_ROOT:
> +      [additional addObject:NSAccessibilityMathRootIndexAttribute];
> +    case roles::MATHML_SQUARE_ROOT:
> +      [additional addObject:NSAccessibilityMathRootRadicandAttribute];
> +      break;

nit: it would be probably more readable if you kept these two cases separate.

@@ +211,5 @@
>  #endif
>                                                             nil];
>    }
>  
> +  NSArray *objectAttributes = generalAttributes;

you can use generalAttributes instead, right?

@@ +212,5 @@
>                                                             nil];
>    }
>  
> +  NSArray *objectAttributes = generalAttributes;
> +  NSArray *additionalAttributes = [self additionalAccessibilityAttributeNames];

if this method is not supposed to get virtual then I think I would prefer to copy its body here

@@ +311,5 @@
> +      return [self childAt:0];
> +    if ([attribute isEqualToString:NSAccessibilityMathSubscriptAttribute])
> +      return [self childAt:1];
> +    if ([attribute isEqualToString:NSAccessibilityMathSuperscriptAttribute])
> +      return nil;

for things like this this will return nil anyway, even having no this string check. If it stands for to not log then this part should be under DEBUG too.

@@ +333,5 @@
> +  case roles::MATHML_UNDER:
> +    if ([attribute isEqualToString:NSAccessibilityMathBaseAttribute])
> +      return [self childAt:0];
> +    if ([attribute isEqualToString:NSAccessibilityMathUnderAttribute])
> +      return [self childAt:1];

I'm curious if Safari has all these if checks, whether they use maps or something

Comment 18

[Frédéric Wang (:fredw)]

(In reply to alexander :surkov from comment #17)
> > +- (NSArray*)additionalAccessibilityAttributeNames
> 
> shoudln't it be in header?

I'm not sure about the objective C syntax. WebKit's defines this in WebAccessibilityObjectWrapperMac.mm but not in the header...

> nit: type* name, here and below

done.

> nit: it would be probably more readable if you kept these two cases separate.

Done here and in the other switch.

> > +  NSArray *objectAttributes = generalAttributes;
> 
> you can use generalAttributes instead, right?

I'm not sure. Like in the WebKit code, we have a static variable for the attributes and an objectAttributes with additional attributes, that is not static.

> > +  NSArray *objectAttributes = generalAttributes;
> > +  NSArray *additionalAttributes = [self additionalAccessibilityAttributeNames];
> 
> if this method is not supposed to get virtual then I think I would prefer to
> copy its body here

Again, additionalAccessibilityAttributeNames is copied from Apple's code. And as we discussed, we probably want to keep that if we add more of WebKit's attributes.

> for things like this this will return nil anyway, even having no this string
> check. If it stands for to not log then this part should be under DEBUG too.

done

> @@ +333,5 @@
> > +  case roles::MATHML_UNDER:
> > +    if ([attribute isEqualToString:NSAccessibilityMathBaseAttribute])
> > +      return [self childAt:0];
> > +    if ([attribute isEqualToString:NSAccessibilityMathUnderAttribute])
> > +      return [self childAt:1];
> 
> I'm curious if Safari has all these if checks, whether they use maps or
> something

no, they use isEqualToString everywhere :-(

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Patch V5

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 8625604 [details] [diff] [review]
Patch V5

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

you don't need to rerequest review if comments were addressed and changes are minor

Comment 21

[Frédéric Wang (:fredw)]

(In reply to alexander :surkov from comment #20)
> you don't need to rerequest review if comments were addressed and changes
> are minor

Yes, I know. But some comments were not "addressed" but just "Apple does that so let's do it too".

Comment 22

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a333bd9bcccf

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7caec22daf30

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7caec22daf30