Closed Bug 1001641 Opened 8 years ago Closed 7 years ago

Provide equivalent support for MathML as WebKit for NSAccessibility

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jwei, Assigned: fredw)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: user-doc-needed)

Attachments

(1 file, 7 obsolete files)

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.
Attached patch MathML NSAccessibility WIP (obsolete) — Splinter Review
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).
You might want to take a look at

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

for some MathML accessibility tests.
Attached patch MathML NSAccessibility WIP (obsolete) — Splinter Review
Unbitrotting...
Attachment #8532959 - Attachment is patch: true
No longer depends on: 1001634
No longer depends on: 1001635
Depends on: 1001637
Attached patch MathML NSAccessibility WIP (obsolete) — Splinter Review
Unbitrot
Attachment #8532959 - Attachment is obsolete: true
Blocks: 1109022
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.
@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.
Flags: needinfo?(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.
Flags: needinfo?(jwei)
No longer blocks: 1109022
Depends on: 1001635
No longer depends on: 1001637
Attachment #8557468 - Attachment is obsolete: true
Depends on: 1175269
Attached patch Patch (obsolete) — Splinter Review
This adds the Mac attributes without relying on MathML relations.
Attached patch patch (obsolete) — Splinter Review
Attachment #8412917 - Attachment is obsolete: true
Attachment #8624476 - Attachment is obsolete: true
Attached patch Patch V3 (obsolete) — Splinter Review
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.
Attachment #8624490 - Attachment is obsolete: true
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
Flags: needinfo?(mzehe)
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.
Flags: needinfo?(mzehe)
(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?
Flags: needinfo?(mzehe)
(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...
Try build 1 causes VoiceOver to crash, Build 2 does not.
Flags: needinfo?(mzehe)
Attached patch Patch V4 (obsolete) — Splinter Review
Assignee: jwei → fred.wang
Attachment #8624508 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8624967 - Flags: review?(surkov.alexander)
Keywords: user-doc-needed
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
Attachment #8624967 - Flags: review?(surkov.alexander) → review+
Blocks: 1176970
Blocks: 1176973
Blocks: 1176983
(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 :-(
Attached patch Patch V5Splinter Review
Attachment #8624967 - Attachment is obsolete: true
Attachment #8625604 - Flags: review?(surkov.alexander)
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
Attachment #8625604 - Flags: review?(surkov.alexander) → review+
(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".
Blocks: 744790
https://hg.mozilla.org/mozilla-central/rev/7caec22daf30
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: osxa11y
You need to log in before you can comment on or make changes to this bug.