Bug 139582 – AX: Anonymous RenderMathMLOperators are not exposed to the accessibility tree

• Component: Accessibility
• Status: RESOLVED FIXED

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

If we compare

<mfenced open="(" separators="," close=")"><mi>x</mi><mi>y</mi></mfenced>

and

<mrow><mo>(</mo><mi>x</mi><mo>,</mo><mi>y</mi><mo>)</mo></mrow>

we see that the render trees only differ by the fact that RenderMathMLOperator are anonymous in the former case but not in the latter:

  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584
      RenderBlock (anonymous) at (0,0) size 784x16
        RenderMathMLMath {math} at (0,0) size 56x16 [padding: 0 1 0 1]
          RenderMathMLFenced {mfenced} at (1,0) size 54x16
            RenderMathMLOperator (anonymous) at (0,0) size 14x16
              RenderMathMLBlock (anonymous, flex) at (0,0) size 6x16
                RenderBlock (anonymous) at (0,0) size 6x16
                  RenderText at (0,-45) size 6x106
                    text run at (0,-45) width 6: "("
            RenderMathMLToken {mi} at (14,5) size 10x8 [padding: 0 2 0 0]
              RenderMathMLBlock (anonymous, flex) at (0,0) size 8x8
                RenderBlock (anonymous) at (0,0) size 8x8
                  RenderText {#text} at (0,-50) size 8x106
                    text run at (0,-50) width 8: "x"
            RenderMathMLOperator (anonymous) at (23,10) size 8x6
              RenderMathMLBlock (anonymous, flex) at (0,0) size 4x6
                RenderBlock (anonymous) at (0,0) size 4x6
                  RenderText at (0,-55) size 4x106
                    text run at (0,-55) width 4: ","
            RenderMathMLToken {mi} at (30,5) size 10x11 [padding: 0 2 0 0]
              RenderMathMLBlock (anonymous, flex) at (0,0) size 8x11
                RenderBlock (anonymous) at (0,0) size 8x11
                  RenderText {#text} at (0,-50) size 8x106
                    text run at (0,-50) width 8: "y"
            RenderMathMLOperator (anonymous) at (39,0) size 15x16
              RenderMathMLBlock (anonymous, flex) at (0,0) size 6x16
                RenderBlock (anonymous) at (0,0) size 6x16
                  RenderText at (0,-45) size 6x106
                    text run at (0,-45) width 6: ")"
        RenderText {#text} at (0,0) size 0x0
      RenderBlock {P} at (0,32) size 784x17
        RenderText {#text} at (0,0) size 175x17
          text run at (0,0) width 175: "-----------------------------------"
      RenderBlock (anonymous) at (0,65) size 784x16
        RenderMathMLMath {math} at (0,0) size 56x16 [padding: 0 1 0 1]
          RenderMathMLRow {mrow} at (1,0) size 54x16
            RenderMathMLOperator {mo} at (0,0) size 14x16
              RenderMathMLBlock (anonymous, flex) at (0,0) size 14x16
                RenderBlock (anonymous) at (0,0) size 6x16
                  RenderText at (0,-45) size 6x106
                    text run at (0,-45) width 6: "("
            RenderMathMLToken {mi} at (14,5) size 10x8 [padding: 0 2 0 0]
              RenderMathMLBlock (anonymous, flex) at (0,0) size 8x8
                RenderBlock (anonymous) at (0,0) size 8x8
                  RenderText {#text} at (0,-50) size 8x106
                    text run at (0,-50) width 8: "x"
            RenderMathMLOperator {mo} at (23,10) size 8x6
              RenderMathMLBlock (anonymous, flex) at (0,0) size 4x6
                RenderBlock (anonymous) at (0,0) size 4x6
                  RenderText at (0,-55) size 4x106
                    text run at (0,-55) width 4: ","
            RenderMathMLToken {mi} at (30,5) size 10x11 [padding: 0 2 0 0]
              RenderMathMLBlock (anonymous, flex) at (0,0) size 8x11
                RenderBlock (anonymous) at (0,0) size 8x11
                  RenderText {#text} at (0,-50) size 8x106
                    text run at (0,-50) width 8: "y"
            RenderMathMLOperator {mo} at (39,0) size 15x16
              RenderMathMLBlock (anonymous, flex) at (0,0) size 14x16
                RenderBlock (anonymous) at (0,0) size 6x16
                  RenderText at (0,-45) size 6x106
                    text run at (0,-45) width 6: ")"

The point of bug 124838 was to refactor the code to get cleaner and more consistent render trees. However, this change broke platform/mac/accessibility/mathml-elements.html

It seems that the code added into AccessibilityRenderObject::isIgnoredElementWithinMathTree to force exposure of anonymous render operators into the accessibility tree didn't help:

if (m_renderer->isAnonymous()) { 
    if (m_renderer->isRenderMathMLOperator()) 
       return false;
    ...

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

In bug 148393, mac/mathml-elements.html was changed to make it pass again. However, it seems that this bug is still present. At least the math-fenced.html test case uploaded in bug 155018 fails for ATK.

I'm extracting the accessibility refactoring from bug 155018 and will upload a smaller patch here. After bug 155018, token elements will stop creating anonymous nodes and more cleanup will be possible.

[Radar WebKit Bug Importer]

<rdar://problem/26938849>

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

Created attachment 281826 [details]
Patch

[Joanmarie Diggs]

Created attachment 281884 [details]
Alternative patch based on Frédéric's

I'm not obsoleting the previous patch without feedback and review. But I am proposing this patch as an alternative/replacement.

Chris: Please review for accessibility.
Frédéric: Please review for Math (and because it's based on your patch. :) )

Thanks to you both!

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

Comment on attachment 281884 [details]
Alternative patch based on Frédéric's

View in context: https://bugs.webkit.org/attachment.cgi?id=281884&action=review

That looks good to me, especially the option to output subroles ;-) I have some doubts on isIgnoredElementWithinMathTree though, see inline comments.

> Source/WebCore/accessibility/AccessibilityObject.h:998
> +    virtual bool isAnonymousMathOperator() const { return false; }

I'm not sure that this function is needed outside AccessibilityRenderObject.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3785
> +    return ancestorsOfType<RenderMathMLMath>(*m_renderer).first();

So IIUC you are now excluding all the non-MathML element inside a <math> tag, which I suspect would break accessibility in some cases, for example:

- The "Fill the gaps in this matrix with resizable input fields." example https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/Basics
- The last SVG image in https://golem.ph.utexas.edu/wiki/instiki/show/Sandbox (warning: on WebKit this page loads MathJax and so does not use native MathML)

Normally, we have similar layout tests for these cases.

I'm also not sure about the performance of ancestorsOfType or of the old for loop. In my patch I tried to optimize that a bit but I suspect we will really only be able to get simple version after bug 155018 where anonymous nodes are either RenderMathMLOperator children of mfenced (to include) or RenderBlock children of a MathML element (to exclude).

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

(In reply to comment #5)
> https://golem.ph.utexas.edu/wiki/instiki/show/Sandbox (warning: on WebKit
> this page loads MathJax and so does not use native MathML)

So LayoutTests/mathml/presentation/semantics.html verifies this use case.

[Joanmarie Diggs]

Comment on attachment 281884 [details]
Alternative patch based on Frédéric's

(In reply to comment #5)
> Comment on attachment 281884 [details]
> Alternative patch based on Frédéric's
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281884&action=review
> 
> That looks good to me, especially the option to output subroles ;-) I have
> some doubts on isIgnoredElementWithinMathTree though, see inline comments.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:998
> > +    virtual bool isAnonymousMathOperator() const { return false; }
> 
> I'm not sure that this function is needed outside AccessibilityRenderObject.

It's not *yet*. I anticipate there may be a need for platforms to be able to check that for something with MathElementRole. But noted.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3785
> > +    return ancestorsOfType<RenderMathMLMath>(*m_renderer).first();
> 
> So IIUC you are now excluding all the non-MathML element inside a <math>
> tag, which I suspect would break accessibility in some cases, for example:

Ugh. As we discussed via chat, I made the clearly bogus assumption that we'd have MathML in MathML. So, yeah.... That will need to change.

> I'm also not sure about the performance of ancestorsOfType or of the old for
> loop.

I think that getting (and potentially creating) accessible objects just to call isMathElement() on ancestors is ideal. So perhaps doing something like your for loop, but working with the RenderObject instances rather than AccessibilityObject instances.

Anyway, based on the clearly bogus oops I made, I'm going to clear the r? flag.

Thanks for the review!

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

Created attachment 281981 [details]
Patch

[Build Bot]

Comment on attachment 281981 [details]
Patch

Attachment 281981 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1562203

New failing tests:
accessibility/math-foreign-content.html
accessibility/mac/mathml-elements.html
accessibility/math-fenced.html

[Build Bot]

Created attachment 281988 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281981 [details]
Patch

Attachment 281981 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1562218

New failing tests:
accessibility/math-foreign-content.html
accessibility/mac/mathml-elements.html
accessibility/math-fenced.html

[Build Bot]

Created attachment 281991 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281981 [details]
Patch

Attachment 281981 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1562231

New failing tests:
accessibility/math-foreign-content.html
accessibility/mac/mathml-elements.html
accessibility/math-fenced.html

[Build Bot]

Created attachment 281994 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joanmarie Diggs]

So this patch is failing now (as would my patch, which it is based on) is due to the recently-landed change to the render tree for mfenced. Because that patch landed first, we now need to also take into account that the RenderText object (which backs the Mac's StaticTextRole accessible object) no longer exists. I'll see about doing that later today (after all, it's my turn in our game of tag-team patches. ;))

[Joanmarie Diggs]

Created attachment 282133 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 282133 [details]
Patch

@Chris: Please review. Thanks!

[chris fleizach]

Comment on attachment 282133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282133&action=review

I think this looks ok, but I think you should address the comment. Just fixed another crash recently where we had the same problem

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3679
> +    return is<RenderMathMLOperator>(*m_renderer) && m_renderer->isAnonymous();

Should be check m_renderer is not null before derefencing it?

[Joanmarie Diggs]

(In reply to comment #18)
> Comment on attachment 282133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282133&action=review
> 
> I think this looks ok, but I think you should address the comment. Just
> fixed another crash recently where we had the same problem

Sorry, I hadn't seen any comment when I asked for review. Will update the patch. Thanks!

[Joanmarie Diggs]

Created attachment 282138 [details]
patch for landing

[chris fleizach]

Comment on attachment 282138 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=282138&action=review

> LayoutTests/ChangeLog:38
> [phone]  Frederic Wang  <[e-mail]>

This line looks out of place

[Joanmarie Diggs]

Comment on attachment 282138 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=282138&action=review

>> LayoutTests/ChangeLog:38
>> [phone]  Frederic Wang  <[e-mail]>
> 
> This line looks out of place

Yeah. Sorry. One of those days....

[Joanmarie Diggs]

Created attachment 282140 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 282140 [details]
patch for landing

Clearing flags on attachment: 282140

Committed r202497: <http://trac.webkit.org/changeset/202497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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

The FIXME comment in AccessibilityRenderObject::textUnderElement from my initial patch has been lost in later iterations, but it still holds. I'll open a follow-up bug to remove the code.