| Summary: | AX: Anonymous RenderMathMLOperators are not exposed to the accessibility tree | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
| Component: | Accessibility | Assignee: | Joanmarie Diggs <jdiggs> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dbarton, dmazzoni, esprehn+autocc, glenn, jcraig, jdiggs, kondapallykalyan, mario, mrobinson, rniwa, samuel_white, webkit-bug-importer | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Bug Depends on: | 124838, 155018 | ||||||||||||||||||||||
| Bug Blocks: | 159208, 124836, 159205 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
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. Created attachment 281826 [details]
Patch
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 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). (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 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! Created attachment 281981 [details]
Patch
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 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 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 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 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 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
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. ;)) Created attachment 282133 [details]
Patch
Comment on attachment 282133 [details]
Patch
@Chris: Please review. Thanks!
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? (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! Created attachment 282138 [details]
patch for landing
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 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.... Created attachment 282140 [details]
patch for landing
Comment on attachment 282140 [details] patch for landing Clearing flags on attachment: 282140 Committed r202497: <http://trac.webkit.org/changeset/202497> All reviewed patches have been landed. Closing bug. 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. |
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; ...