Improve renderer classes for MathML Token elements

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

Proposal:

- For MathML Token elements, create a renderer class that use an anonymous wrapper to store the child nodes and add CSS style.

- Make MathMLOperator inherits from this class. This will allow to apply start/end margins to implement lspace/rspace (bug 99620)

- For <mi>, this will allow to apply CSS italic only when necessary (bug 44208)

- For other elements like <mtext>, allow to have non-text children, like what foreignObject does (bug 124128)

- Implement addChild/removeChild and attribute changes correctly (bug 57695)

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

Created attachment 218993 [details]
WIP Patch

Here is a patch that move the mo anonymous code into a generic class for token frames (and will hopefully allow to fix the remaining style update issue in bug 99620), fixes MathML whitespace collapse (http://www.w3.org/TR/MathML/chapter2.html#fund.collapse) and the serious italic mi bug (bug 44208). Hopefully it will also help for bug 44210 too, but I haven't done extensive testing yet.

Unfortunately I just realized that the mo implementation prevents dumpAsText to access the text (bug 125597). Since the patch moved the implementation to other token elements, this also breaks various tests relying on dumpAsText and putting text in <mtext>. Any idea on how to solve that issue? Or any suggestion of implementation for the MathML token elements?

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

(In reply to comment #1)
> Unfortunately I just realized that the mo implementation prevents dumpAsText to access the text (bug 125597). Since the patch moved the implementation to other token elements, this also breaks various tests relying on dumpAsText and putting text in <mtext>. Any idea on how to solve that issue? Or any suggestion of implementation for the MathML token elements?

One difference I noticed is that the RenderText does not have any "{#text}" in the dumped render tree. I tried to pass an arbitrary text child of the token element when creating the RenderText renderer and that seems to remove that difference. However, the text content is still not shown in the DumpAsTree output.

I'm wondering if the clean way to fix bug 125628 would be to implement something like RenderSVGInlineText, but that seems a bit overkill to do that now for more important bugs like bug 44208 or bug 99620... So perhaps I should finally only refactor the classes for MathML token later...

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

Created attachment 219325 [details]
WIP Patch

So here is a patch without the whitespace collapse changes, so that the RenderText are still attached correctly.

I'm setting this to "review" to get feedback about the general approach of anonymous wrapper + updating/propagating the style, but it's not ready yet.

This fixes bug 44208, but there are unit tests with dynamic Javascript changes for which the style of <mi> elements does not seem to be updated correctly, so I'm wondering if I'm doing something incorrectly...

[WebKit Commit Bot]

Attachment 219325 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/mathml/presentation/mspace-children-expected.txt', u'LayoutTests/mathml/presentation/mspace-children.html', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/css/mathml.css', u'Source/WebCore/mathml/MathMLElement.cpp', u'Source/WebCore/mathml/MathMLInlineContainerElement.cpp', u'Source/WebCore/mathml/MathMLTextElement.cpp', u'Source/WebCore/mathml/mathtags.in', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/mathml/RenderMathMLBlock.h', u'Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLOperator.h', u'Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLSpace.h', u'Source/WebCore/rendering/mathml/RenderMathMLToken.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLToken.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/mathml/MathMLInlineContainerElement.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:87:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:89:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:90:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/49708128

[Build Bot]

Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48668374

[EFL EWS Bot]

Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/47778010

[Build Bot]

Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46108028

[Build Bot]

Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/47078057

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

Created attachment 219448 [details]
WIP Patch

[Build Bot]

Comment on attachment 219448 [details]
WIP Patch

Attachment 219448 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/49568220

[Build Bot]

Comment on attachment 219448 [details]
WIP Patch

Attachment 219448 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/45288317

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

Created attachment 219522 [details]
WIP Patch

[EFL EWS Bot]

Comment on attachment 219522 [details]
WIP Patch

Attachment 219522 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/44318499

[Build Bot]

Comment on attachment 219522 [details]
WIP Patch

Attachment 219522 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/47078435

[Build Bot]

Comment on attachment 219522 [details]
WIP Patch

Attachment 219522 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46658595

[Build Bot]

Comment on attachment 219522 [details]
WIP Patch

Attachment 219522 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46658603

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

Created attachment 219651 [details]
WIP Patch

[Build Bot]

Comment on attachment 219651 [details]
WIP Patch

Attachment 219651 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/45388257

[Build Bot]

Comment on attachment 219651 [details]
WIP Patch

Attachment 219651 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/45258228

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

Comment on attachment 219651 [details]
WIP Patch

Can anyone with a Mac update the Xcode file? xcodebodge no longer works for me and I'm not sure how to do this by hand...

Some tests fail for me (styles like italic or margins are sometimes not set/updated correctly). But for some reason they seem to pass here.

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

Attachment 219653 [details] does not render correctly for me with the patch (x is not italic). However, similar cases render correctly on the Mozilla MathML Torture (e.g. example 24).

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

I would still appreciate if someone can update the Xcode file or give a reliable way to do so, so that we can check if the tests pass on Mac too.

And any hint on the anonymous frame, style inheritance and possible coding errors that lead to local test failures would be helpful too.

[chris fleizach]

(In reply to comment #23)
> I would still appreciate if someone can update the Xcode file or give a reliable way to do so, so that we can check if the tests pass on Mac too.
> 
> And any hint on the anonymous frame, style inheritance and possible coding errors that lead to local test failures would be helpful too.

What I've done in the passed with Xcode project is look for another file name in the same area of code, then duplicate that line (and other lines that match that name - usually there are 2 or 3 in project.pbxproj). Then change a few bits in the hash that references it so it's unique. It usually seems to work

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

Created attachment 220640 [details]
WIP Patch

Refreshing the patch after bugs 126622, 126628. Also try to change the Xcode hash.

[WebKit Commit Bot]

Attachment 220640 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/mathml/presentation/fenced-mi-expected.html', u'LayoutTests/mathml/presentation/fenced-mi.html', u'LayoutTests/mathml/presentation/mfenced-attributes-dynamic-expected.html', u'LayoutTests/mathml/presentation/mfenced-attributes-dynamic.html', u'LayoutTests/mathml/presentation/mspace-children-expected.txt', u'LayoutTests/mathml/presentation/mspace-children.html', u'LayoutTests/mathml/presentation/tokenElements-dynamic-expected.html', u'LayoutTests/mathml/presentation/tokenElements-dynamic.html', u'LayoutTests/mathml/presentation/tokenElements-mathvariant-expected.html', u'LayoutTests/mathml/presentation/tokenElements-mathvariant.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/mathml.css', u'Source/WebCore/mathml/MathMLTextElement.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLFenced.h', u'Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLOperator.h', u'Source/WebCore/rendering/mathml/RenderMathMLToken.cpp', u'Source/WebCore/rendering/mathml/RenderMathMLToken.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/rendering/mathml/RenderMathMLToken.h:27:  RenderMathMLToken_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 220640 [details]
WIP Patch

Attachment 220640 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5340001021722624

New failing tests:
mathml/presentation/attributes-mathvariant.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 220643 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Build Bot]

Comment on attachment 220640 [details]
WIP Patch

Attachment 220640 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4866181974458368

New failing tests:
mathml/presentation/attributes-mathvariant.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 220647 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

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

The style error report does not seem to be correct.

So now this builds on mac and generates the same test failures as my local settings. I still wonder why the CSS style is not applied/updated correctly.

[Build Bot]

Comment on attachment 220640 [details]
WIP Patch

Attachment 220640 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5716404170915840

New failing tests:
mathml/presentation/attributes-mathvariant.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 220651 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

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

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

(In reply to comment #23)
> And any hint on the anonymous frame, style inheritance and possible coding errors that lead to local test failures would be helpful too.

Can anyone provide any advice on this, please?

[chris fleizach]

(In reply to comment #34)
> (In reply to comment #23)
> > And any hint on the anonymous frame, style inheritance and possible coding errors that lead to local test failures would be helpful too.
> 
> Can anyone provide any advice on this, please?

What are you in need of here. I don't have any advice. Maybe making this patch smaller and drop in piece by piece so we can more easily identify when the tests fail

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

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #23)
> > > And any hint on the anonymous frame, style inheritance and possible coding errors that lead to local test failures would be helpful too.
> > 
> > Can anyone provide any advice on this, please?
> 
> What are you in need of here. I don't have any advice. Maybe making this patch smaller and drop in piece by piece so we can more easily identify when the tests fail

Thanks. Unfortunately I already extracted these changes from the operator dictionary patch and I'm afraid I can not really reduce it any further (once you start moving the code for <mo>, you must also do the changes for <mfenced>). However for the failing tests, we can just focus on the <mi>x</mi> (italic) vs <mi>sin</mi> cases (upright) for now.

I am trying to follow what seems to be the WebKit way to attach anonymous style: create anonymous renderers and ensure that they are kept in sync with the parent style and in general correctly updated when dynamic changes happen. However, I don't really know how the whole layout code works so I'm probably doing something wrong. So I'd just need that someone who is more familiar with the layout code (not necessarily MathML) take a look at the changes to the following files:

rendering/mathml/RenderMathMLOperator.cpp
rendering/mathml/RenderMathMLToken.cpp

and say if there is an obvious mistake in how the WebKit API is used.

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

Created attachment 222977 [details]
WIP Patch

Just refreshing the previous patch.

I still have not made any progress on this since my request for information one month ago. I think I can extract the part for bug 44208, but I'm not sure this will really help to get feedback... I'm not asking a full review but only whether what is done in RenderMathMLToken.cpp is the right way to add anonymous style and to keep it up-to-date. I've been trying to ask people by mail and on IRC but nobody has found time to even just take a look at that single file. At the moment, I'm just stuck on this and I have no idea about who to ask for help... Thus any suggestion would be very welcome.

[Build Bot]

Comment on attachment 222977 [details]
WIP Patch

Attachment 222977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6410704491905024

New failing tests:
mathml/presentation/attributes-mathvariant.html
fast/dom/HTMLAreaElement/area-password.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
fast/dom/HTMLAreaElement/area-username.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 222983 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

[Build Bot]

Comment on attachment 222977 [details]
WIP Patch

Attachment 222977 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4632404421509120

New failing tests:
mathml/presentation/attributes-mathvariant.html
fast/dom/HTMLAreaElement/area-password.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
fast/dom/HTMLAreaElement/area-username.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 222985 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 222977 [details]
WIP Patch

Attachment 222977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5172556688523264

New failing tests:
mathml/presentation/attributes-mathvariant.html
fast/dom/HTMLAreaElement/area-password.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
fast/dom/HTMLAreaElement/area-username.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 222986 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

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

[Build Bot]

Comment on attachment 222977 [details]
WIP Patch

Attachment 222977 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5955769744752640

New failing tests:
mathml/presentation/attributes-mathvariant.html
fast/dom/HTMLAreaElement/area-password.html
mathml/presentation/mo-stretch.html
mathml/presentation/fenced-mi.html
mathml/presentation/tokenElements-dynamic.html
mathml/presentation/mspace-children.html
platform/mac/accessibility/mathml-elements.html
fast/dom/HTMLAreaElement/area-username.html
mathml/presentation/fenced.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/tokenElements-mathvariant.html

[Build Bot]

Created attachment 222989 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

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

[chris fleizach]

(In reply to comment #37)
> Created an attachment (id=222977) [details]
> WIP Patch
> 
> Just refreshing the previous patch.
> 
> I still have not made any progress on this since my request for information one month ago. I think I can extract the part for bug 44208, but I'm not sure this will really help to get feedback... I'm not asking a full review but only whether what is done in RenderMathMLToken.cpp is the right way to add anonymous style and to keep it up-to-date. I've been trying to ask people by mail and on IRC but nobody has found time to even just take a look at that single file. At the moment, I'm just stuck on this and I have no idea about who to ask for help... Thus any suggestion would be very welcome.

Maybe sending an email to webkit-dev with your small snippet and a directed question would get more feedback. I think the combination of a failing tests + large patch + unknown code area = hard to get feedback

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

(In reply to comment #46)
> Maybe sending an email to webkit-dev with your small snippet and a directed question would get more feedback. I think the combination of a failing tests + large patch + unknown code area = hard to get feedback

OK, I will extract the mi changes tomorrow and submit a message to webkit-dev tomorrow.

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

Created attachment 223101 [details]
mi-only patch

[WebKit Commit Bot]

Attachment 223101 [details] did not pass style-queue:


ERROR: Source/WebCore/mathml/MathMLTextElement.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/mathml/MathMLTextElement.cpp:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/mathml/MathMLTextElement.cpp:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/mathml/MathMLTextElement.cpp:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 223101 [details]
mi-only patch

Attachment 223101 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5726830136918016

New failing tests:
mathml/presentation/attributes-mathvariant.html

[Build Bot]

Created attachment 223113 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Build Bot]

Comment on attachment 223101 [details]
mi-only patch

Attachment 223101 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5401982030315520

New failing tests:
mathml/presentation/attributes-mathvariant.html

[Build Bot]

Created attachment 223148 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

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

[chris fleizach]

Comment on attachment 222977 [details]
WIP Patch

will need a new patch here

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

Created attachment 223858 [details]
WIP Patch

(In reply to comment #54)
> (From update of attachment 222977 [details])
> will need a new patch here

Here is an updated patch, but some tests fail with incorrect spacing. Still no feedback on webkit-dev about the proper way to handle that...

[Build Bot]

Comment on attachment 223858 [details]
WIP Patch

Attachment 223858 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6649031019200512

New failing tests:
mathml/presentation/fenced.html
mathml/presentation/fenced-mi.html
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 223865 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 223858 [details]
WIP Patch

Attachment 223858 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4619865130270720

New failing tests:
mathml/presentation/fenced-mi.html
mathml/presentation/fenced.html
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mfenced-attributes-dynamic.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 223874 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

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

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

Created attachment 224842 [details]
Patch

OK, I've finally found a patch to refactor the mo/mfenced code that does not break the spacing of previous tests and also improves slightly mfenced. From that, it will be easy to fix bug 115787. This applies on top of bug 119043, bug 124827 and bug 126842 so I'll come back to this later.

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

Created attachment 224861 [details]
Patch for testing

There is an error in

LayoutTests/platform/mac/accessibility/mathml-elements.html

with a closing </mroot> in place of a closing </mfenced>. I'm not sure that's the issue, but I'm running the test again just in case.

[WebKit Commit Bot]

Attachment 224861 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/mathml/RenderMathMLFenced.h:48:  The parameter name "form" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLFenced.h:48:  The parameter name "flag" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 224861 [details]
Patch for testing

Attachment 224861 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6635365507006464

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224865 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

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

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

Created attachment 224866 [details]
Patch

So the failing accessibility test is in LayoutTests/platform/mac/accessibility/mathml-elements.html:

<math id="fenced">
  <mfenced open="{" close="}" separators=",,"><mi>2</mi><mi>a</mi><mi>e</mi></mfenced>
</math>

The error seems to be that child.childAtIndex(0) is undefined for child = one of the anonymous <mo>.

PASS fenced.role is 'AXRole: AXGroup'
PASS fenced.subrole is 'AXSubrole: AXMathFenced'
PASS fenced.stringAttributeValue('AXMathFencedOpen') is '{'
PASS fenced.stringAttributeValue('AXMathFencedClose') is '}'
FAIL child.childAtIndex(0).stringValue should be AXValue: {. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
PASS child.childAtIndex(0).stringValue is 'AXValue: 2'
FAIL child.childAtIndex(0).stringValue should be AXValue: ,. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
PASS child.childAtIndex(0).stringValue is 'AXValue: a'
FAIL child.childAtIndex(0).stringValue should be AXValue: ,. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
PASS child.childAtIndex(0).stringValue is 'AXValue: e'
FAIL child.childAtIndex(0).stringValue should be AXValue: }. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')

The changes made by the patch is the one suggested by a FIXME comment: the RenderMathMLOperator's created by mfenced are now really anonymous i.e. they are created by passing the document to the constructor rather than the mfenced element:

         RenderMathMLFenced {mfenced} at (1,0) size 71x15
-          RenderMathMLOperator {mfenced} at (1,0) size 11x15
-            RenderMathMLBlock (flex) {mfenced} at (0,0) size 10x15
+          RenderMathMLOperator (anonymous) at (0,0) size 14x15
+            RenderMathMLBlock (anonymous, flex) at (1,0) size 9x15
               RenderBlock (anonymous) at (0,0) size 8x15
                 RenderText at (0,-5) size 8x25
                   text run at (0,-5) width 8: "{"

I've preserved the "setIgnoreInAccessibilityTree" call and this bad RenderText. There is some code in accessibility/ to workaround the issue with this RenderText, but I'm not sure to understand how it works:

http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp#L653
http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp#L3484

@Chris: any suggestion on how to modify the accessibility code or the test?

[Build Bot]

Comment on attachment 224861 [details]
Patch for testing

Attachment 224861 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5135083769954304

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224868 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

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

[Build Bot]

Comment on attachment 224861 [details]
Patch for testing

Attachment 224861 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5091172426973184

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224874 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 224861 [details]
Patch for testing

Attachment 224861 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5395913107308544

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224875 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

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

[chris fleizach]

(In reply to comment #65)
> Created an attachment (id=224866) [details]
> Patch
> 
> So the failing accessibility test is in LayoutTests/platform/mac/accessibility/mathml-elements.html:
> 
> <math id="fenced">
>   <mfenced open="{" close="}" separators=",,"><mi>2</mi><mi>a</mi><mi>e</mi></mfenced>
> </math>
> 
> The error seems to be that child.childAtIndex(0) is undefined for child = one of the anonymous <mo>.
> 
> PASS fenced.role is 'AXRole: AXGroup'
> PASS fenced.subrole is 'AXSubrole: AXMathFenced'
> PASS fenced.stringAttributeValue('AXMathFencedOpen') is '{'
> PASS fenced.stringAttributeValue('AXMathFencedClose') is '}'
> FAIL child.childAtIndex(0).stringValue should be AXValue: {. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
> PASS child.childAtIndex(0).stringValue is 'AXValue: 2'
> FAIL child.childAtIndex(0).stringValue should be AXValue: ,. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
> PASS child.childAtIndex(0).stringValue is 'AXValue: a'
> FAIL child.childAtIndex(0).stringValue should be AXValue: ,. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
> PASS child.childAtIndex(0).stringValue is 'AXValue: e'
> FAIL child.childAtIndex(0).stringValue should be AXValue: }. Threw exception TypeError: undefined is not an object (evaluating 'child.childAtIndex(0).stringValue')
> 
> The changes made by the patch is the one suggested by a FIXME comment: the RenderMathMLOperator's created by mfenced are now really anonymous i.e. they are created by passing the document to the constructor rather than the mfenced element:
> 
>          RenderMathMLFenced {mfenced} at (1,0) size 71x15
> -          RenderMathMLOperator {mfenced} at (1,0) size 11x15
> -            RenderMathMLBlock (flex) {mfenced} at (0,0) size 10x15
> +          RenderMathMLOperator (anonymous) at (0,0) size 14x15
> +            RenderMathMLBlock (anonymous, flex) at (1,0) size 9x15
>                RenderBlock (anonymous) at (0,0) size 8x15
>                  RenderText at (0,-5) size 8x25
>                    text run at (0,-5) width 8: "{"
> 
> I've preserved the "setIgnoreInAccessibilityTree" call and this bad RenderText. There is some code in accessibility/ to workaround the issue with this RenderText, but I'm not sure to understand how it works:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp#L653
> http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp#L3484
> 
> @Chris: any suggestion on how to modify the accessibility code or the test?

I think we want to set setIgnoreInAccessibilityTree(false) if the operators are now anonymous. We want the AX code to expose RenderMathMLOperator (anonymous)  in the AX tree.

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

Created attachment 224957 [details]
Patch

[Build Bot]

Comment on attachment 224957 [details]
Patch

Attachment 224957 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4743150019018752

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224960 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 224957 [details]
Patch

Attachment 224957 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5260436685455360

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224962 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

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

Created attachment 224963 [details]
Patch

[Build Bot]

Comment on attachment 224957 [details]
Patch

Attachment 224957 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5649025193738240

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224964 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Build Bot]

Comment on attachment 224963 [details]
Patch

Attachment 224963 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5490606130331648

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224965 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 224963 [details]
Patch

Attachment 224963 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4857902015709184

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224966 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

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

Created attachment 224967 [details]
Patch

[Build Bot]

Comment on attachment 224963 [details]
Patch

Attachment 224963 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4594847751077888

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224968 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

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

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

Created attachment 224969 [details]
Patch

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

Created attachment 224970 [details]
Patch

[Build Bot]

Comment on attachment 224970 [details]
Patch

Attachment 224970 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4746869192261632

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224971 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 224970 [details]
Patch

Attachment 224970 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5653630606639104

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224972 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

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

[Build Bot]

Comment on attachment 224970 [details]
Patch

Attachment 224970 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4560060663463936

New failing tests:
platform/mac/accessibility/mathml-elements.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 224975 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

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

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

(In reply to comment #72)
> I think we want to set setIgnoreInAccessibilityTree(false) if the operators are now anonymous. We want the AX code to expose RenderMathMLOperator (anonymous)  in the AX tree.

My understanding is that with this patch the MathML code only uses anonymous trees. So this m_ignoreInAccessibilityTree member and other things to skip the corresponding renderers in the accessibility code are no longer needed. However, the RenderText still needs special handling since it is not created from a real DOM text node.

So I've tried to remove some code, made the anonymous RenderMathOperator/RenderText visible in the accessibility code and tried to adapt the code to get the text. However, the test is still failing and it's a bit hard to understand what's happening without a Mac...

@Chris: could you check the last patch and give any advice?

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

Created attachment 225225 [details]
Patch

Just refreshing the patch...

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

Comment on attachment 225225 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1538
>      }

I just realized this weird thing: We are calling RenderMathMLBlock::paint two times. It seems that it has been introduced by
http://trac.webkit.org/changeset/157070/trunk/Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp

@Martin: is it intentional or just a mistake? I believe the first RenderMathMLBlock::paint call before the visibility check should not be done.

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

Created attachment 226319 [details]
Patch

[Build Bot]

Comment on attachment 226319 [details]
Patch

Attachment 226319 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5883212010094592

New failing tests:
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 226325 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

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

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

Created attachment 226326 [details]
Patch

[chris fleizach]

Comment on attachment 226326 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:649
>              return toRenderText(*m_renderer).text();

indentation seems wrong on this line

> Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:55
> +    m_open = open.isNull() ? "(" : open;

we should make this a constant at the top of the file probably.

> Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:59
> +    m_close = close.isNull() ? ")" : close;

ditto

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

Committed r165436: <http://trac.webkit.org/changeset/165436>