Bug 97390

Summary: REGRESSION (r128837): mathml/presentation/subsup.xhtml became flaky
Product: WebKit Reporter: Zan Dobersek <zan>
Component: MathMLAssignee: Dave Barton <dbarton>
Status: RESOLVED FIXED    
Severity: Normal CC: dbarton, eric, fred.wang, gyuyoung.kim, ojan, rakuco, tony, webkit.review.bot
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 98791    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Zan Dobersek
Reported 2012-09-22 00:56:20 PDT
This MathML test became flaky after the changes that were made in r128837[1]. The rebaseline for these changes was done in r128862[2] and after that the test is intermittently failing with the following diff: --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/mathml/presentation/subsup-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/mathml/presentation/subsup-actual.txt @@ -1,15 +1,15 @@ layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 -layer at (0,0) size 800x308 - RenderBlock {html} at (0,0) size 800x308 - RenderBody {body} at (8,16) size 784x276 +layer at (0,0) size 800x306 + RenderBlock {html} at (0,0) size 800x306 + RenderBody {body} at (8,16) size 784x274 RenderBlock {p} at (0,0) size 784x20 RenderText {#text} at (0,0) size 36x19 text run at (0,0) width 36: "both: " RenderMathMLMath {math} at (36,0) size 16x20 [padding: 0 1 0 1] RenderMathMLSubSup {msubsup} at (1,0) size 14x20 - RenderMathMLBlock (anonymous, flex) at (0,0) size 7x20 [padding: 7 0 2 0] - RenderBlock {mi} at (0,7) size 7x11 + RenderMathMLBlock (anonymous, flex) at (0,0) size 7x20 [padding: 7 0 1 0] + RenderBlock {mi} at (0,7) size 7x12 RenderText {#text} at (0,-9) size 7x25 text run at (0,-9) width 7: "x" RenderMathMLBlock (anonymous, flex) at (7,0) size 7x20 @@ -25,8 +25,8 @@ text run at (0,0) width 148: "long subscript w/ both: " RenderMathMLMath {math} at (148,0) size 53x21 [padding: 0 1 0 1] RenderMathMLSubSup {msubsup} at (1,0) size 51x21 - RenderMathMLBlock (anonymous, flex) at (0,0) size 9x21 [padding: 4 0 2 0] - RenderBlock {mi} at (0,4) size 9x15 + RenderMathMLBlock (anonymous, flex) at (0,0) size 9x21 [padding: 4 0 1 0] + RenderBlock {mi} at (0,4) size 9x16 RenderText {#text} at (0,-6) size 9x25 text run at (0,-6) width 9: "Z" RenderMathMLBlock (anonymous, flex) at (9,0) size 42x21 @@ -54,17 +54,17 @@ RenderText {#text} at (0,-5) size 3x19 text run at (0,-5) width 3: "j" RenderText {#text} at (0,0) size 0x0 - RenderBlock {p} at (0,73) size 784x21 + RenderBlock {p} at (0,73) size 784x19 RenderText {#text} at (0,1) size 160x19 text run at (0,1) width 160: "long superscript w/ both: " - RenderMathMLMath {math} at (160,0) size 34x21 [padding: 0 1 0 1] - RenderMathMLSubSup {msubsup} at (1,0) size 32x21 - RenderMathMLBlock (anonymous, flex) at (0,0) size 9x21 [padding: 5 0 4 0] + RenderMathMLMath {math} at (160,0) size 34x19 [padding: 0 1 0 1] + RenderMathMLSubSup {msubsup} at (1,0) size 32x19 + RenderMathMLBlock (anonymous, flex) at (0,0) size 9x19 [padding: 5 0 2 0] RenderBlock {mi} at (0,5) size 9x12 RenderText {#text} at (0,-6) size 9x25 text run at (0,-6) width 9: "Z" - RenderMathMLBlock (anonymous, flex) at (9,0) size 23x21 - RenderBlock {mi} at (0,14) size 23x7 + RenderMathMLBlock (anonymous, flex) at (9,0) size 23x19 + RenderBlock {mi} at (0,12) size 23x7 RenderText {#text} at (0,-7) size 5x19 text run at (0,-7) width 5: "x" RenderMathMLRow {mrow} at (1,0) size 22x12 @@ -80,13 +80,13 @@ RenderText {#text} at (0,-5) size 3x19 text run at (0,-5) width 3: "j" RenderText {#text} at (0,0) size 0x0 - RenderBlock {p} at (0,110) size 784x22 + RenderBlock {p} at (0,108) size 784x22 RenderText {#text} at (0,1) size 88x19 text run at (0,1) width 88: "long w/ both: " RenderMathMLMath {math} at (88,0) size 53x22 [padding: 0 1 0 1] RenderMathMLSubSup {msubsup} at (1,0) size 51x22 - RenderMathMLBlock (anonymous, flex) at (0,0) size 9x22 [padding: 5 0 2 0] - RenderBlock {mi} at (0,5) size 9x15 + RenderMathMLBlock (anonymous, flex) at (0,0) size 9x22 [padding: 5 0 1 0] + RenderBlock {mi} at (0,5) size 9x16 RenderText {#text} at (0,-6) size 9x25 text run at (0,-6) width 9: "Z" RenderMathMLBlock (anonymous, flex) at (9,0) size 42x22 @@ -123,14 +123,14 @@ RenderText {#text} at (0,-5) size 3x19 text run at (0,-5) width 3: "j" RenderText {#text} at (0,0) size 0x0 - RenderBlock {p} at (0,148) size 784x20 + RenderBlock {p} at (0,146) size 784x20 RenderText {#text} at (0,0) size 123x19 text run at (0,0) width 123: "Wrapped in mrow: " RenderMathMLMath {math} at (123,0) size 16x20 [padding: 0 1 0 1] RenderMathMLRow {mrow} at (1,0) size 14x20 RenderMathMLSubSup {msubsup} at (0,0) size 14x20 - RenderMathMLBlock (anonymous, flex) at (0,0) size 7x20 [padding: 7 0 3 0] - RenderBlock {mi} at (0,7) size 7x10 + RenderMathMLBlock (anonymous, flex) at (0,0) size 7x20 [padding: 7 0 2 0] + RenderBlock {mi} at (0,7) size 7x11 RenderText {#text} at (0,-9) size 7x25 text run at (0,-9) width 7: "x" RenderMathMLBlock (anonymous, flex) at (7,0) size 7x20 @@ -141,17 +141,17 @@ RenderText {#text} at (0,-4) size 5x19 text run at (0,-4) width 5: "k" RenderText {#text} at (0,0) size 0x0 - RenderBlock {p} at (0,184) size 784x52 + RenderBlock {p} at (0,182) size 784x52 RenderText {#text} at (0,15) size 169x19 text run at (0,15) width 169: "parts with various heights: " RenderMathMLMath {math} at (169,0) size 118x52 [padding: 0 1 0 1] - RenderMathMLSubSup {msubsup} at (1,0) size 20x35 - RenderMathMLBlock (anonymous, flex) at (0,0) size 7x35 [padding: 22 0 4 0] + RenderMathMLSubSup {msubsup} at (1,0) size 20x33 + RenderMathMLBlock (anonymous, flex) at (0,0) size 7x33 [padding: 22 0 2 0] RenderBlock {mi} at (0,22) size 7x9 RenderText {#text} at (0,-9) size 7x25 text run at (0,-9) width 7: "x" - RenderMathMLBlock (anonymous, flex) at (7,0) size 13x35 - RenderBlock {mi} at (0,28) size 13x7 + RenderMathMLBlock (anonymous, flex) at (7,0) size 13x33 + RenderBlock {mi} at (0,26) size 13x7 RenderText {#text} at (0,-7) size 6x19 text run at (0,-7) width 6: "n" RenderMathMLFraction {mfrac} at (1,0) size 11x26 @@ -169,8 +169,8 @@ RenderText {mo} at (0,-8) size 11x25 text run at (0,-8) width 11: "+" RenderMathMLSubSup {msubsup} at (38,18) size 20x33 - RenderMathMLBlock (anonymous, flex) at (0,0) size 7x33 [padding: 4 0 2 0] - RenderBlock {mi} at (0,4) size 7x27 + RenderMathMLBlock (anonymous, flex) at (0,0) size 7x33 [padding: 4 0 1 0] + RenderBlock {mi} at (0,4) size 7x28 RenderText {#text} at (0,-9) size 7x25 text run at (0,-9) width 7: "x" RenderMathMLBlock (anonymous, flex) at (7,0) size 13x33 @@ -213,7 +213,7 @@ RenderText {#text} at (0,-7) size 5x19 text run at (0,-7) width 5: "x" RenderText {#text} at (0,0) size 0x0 - RenderBlock {p} at (0,252) size 784x24 + RenderBlock {p} at (0,250) size 784x24 RenderText {#text} at (0,6) size 26x19 text run at (0,6) width 26: "For " RenderInline {a} at (0,0) size 71x19 [color=#0000EE] @@ -241,51 +241,51 @@ RenderText {#text} at (0,-3) size 5x15 text run at (0,-3) width 5: "2" RenderText {#text} at (0,0) size 0x0 -layer at (253,207) size 7x10 scrollHeight 19 +layer at (253,205) size 7x10 scrollHeight 19 RenderMathMLBlock (flex) {mfenced} at (0,0) size 7x10 RenderBlock (anonymous) at (0,0) size 7x10 -layer at (253,217) size 7x10 scrollHeight 18 +layer at (253,215) size 7x10 scrollHeight 18 RenderMathMLBlock (flex) {mfenced} at (0,10) size 7x10 RenderBlock (anonymous) at (0,0) size 7x10 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239C}" -layer at (253,227) size 7x10 scrollHeight 18 +layer at (253,225) size 7x10 scrollHeight 18 RenderMathMLBlock (flex) {mfenced} at (0,20) size 7x10 RenderBlock (anonymous) at (0,0) size 7x10 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239C}" -layer at (253,237) size 7x15 scrollHeight 18 +layer at (253,235) size 7x15 scrollHeight 18 RenderMathMLBlock (flex) {mfenced} at (0,30) size 7x15 RenderBlock (anonymous) at (0,0) size 7x15 -layer at (279,207) size 7x10 scrollHeight 19 +layer at (279,205) size 7x10 scrollHeight 19 RenderMathMLBlock (flex) {mfenced} at (0,0) size 7x10 RenderBlock (anonymous) at (0,0) size 7x10 -layer at (279,217) size 7x10 scrollHeight 18 +layer at (279,215) size 7x10 scrollHeight 18 RenderMathMLBlock (flex) {mfenced} at (0,10) size 7x10 RenderBlock (anonymous) at (0,0) size 7x10 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239F}" -layer at (279,227) size 7x10 scrollHeight 18 +layer at (279,225) size 7x10 scrollHeight 18 RenderMathMLBlock (flex) {mfenced} at (0,20) size 7x10 RenderBlock (anonymous) at (0,0) size 7x10 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239F}" -layer at (279,237) size 7x15 scrollHeight 18 +layer at (279,235) size 7x15 scrollHeight 18 RenderMathMLBlock (flex) {mfenced} at (0,30) size 7x15 RenderBlock (anonymous) at (0,0) size 7x15 -layer at (253,208) size 7x15 backgroundClip at (253,207) size 7x10 clip at (253,207) size 7x10 outlineClip at (253,207) size 7x10 +layer at (253,206) size 7x15 backgroundClip at (253,205) size 7x10 clip at (253,205) size 7x10 outlineClip at (253,205) size 7x10 RenderBlock (relative positioned) {mfenced} at (0,0) size 7x15 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239B}" -layer at (253,234) size 7x15 backgroundClip at (253,237) size 7x15 clip at (253,237) size 7x15 outlineClip at (253,237) size 7x15 +layer at (253,232) size 7x15 backgroundClip at (253,235) size 7x15 clip at (253,235) size 7x15 outlineClip at (253,235) size 7x15 RenderBlock (relative positioned) {mfenced} at (0,0) size 7x15 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239D}" -layer at (279,208) size 7x15 backgroundClip at (279,207) size 7x10 clip at (279,207) size 7x10 outlineClip at (279,207) size 7x10 +layer at (279,206) size 7x15 backgroundClip at (279,205) size 7x10 clip at (279,205) size 7x10 outlineClip at (279,205) size 7x10 RenderBlock (relative positioned) {mfenced} at (0,0) size 7x15 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{239E}" -layer at (279,234) size 7x15 backgroundClip at (279,237) size 7x15 clip at (279,237) size 7x15 outlineClip at (279,237) size 7x15 +layer at (279,232) size 7x15 backgroundClip at (279,235) size 7x15 clip at (279,235) size 7x15 outlineClip at (279,235) size 7x15 RenderBlock (relative positioned) {mfenced} at (0,0) size 7x15 RenderText {mfenced} at (0,-4) size 7x22 text run at (0,-4) width 7: "\x{23A0}" [1] http://trac.webkit.org/changeset/128837 [2] http://trac.webkit.org/changeset/128862
Attachments
Patch (9.63 KB, patch)
2012-10-29 20:26 PDT, Dave Barton 		no flags
DetailsFormatted DiffDiff
Patch (9.58 KB, patch)
2012-11-01 11:16 PDT, Dave Barton 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dave Barton
Comment 1 2012-09-30 11:10:53 PDT
I believe the problem is that RenderMathMLSubSup::layout should now mark more sub-parts as needing layout, because of flex stretching. I'll be unavailable most of this week, so I'll probably get to this the week of Oct 8. Feel free to cc me on future MathML issues.
Zan Dobersek
Comment 2 2012-10-04 01:45:37 PDT
(In reply to comment #1) > I believe the problem is that RenderMathMLSubSup::layout should now mark more sub-parts as needing layout, because of flex stretching. I'll be unavailable most of this week, so I'll probably get to this the week of Oct 8. > > Feel free to cc me on future MathML issues. The test is only flaky on GTK 64-bit release builder though: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=mathml%2Fpresentation%2Fsubsup.xhtml The nature of regressing commit made me think that the GTK port has some issues with the flexbox model and that resolving this could perhaps make a few other tests more stable. If you'd still like to have a look please go ahead, but it seems to me this flakiness is originating from a GTK-specific problem.
Dave Barton
Comment 3 2012-10-09 17:54:55 PDT
I have a patch for this, which I'll upload after bug 98791 closes. I think it's my MathML mistake, not a gtk problem. Thanks for reporting it so well, and sorry for the bug. :)
Dave Barton
Comment 4 2012-10-24 17:13:32 PDT
subsup.xhtml and msubsup-base-changed.xhtml are flaky on chromium also. The problem is now that MathML is using flexboxes, we have to do firstChild()->style()->setFlexDirection(FlowColumn); at some point [basically inside fixScriptsStyle(), renamed to fixChildStyles()], so that baseHeight in layout() will be an unstretched height. That is, we don't want flexing to stretch our base's height. I'll upload a patch after bug 98791 closes.
Dave Barton
Comment 5 2012-10-29 20:26:53 PDT
Created attachment 171365 [details] Patch
Dave Barton
Comment 6 2012-10-29 20:35:14 PDT
I feel bad always asking Eric for reviews. Any chance Ojan or Tony wants to review a quick MathML/flexbox issue? This patch just changes the flex direction of the RenderMathMLSubSup base wrapper that Ojan's been looking at lately. :) That's the wrapper that surrounds a term's "base", i.e. the part without a subscript and/or superscript (exponent).
Eric Seidel (no email)
Comment 7 2012-10-29 22:05:25 PDT
Comment on attachment 171365 [details] Patch Seems reasonable to me. Tony/Ojan are the flexbox experts more than I, so you might leave them until morning to review.
Ojan Vafai
Comment 8 2012-10-30 09:17:39 PDT
Comment on attachment 171365 [details] Patch Do you just want to disable stretching? You could just set firstChild->style()->setAlignItems(FlexStart) for that. I'm not sure what else making it a column buys you here other than avoiding stretching.
Ojan Vafai
Comment 9 2012-10-30 09:18:42 PDT
(In reply to comment #8) > (From update of attachment 171365 [details]) > Do you just want to disable stretching? You could just set firstChild->style()->setAlignItems(FlexStart) for that. I'm not sure what else making it a column buys you here other than avoiding stretching. Sorry, that should have been setAlignSelf.
Dave Barton
Comment 10 2012-10-30 10:15:18 PDT
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 171365 [details] [details]) > > Do you just want to disable stretching? You could just set firstChild->style()->setAlignItems(FlexStart) for that. I'm not sure what else making it a column buys you here other than avoiding stretching. > > Sorry, that should have been setAlignSelf. Thanks for the expert comment. <mmultiscripts> will probably use this or similar code, and it seemed maybe simpler especially in that case if all the children (base and subscript/superscript pairs) were 'column', instead of all but one. Also perhaps the padding-top on the base wrapper is really more of a margin-top, but that might possibly interact with flex-start, maybe in the future? I think the specs on alignment/justification, i.e. CSS Box Alignment Module Level 3, are still evolving, so I was waiting on that. In summary, I think either way would work, but this was my thinking on my choice. I'd be happy to change if you recommend it.
Ojan Vafai
Comment 11 2012-10-31 13:03:38 PDT
I don't know that it makes a big difference either way. There are cases where using column will cause a second layout, so, all things being equal, I'd adjust the alignment instead.
Dave Barton
Comment 12 2012-11-01 11:16:56 PDT
Created attachment 171894 [details] Patch
WebKit Review Bot
Comment 13 2012-11-01 13:42:49 PDT
Comment on attachment 171894 [details] Patch Clearing flags on attachment: 171894 Committed r133221: <http://trac.webkit.org/changeset/133221>
WebKit Review Bot
Comment 14 2012-11-01 13:42:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.