Next patch in the series adding code to handle the layout of the scripts without the flexbox.
Created attachment 270746 [details] Patch
Created attachment 271155 [details] GTK a11y test ref update This patch modifies the reference of one GTK accessibility test (when I apply it on master after the patches from the previous MathML refactoring bugs). It's not a real test failure, just some bounding box changes. See attachment.
(In reply to comment #2) > Created attachment 271155 [details] > GTK a11y test ref update > > This patch modifies the reference of one GTK accessibility test (when I > apply it on master after the patches from the previous MathML refactoring > bugs). It's not a real test failure, just some bounding box changes. See > attachment. Thanks for the test, I'll upload the patch with this modification.
Created attachment 271165 [details] Patch
(In reply to comment #3) > (In reply to comment #2) > > Created attachment 271155 [details] > > GTK a11y test ref update > > > > This patch modifies the reference of one GTK accessibility test (when I > > apply it on master after the patches from the previous MathML refactoring > > bugs). It's not a real test failure, just some bounding box changes. See > > attachment. > > Thanks for the test, I'll upload the patch with this modification. So you'll also need to add a changelog for this test change.
Created attachment 272854 [details] Patch Attached is a complete refactoring of RenderMathMLScript to avoid anonymous wrappers and flexboxes. There are a couple of things to adjust for mac tests, so I'll check that on Monday. But I'm already attaching for feedback.
Created attachment 273277 [details] Patch OK, I checked the tests on mac. Now, I think it's ready for review.
The timeout to wait web font in LayoutTests/mathml/mathml-in-html5/subsup-parameters-1.html is a bit hacky. I'll upload a new version later that uses document.fonts.ready (implemented in bug 153348).
Created attachment 273703 [details] Patch I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.
Comment on attachment 273703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273703&action=review > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:415 > + return Optional<int>(base->top() + base->firstLineBaseline().value()); As said in the comment conversions between firstLineBaseline (int) and LayoutUnit (fractional pixel) is bad for the layout accuracy. However, a better rounding of the value here seems to improve the rendering: return Optional<int>(static_cast<int>(lroundf(base->top() + base->firstLineBaseline().value())));
Created attachment 275286 [details] Patch
Created attachment 275399 [details] Patch
Comment on attachment 275399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275399&action=review LGTM, it is a complex layout but with the refactor it is clear how it works. I've implemented part of the code so I'll let other reviewer to r+. > Source/WebCore/ChangeLog:24 > + (WebCore::RenderMathMLScripts::getScriptMetricsAndLayoutIfNeeded): Helpef function to get the maximum ascent/descent of all the scripts and determine the minimal sub/sup shifts to apply. Nit, Helpef -> Helper
Created attachment 275993 [details] Patch Thanks Alex. I'm uploading a new patch with the typo fix and other trivial changes (line wrap ~100 chars in changelog, use logical Width/Height, add link to bug 156401 etc)
Created attachment 276229 [details] Patch Same patch, but just to check EWS...
Comment on attachment 276229 [details] Patch Attachment 276229 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1141404 New failing tests: mathml/opentype/large-operators-italic-correction.html mathml/presentation/roots.xhtml mathml/presentation/multiscripts-positions.html mathml/presentation/scripts-subsup.html mathml/presentation/scripts-height.html
Created attachment 276230 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 276232 [details] Patch
Comment on attachment 276232 [details] Patch Attachment 276232 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1142034 New failing tests: mathml/presentation/scripts-height.html
Created attachment 276233 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 276237 [details] Patch
Created attachment 276411 [details] Patch
Comment on attachment 276411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276411&action=review Looks good to me, other than two issues and then a few nits. > LayoutTests/mathml/mathml-in-html5/LICENSE:3 > +Tests in this directory have been imported from > +https://github.com/MathML/MathMLinHTML5-tests > + We need to figure out if this license is compatible with WebKit and what the policy is going to be for importing mathml-in-html5 tests. > Source/WebCore/css/mathml.css:104 > +/* This is a special style for erroneous markup */ Super nit: this sentence is missing a period. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:76 > + if (!base) This function is really complicated! I think it may need some comments linking each decision to the spec and to decrease the complexity of the control. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:170 > + } break; This should probably have its own line. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:172 > + default: > + ASSERT_NOT_REACHED(); This can be omitted if there is not other member in the enum. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:246 > + for (subScript = script; subScript && !isPrescript(*subScript); subScript = supScript->nextSiblingBox()) { I think you can define subScript inside the for loop like: for (RenderBox* subScript =...) > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:247 > + supScript = subScript->nextSiblingBox(); This probably needs an ASSERT, since you rely on it later. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:357 > + for (subScript = firstPreScript; subScript; subScript = supScript->nextSiblingBox()) { For each of these loops, it's probably a good idea to define subScript inside of the for loop: for (RenderBox* subScript =...) > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:358 > + supScript = subScript->nextSiblingBox(); You should probably ASSERT(supScript) > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:364 > + supScript = subScript->nextSiblingBox(); You should probably ASSERT(supScript). > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:371 > + supScript = subScript->nextSiblingBox(); Ditto. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:385 > + supScript = subScript->nextSiblingBox(); Ditto. > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:399 > + default: > + ASSERT_NOT_REACHED(); > } You can probably just remove this if there are no other members.
Comment on attachment 276411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276411&action=review >> LayoutTests/mathml/mathml-in-html5/LICENSE:3 >> + > > We need to figure out if this license is compatible with WebKit and what the policy is going to be for importing mathml-in-html5 tests. OK, I found the tests from the Test The Web Forward project: http://trac.webkit.org/browser/trunk/LayoutTests/imported/w3c/web-platform-tests They use the same license and try and keep the structure of https://github.com/w3c/web-platform-tests So I'll basically do the same for https://github.com/MathML/MathMLinHTML5-tests >> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:76 >> + if (!base) > > This function is really complicated! I think it may need some comments linking each decision to the spec and to decrease the complexity of the control. OK, I think I'll be able to simplify the code and add many comments.
Created attachment 276623 [details] Patch @Martin: can you please check this new patch? Thanks!
Comment on attachment 276623 [details] Patch That's a lot easier to read now. Thanks!
Committed r199665: <http://trac.webkit.org/changeset/199665>
Committed r199666: <http://trac.webkit.org/changeset/199666>