Bug 124838 - Improve renderer classes for MathML Token elements
Summary: Improve renderer classes for MathML Token elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 44208 99620 124827 126622 126628 126842 128325
Blocks: 57696 125628 125938 57695 115786 115787 124128 125597 139582
  Show dependency treegraph
 
Reported: 2013-11-25 04:37 PST by Frédéric Wang (:fredw)
Modified: 2014-12-12 04:04 PST (History)
26 users (show)

See Also:


Attachments
WIP Patch (26.82 KB, patch)
2013-12-11 12:15 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (26.50 KB, patch)
2013-12-16 10:04 PST, Frédéric Wang (:fredw)
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (34.08 KB, patch)
2013-12-17 13:40 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (40.98 KB, patch)
2013-12-18 04:44 PST, Frédéric Wang (:fredw)
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (56.22 KB, patch)
2013-12-19 07:16 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (56.28 KB, patch)
2014-01-08 09:31 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (731.98 KB, application/zip)
2014-01-08 10:55 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (733.15 KB, application/zip)
2014-01-08 11:55 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (700.79 KB, application/zip)
2014-01-08 12:31 PST, Build Bot
no flags Details
WIP Patch (56.42 KB, patch)
2014-02-03 05:18 PST, Frédéric Wang (:fredw)
cfleizach: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (739.70 KB, application/zip)
2014-02-03 06:48 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (710.86 KB, application/zip)
2014-02-03 07:26 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (739.00 KB, application/zip)
2014-02-03 07:49 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (706.45 KB, application/zip)
2014-02-03 08:19 PST, Build Bot
no flags Details
mi-only patch (18.11 KB, patch)
2014-02-04 05:22 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (600.81 KB, application/zip)
2014-02-04 07:26 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (811.45 KB, application/zip)
2014-02-04 11:41 PST, Build Bot
no flags Details
WIP Patch (29.65 KB, patch)
2014-02-11 07:12 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (600.37 KB, application/zip)
2014-02-11 08:30 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (625.85 KB, application/zip)
2014-02-11 09:58 PST, Build Bot
no flags Details
Patch (26.21 KB, patch)
2014-02-21 01:34 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for testing (73.84 KB, patch)
2014-02-21 06:51 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (512.10 KB, application/zip)
2014-02-21 07:58 PST, Build Bot
no flags Details
Patch (30.62 KB, patch)
2014-02-21 08:07 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (542.78 KB, application/zip)
2014-02-21 08:24 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (516.01 KB, application/zip)
2014-02-21 08:56 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (543.66 KB, application/zip)
2014-02-21 09:00 PST, Build Bot
no flags Details
Patch (36.53 KB, patch)
2014-02-22 03:50 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (513.90 KB, application/zip)
2014-02-22 04:40 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (545.86 KB, application/zip)
2014-02-22 05:12 PST, Build Bot
no flags Details
Patch (37.00 KB, patch)
2014-02-22 05:41 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (547.16 KB, application/zip)
2014-02-22 06:15 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (517.47 KB, application/zip)
2014-02-22 06:42 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (542.95 KB, application/zip)
2014-02-22 07:00 PST, Build Bot
no flags Details
Patch (36.96 KB, patch)
2014-02-22 07:06 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (514.47 KB, application/zip)
2014-02-22 07:39 PST, Build Bot
no flags Details
Patch (37.10 KB, patch)
2014-02-22 07:47 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (37.10 KB, patch)
2014-02-22 08:03 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (511.32 KB, application/zip)
2014-02-22 09:00 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (541.22 KB, application/zip)
2014-02-22 09:20 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (545.54 KB, application/zip)
2014-02-22 10:30 PST, Build Bot
no flags Details
Patch (37.07 KB, patch)
2014-02-25 23:22 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (37.59 KB, patch)
2014-03-10 10:41 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (602.32 KB, application/zip)
2014-03-10 11:52 PDT, Build Bot
no flags Details
Patch (62.88 KB, patch)
2014-03-10 12:06 PDT, Frédéric Wang (:fredw)
cfleizach: review+
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) 2013-11-25 04:37:33 PST
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)
Comment 1 Frédéric Wang (:fredw) 2013-12-11 12:15:38 PST
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?
Comment 2 Frédéric Wang (:fredw) 2013-12-12 04:01:09 PST
(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...
Comment 3 Frédéric Wang (:fredw) 2013-12-16 10:04:57 PST
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...
Comment 4 WebKit Commit Bot 2013-12-16 10:07:54 PST
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.
Comment 5 EFL EWS Bot 2013-12-16 10:26:31 PST
Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/49708128
Comment 6 Build Bot 2013-12-16 10:42:03 PST
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
Comment 7 EFL EWS Bot 2013-12-16 11:12:40 PST
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
Comment 8 Build Bot 2013-12-16 11:23:57 PST
Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46108028
Comment 9 Build Bot 2013-12-16 12:16:41 PST
Comment on attachment 219325 [details]
WIP Patch

Attachment 219325 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/47078057
Comment 10 Frédéric Wang (:fredw) 2013-12-17 13:40:51 PST
Created attachment 219448 [details]
WIP Patch
Comment 11 Build Bot 2013-12-17 14:18:10 PST
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
Comment 12 Build Bot 2013-12-17 14:47:33 PST
Comment on attachment 219448 [details]
WIP Patch

Attachment 219448 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/45288317
Comment 13 Frédéric Wang (:fredw) 2013-12-18 04:44:39 PST
Created attachment 219522 [details]
WIP Patch
Comment 14 EFL EWS Bot 2013-12-18 05:01:41 PST
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
Comment 15 Build Bot 2013-12-18 05:16:16 PST
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
Comment 16 Build Bot 2013-12-18 06:04:52 PST
Comment on attachment 219522 [details]
WIP Patch

Attachment 219522 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46658595
Comment 17 Build Bot 2013-12-18 06:44:19 PST
Comment on attachment 219522 [details]
WIP Patch

Attachment 219522 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46658603
Comment 18 Frédéric Wang (:fredw) 2013-12-19 07:16:41 PST
Created attachment 219651 [details]
WIP Patch
Comment 19 Build Bot 2013-12-19 07:26:34 PST
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
Comment 20 Build Bot 2013-12-19 07:43:02 PST
Comment on attachment 219651 [details]
WIP Patch

Attachment 219651 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/45258228
Comment 21 Frédéric Wang (:fredw) 2013-12-19 08:37:01 PST
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.
Comment 22 Frédéric Wang (:fredw) 2013-12-19 08:49:10 PST
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).
Comment 23 Frédéric Wang (:fredw) 2014-01-07 02:07:32 PST
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.
Comment 24 chris fleizach 2014-01-07 08:57:20 PST
(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
Comment 25 Frédéric Wang (:fredw) 2014-01-08 09:31:17 PST
Created attachment 220640 [details]
WIP Patch

Refreshing the patch after bugs 126622, 126628. Also try to change the Xcode hash.
Comment 26 WebKit Commit Bot 2014-01-08 09:33:40 PST
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.
Comment 27 Build Bot 2014-01-08 10:55:16 PST
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
Comment 28 Build Bot 2014-01-08 10:55:20 PST
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
Comment 29 Build Bot 2014-01-08 11:55:23 PST
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
Comment 30 Build Bot 2014-01-08 11:55:27 PST
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
Comment 31 Frédéric Wang (:fredw) 2014-01-08 12:12:50 PST
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.
Comment 32 Build Bot 2014-01-08 12:31:36 PST
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
Comment 33 Build Bot 2014-01-08 12:31:40 PST
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
Comment 34 Frédéric Wang (:fredw) 2014-01-23 23:58:45 PST
(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?
Comment 35 chris fleizach 2014-01-27 11:39:12 PST
(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
Comment 36 Frédéric Wang (:fredw) 2014-01-27 12:30:30 PST
(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.
Comment 37 Frédéric Wang (:fredw) 2014-02-03 05:18:29 PST
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.
Comment 38 Build Bot 2014-02-03 06:48:45 PST
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
Comment 39 Build Bot 2014-02-03 06:48:54 PST
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
Comment 40 Build Bot 2014-02-03 07:26:47 PST
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
Comment 41 Build Bot 2014-02-03 07:26:51 PST
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
Comment 42 Build Bot 2014-02-03 07:48:57 PST
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
Comment 43 Build Bot 2014-02-03 07:49:01 PST
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
Comment 44 Build Bot 2014-02-03 08:19:00 PST
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
Comment 45 Build Bot 2014-02-03 08:19:04 PST
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
Comment 46 chris fleizach 2014-02-03 08:56:38 PST
(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
Comment 47 Frédéric Wang (:fredw) 2014-02-03 08:59:47 PST
(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.
Comment 48 Frédéric Wang (:fredw) 2014-02-04 05:22:30 PST
Created attachment 223101 [details]
mi-only patch
Comment 49 WebKit Commit Bot 2014-02-04 05:24:17 PST
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.
Comment 50 Build Bot 2014-02-04 07:26:52 PST
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
Comment 51 Build Bot 2014-02-04 07:26:58 PST
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
Comment 52 Build Bot 2014-02-04 11:41:02 PST
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
Comment 53 Build Bot 2014-02-04 11:41:09 PST
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
Comment 54 chris fleizach 2014-02-07 09:20:56 PST
Comment on attachment 222977 [details]
WIP Patch

will need a new patch here
Comment 55 Frédéric Wang (:fredw) 2014-02-11 07:12:47 PST
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...
Comment 56 Build Bot 2014-02-11 08:30:02 PST
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
Comment 57 Build Bot 2014-02-11 08:30:07 PST
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
Comment 58 Build Bot 2014-02-11 09:58:23 PST
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
Comment 59 Build Bot 2014-02-11 09:58:28 PST
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
Comment 60 Frédéric Wang (:fredw) 2014-02-21 01:34:01 PST
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.
Comment 61 Frédéric Wang (:fredw) 2014-02-21 06:51:50 PST
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.
Comment 62 WebKit Commit Bot 2014-02-21 06:54:28 PST
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.
Comment 63 Build Bot 2014-02-21 07:58:02 PST
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
Comment 64 Build Bot 2014-02-21 07:58:09 PST
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
Comment 65 Frédéric Wang (:fredw) 2014-02-21 08:07:59 PST
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?
Comment 66 Build Bot 2014-02-21 08:23:52 PST
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
Comment 67 Build Bot 2014-02-21 08:24:01 PST
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
Comment 68 Build Bot 2014-02-21 08:56:17 PST
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
Comment 69 Build Bot 2014-02-21 08:56:26 PST
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
Comment 70 Build Bot 2014-02-21 09:00:00 PST
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
Comment 71 Build Bot 2014-02-21 09:00:09 PST
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
Comment 72 chris fleizach 2014-02-22 00:59:13 PST
(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.
Comment 73 Frédéric Wang (:fredw) 2014-02-22 03:50:40 PST
Created attachment 224957 [details]
Patch
Comment 74 Build Bot 2014-02-22 04:40:48 PST
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
Comment 75 Build Bot 2014-02-22 04:40:54 PST
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
Comment 76 Build Bot 2014-02-22 05:12:13 PST
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
Comment 77 Build Bot 2014-02-22 05:12:21 PST
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
Comment 78 Frédéric Wang (:fredw) 2014-02-22 05:41:07 PST
Created attachment 224963 [details]
Patch
Comment 79 Build Bot 2014-02-22 06:15:39 PST
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
Comment 80 Build Bot 2014-02-22 06:15:47 PST
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
Comment 81 Build Bot 2014-02-22 06:42:46 PST
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
Comment 82 Build Bot 2014-02-22 06:42:53 PST
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
Comment 83 Build Bot 2014-02-22 07:00:35 PST
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
Comment 84 Build Bot 2014-02-22 07:00:44 PST
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
Comment 85 Frédéric Wang (:fredw) 2014-02-22 07:06:40 PST
Created attachment 224967 [details]
Patch
Comment 86 Build Bot 2014-02-22 07:39:02 PST
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
Comment 87 Build Bot 2014-02-22 07:39:11 PST
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
Comment 88 Frédéric Wang (:fredw) 2014-02-22 07:47:50 PST
Created attachment 224969 [details]
Patch
Comment 89 Frédéric Wang (:fredw) 2014-02-22 08:03:18 PST
Created attachment 224970 [details]
Patch
Comment 90 Build Bot 2014-02-22 09:00:50 PST
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
Comment 91 Build Bot 2014-02-22 09:00:58 PST
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
Comment 92 Build Bot 2014-02-22 09:20:41 PST
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
Comment 93 Build Bot 2014-02-22 09:20:50 PST
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
Comment 94 Build Bot 2014-02-22 10:29:55 PST
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
Comment 95 Build Bot 2014-02-22 10:30:04 PST
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
Comment 96 Frédéric Wang (:fredw) 2014-02-24 00:47:58 PST
(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?
Comment 97 Frédéric Wang (:fredw) 2014-02-25 23:22:01 PST
Created attachment 225225 [details]
Patch

Just refreshing the patch...
Comment 98 Frédéric Wang (:fredw) 2014-02-26 00:17:01 PST
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.
Comment 99 Frédéric Wang (:fredw) 2014-03-10 10:41:06 PDT
Created attachment 226319 [details]
Patch
Comment 100 Build Bot 2014-03-10 11:52:41 PDT
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
Comment 101 Build Bot 2014-03-10 11:52:51 PDT
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
Comment 102 Frédéric Wang (:fredw) 2014-03-10 12:06:07 PDT
Created attachment 226326 [details]
Patch
Comment 103 chris fleizach 2014-03-10 15:24:16 PDT
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
Comment 104 Frédéric Wang (:fredw) 2014-03-11 00:45:45 PDT
Committed r165436: <http://trac.webkit.org/changeset/165436>