Bug 155018

Created attachment 273290 [details] Patch

Created attachment 273418 [details] Patch A slightly better version where I implement layoutBlock and remove computeLogicalHeight so that bug 153991 will be easier. I also had forgotten to layout children when we do "special" painting ; fixing that changes some text expectations.

Comment on attachment 273418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273418&action=review > Source/WebCore/css/mathml.css:43 > Note: it also makes sense to remove the following rule in this patch. It does not seem to have any effect (see bug 151592). mtext { line-height: 1.0; }

Created attachment 273706 [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.

Created attachment 275289 [details] Patch

In terms of the accessibility-related changes (tests only), the one-pixel changes reported for size and position are fine. And it looks like we're now exposing the operator text in the same way we expose the other element text. Arguably, that's a bug fix. :) So a11y-wize, lgtm.

Comment on attachment 275289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275289&action=review The cleaning looks great, I'll let other reviewer to check it. > LayoutTests/ChangeLog:10 > + * platform/mac/TestExpectations: Disable some tests for now. Some just fail because of small rendering differences. The accessibility test for mfenced must be updated to take into account changes in the render tree. Why don't we update the ones that fail because of small rendering differences? > LayoutTests/platform/mac/TestExpectations:818 > +# FIXME: Small differences. > +mathml/presentation/attributes-mathvariant.html [ ImageOnlyFailure ] > +mathml/presentation/fenced-mi.html [ ImageOnlyFailure ] > +mathml/presentation/fenced.html [ ImageOnlyFailure ] > +mathml/presentation/tokenElements-mathvariant.html [ ImageOnlyFailure ] Shall we update the images? Or you think we are adding a bug? In that case we should add a bug for it and add it here with the explanation. > Source/WebCore/ChangeLog:11 > + For anonymous RenderMathMLOperator renderers created by RenderMathMLFenced, we have to paint the character by ourselves ; Nit, there is a space before the ; > Source/WebCore/ChangeLog:29 > + (WebCore::RenderMathMLOperator::specialPainting): New function to determine what will be painted using StretchyOperator. specialPainting means stretched? Not sure if the name is very clear. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:279 > + for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) > + child->layoutIfNeeded(); What children an Operator can have when the anonymous are there, the text? Is it just one child? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:287 > + // We first do the normal layout without spacing. > + recomputeLogicalWidth(); > + setLogicalWidth(width() - m_leadingSpace - m_trailingSpace); > + RenderMathMLToken::layoutBlock(relayoutChildren, pageLogicalHeight); > + recomputeLogicalWidth(); This does not look correct, what are you trying to do calling recomputeLogicalWidth twice, it actually returns a bool to check if the width was actually recomputed. Also setting the logicalwidth and later calling the layoutBlock of the parent looks weird to me. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:64 > - void updateTokenContent() final; > + void updateTokenContent() override; Do you need to override in this patch? > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:159 > + ASSERT(needsLayout()); Why we do not have this in the first line of the function? > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:165 > + for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) > + child->layoutIfNeeded(); Is this too general, in what situations a token could have more than one child? > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:175 > + RenderMathMLBlock::paint(info, paintOffset); Is this required in case isValid and we paint the glyph?

Created attachment 277376 [details] Patch

Created attachment 278626 [details] Testing a small subset in EWS

Comment on attachment 275289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275289&action=review >> LayoutTests/ChangeLog:10 >> + * platform/mac/TestExpectations: Disable some tests for now. Some just fail because of small rendering differences. The accessibility test for mfenced must be updated to take into account changes in the render tree. > > Why don't we update the ones that fail because of small rendering differences? See below. >> LayoutTests/platform/mac/TestExpectations:818 >> +mathml/presentation/tokenElements-mathvariant.html [ ImageOnlyFailure ] > > Shall we update the images? Or you think we are adding a bug? In that case we should add a bug for it and add it here with the explanation. These are reftests not pixel tests, so that's not easy to just update. The problem is the following: for some math text (like anonymous operator created by mfenced or italic mi and more generally mathvariant char) we will for use our own layout instead of the standard text layout because we need special painting. This works well on GTK but on other platforms you have small shifts. The problem is probably the mix between int and LayoutUnit (see bug 155879). I've been able to fixed the case of mfenced operators by correct int rounding of firstBaseLine. I'll see if I can do something for the non-mo token elements tomorrow. >> Source/WebCore/ChangeLog:29 >> + (WebCore::RenderMathMLOperator::specialPainting): New function to determine what will be painted using StretchyOperator. > > specialPainting means stretched? Not sure if the name is very clear. I renamed it to useMathOperator and moved that to bug 157519. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:279 >> + child->layoutIfNeeded(); > > What children an Operator can have when the anonymous are there, the text? Is it just one child? I think only one anonymous block created by the CSS rules (maybe 0 too), but it does not hurt to have a general loop I guess. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:287 >> + recomputeLogicalWidth(); > > This does not look correct, what are you trying to do calling recomputeLogicalWidth twice, it actually returns a bool to check if the width was actually recomputed. Also setting the logicalwidth and later calling the layoutBlock of the parent looks weird to me. I need to check that and see if I can make this less hacky. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:64 >> + void updateTokenContent() override; > > Do you need to override in this patch? I removed that. Probably I just mixed things up when trying to update all the MathML patches to move to the new override/final/virtual conventions. >> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:159 >> + ASSERT(needsLayout()); > > Why we do not have this in the first line of the function? I moved that to the top for consistency with other functions. (This assert is probably in RenderMathMLBlock::layoutBlock too.) >> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:165 >> + child->layoutIfNeeded(); > > Is this too general, in what situations a token could have more than one child? Same here, I think it's fine to have a loop even it there is probably only one anonymous block container. >> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:175 >> + RenderMathMLBlock::paint(info, paintOffset); > > Is this required in case isValid and we paint the glyph? I think RenderMathMLBlock has some compile flag to paint the MathML frame (not sure if that's really needed though).

Created attachment 278630 [details] Patch Here is an updated version. Some parts have been moved to bug 157519, bug 157521 and bug 157568, so the code change will be smaller here and hopefully easier to review. I still need to check again Alex's comments and the tests on iOS/Mac.

Created attachment 278706 [details] Patch for EWS testing

Comment on attachment 278706 [details] Patch for EWS testing Attachment 278706 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1308126 New failing tests: accessibility/math-text.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml fast/css/readwrite-pseudoclass-editable.html fast/css/readonly-pseudoclass-common-element.html accessibility/mac/mathml-elements.html

Created attachment 278709 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 278706 [details] Patch for EWS testing Attachment 278706 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1308134 New failing tests: accessibility/ios-simulator/math.html fast/css/readwrite-pseudoclass-editable.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml fast/css/readonly-pseudoclass-common-element.html

Created attachment 278710 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4

Created attachment 278711 [details] Patch for EWS testing

Created attachment 278715 [details] Patch

Comment on attachment 278711 [details] Patch for EWS testing Attachment 278711 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1308390 New failing tests: fast/css/readwrite-pseudoclass-editable.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml fast/css/readonly-pseudoclass-common-element.html

Created attachment 278717 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 278711 [details] Patch for EWS testing Attachment 278711 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1308370 New failing tests: fast/css/readwrite-pseudoclass-editable.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml fast/css/readonly-pseudoclass-common-element.html

Created attachment 278718 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 278711 [details] Patch for EWS testing Attachment 278711 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1308418 New failing tests: fast/css/readwrite-pseudoclass-editable.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml fast/css/readonly-pseudoclass-common-element.html

Created attachment 278720 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4

Created attachment 278721 [details] Patch

Comment on attachment 278721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278721&action=review > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:187 > + IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + LayoutPoint(0, glyphAscent)); So after more testing in bug 108778 on Mac/iOS, it seems that we should only round the vertical coordinate (glyphAscent) not the horizontal one or otherwise one mathvariant reftest does not work.

Created attachment 279116 [details] Patch

Created attachment 281732 [details] Patch

Created attachment 281743 [details] Patch

Created attachment 281821 [details] Patch Here is an updated patch to run EWS, but I'll try and move some fenced accessibility code into a separate patch.

Created attachment 281828 [details] Patch This version applies on top of the patch for bug 139582.

Created attachment 281847 [details] Patch After discussion with joanie, it probably makes more sense to make the accessibility changes independent from this layout refactoring. So I'm uploading a new patch that goes back to the idea of temporarily disabling the fenced accessibility test on mac. The code to make that test work again and to fix a similar bug on ATK/Linux is essentially ready on bug 139582, but the way to organize the patches and review has to be sorted out with accessibility people.

Created attachment 281972 [details] Patch

Comment on attachment 281972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281972&action=review > LayoutTests/accessibility/mac/mathml-elements.html:114 > + /* > + FIXME: AccessibilityRenderObject should be updated to properly expose the text of mfenced operators. > + See https://bugs.webkit.org/show_bug.cgi?id=139582. > var fenceValues = new Array("{", "2", ",", "a", ",", "e", "}"); Instead of commenting out this code you should remove it. > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:115 > + // Early return if the token element contains RendererElement's. Do you mean RenderElements here?

Committed r202420: <http://trac.webkit.org/changeset/202420>

Comment on attachment 281972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281972&action=review > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:167 > + setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph)); What guarantees m_mathVariantGlyph.font is non-null and alive? I suspect it could be because we see crashes like these in the wild: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Subtype: KERN_INVALID_ADDRESS at 0x000000000000074f Triggered by Thread: 0 Filtered syslog: None found Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed ↩: 0 WebCore 0x0000000190d67970 WTF::HashTableAddResult<WTF::HashTableIterator<int, WTF::KeyValuePair<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >, WTF::IntHash<unsigned int>, WTF::HashMap<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> >, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >::KeyValuePairTraits, WTF::HashTraits<int> > > WTF::HashMap<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> >, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >::add<std::nullptr_t>(int&&, std::nullptr_t&&) + 28 (HashTable.h:869) 1 WebCore 0x0000000190970634 WebCore::GlyphMetricsMap<float>::locatePageSlowCase(unsigned int) + 176 (GlyphMetricsMap.h:120) 2 WebCore 0x0000000190970634 WebCore::GlyphMetricsMap<float>::locatePageSlowCase(unsigned int) + 176 (GlyphMetricsMap.h:120) 3 WebCore 0x00000001914cfb3c WebCore::RenderMathMLToken::layoutBlock(bool, WebCore::LayoutUnit) + 240 (GlyphMetricsMap.h:87) 4 WebCore 0x00000001908de104 WebCore::RenderBlock::layout() + 72 (RenderBlock.cpp:1075) 5 WebCore 0x00000001914cc320 WebCore::RenderMathMLRow::computeLineVerticalStretch(WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 152 (RenderElement.h:133) 6 WebCore 0x00000001914ccca0 WebCore::RenderMathMLRow::layoutBlock(bool, WebCore::LayoutUnit) + 64 (RenderMathMLRow.cpp:190) 7 WebCore 0x00000001908de104 WebCore::RenderBlock::layout() + 72 (RenderBlock.cpp:1075)

(In reply to comment #36) > Comment on attachment 281972 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281972&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:167 > > + setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph)); > > What guarantees m_mathVariantGlyph.font is non-null and alive? I suspect it There are isValid() calls everywhere in the MathML code. However, they do "glyph || font" rather than "glyph && font" as I expected...

I see in bug 150171 comment 5 that the initial intent was GlyphData::isEmpty(). For me GlyphData::isValid() means having a glyph id and a font. @Myles, Carlos: is there any reason to call a GlyphData "valid" without a font?

I opened bug 159783 and will upload a patch tomorrow.