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.

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.

Unbitrotting...

Unbitrot

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.

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

This adds the Mac attributes without relying on MathML relations.

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.

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

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.

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

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

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

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

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