Bug 139582 - AX: Anonymous RenderMathMLOperators are not exposed to the accessibility tree
Summary: AX: Anonymous RenderMathMLOperators are not exposed to the accessibility tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords: InRadar
Depends on: 124838 155018
Blocks: 159208 124836 159205
  Show dependency treegraph
 
Reported: 2014-12-12 04:04 PST by Frédéric Wang (:fredw)
Modified: 2016-06-28 03:21 PDT (History)
17 users (show)

See Also:


Attachments
Patch (14.57 KB, patch)
2016-06-22 01:44 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Alternative patch based on Frédéric's (17.77 KB, patch)
2016-06-22 16:41 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (21.74 KB, patch)
2016-06-24 09:04 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.04 MB, application/zip)
2016-06-24 09:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (951.69 KB, application/zip)
2016-06-24 09:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.24 MB, application/zip)
2016-06-24 10:03 PDT, Build Bot
no flags Details
Patch (24.05 KB, patch)
2016-06-27 08:06 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
patch for landing (23.99 KB, patch)
2016-06-27 09:49 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
patch for landing (23.33 KB, patch)
2016-06-27 10:02 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2014-12-12 04:04:44 PST
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;
    ...
Comment 1 Frédéric Wang (:fredw) 2016-06-22 01:22:35 PDT
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.
Comment 2 Radar WebKit Bug Importer 2016-06-22 01:24:45 PDT
<rdar://problem/26938849>
Comment 3 Frédéric Wang (:fredw) 2016-06-22 01:44:12 PDT
Created attachment 281826 [details]
Patch
Comment 4 Joanmarie Diggs (irc: joanie) 2016-06-22 16:41:21 PDT
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!
Comment 5 Frédéric Wang (:fredw) 2016-06-22 21:34:05 PDT
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).
Comment 6 Frédéric Wang (:fredw) 2016-06-22 21:41:38 PDT
(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.
Comment 7 Joanmarie Diggs (irc: joanie) 2016-06-22 22:22:20 PDT
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!
Comment 8 Frédéric Wang (:fredw) 2016-06-24 09:04:14 PDT
Created attachment 281981 [details]
Patch
Comment 9 Build Bot 2016-06-24 09:47:44 PDT
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
Comment 10 Build Bot 2016-06-24 09:47:49 PDT
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
Comment 11 Build Bot 2016-06-24 09:51:22 PDT
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
Comment 12 Build Bot 2016-06-24 09:51:39 PDT
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
Comment 13 Build Bot 2016-06-24 10:03:31 PDT
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
Comment 14 Build Bot 2016-06-24 10:03:36 PDT
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
Comment 15 Joanmarie Diggs (irc: joanie) 2016-06-24 10:05:26 PDT
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. ;))
Comment 16 Joanmarie Diggs (irc: joanie) 2016-06-27 08:06:01 PDT
Created attachment 282133 [details]
Patch
Comment 17 Joanmarie Diggs (irc: joanie) 2016-06-27 08:41:38 PDT
Comment on attachment 282133 [details]
Patch

@Chris: Please review. Thanks!
Comment 18 chris fleizach 2016-06-27 08:53:27 PDT
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?
Comment 19 Joanmarie Diggs (irc: joanie) 2016-06-27 09:13:10 PDT
(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!
Comment 20 Joanmarie Diggs (irc: joanie) 2016-06-27 09:49:21 PDT
Created attachment 282138 [details]
patch for landing
Comment 21 chris fleizach 2016-06-27 09:55:00 PDT
Comment on attachment 282138 [details]
patch for landing

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

> LayoutTests/ChangeLog:38
> +2016-06-27  Frederic Wang  <fwang@igalia.com>

This line looks out of place
Comment 22 Joanmarie Diggs (irc: joanie) 2016-06-27 09:58:29 PDT
Comment on attachment 282138 [details]
patch for landing

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

>> LayoutTests/ChangeLog:38
>> +2016-06-27  Frederic Wang  <fwang@igalia.com>
> 
> This line looks out of place

Yeah. Sorry. One of those days....
Comment 23 Joanmarie Diggs (irc: joanie) 2016-06-27 10:02:33 PDT
Created attachment 282140 [details]
patch for landing
Comment 24 WebKit Commit Bot 2016-06-27 10:58:07 PDT
Comment on attachment 282140 [details]
patch for landing

Clearing flags on attachment: 282140

Committed r202497: <http://trac.webkit.org/changeset/202497>
Comment 25 WebKit Commit Bot 2016-06-27 10:58:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Frédéric Wang (:fredw) 2016-06-28 02:32:17 PDT
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.