Bug 52444

Summary:

After appending MathML with jquery the table renders with overlaps

Product:

WebKit

Reporter:

Eckhard Nees <reklame.nees>

Component:

MathML

Assignee:

Alex Milowski <alex>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alex, davidc, dbarton, eric, fred.wang, jchaffraix, mitz, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

Bug Depends on:

78034, 78180

Bug Blocks:

Attachments:

Description	Flags
The rendered Math	none
After generating a PDF-preview	none
A smaller example - after clicking on "move" to move the DOM subtree containing MathML, the X overlaps the previous expression	none
Patch	none
Patch for review only	none
Layout test file, with new test case at the end	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Eckhard Nees

Reported 2011-01-14 05:56:30 PST

Created attachment 78933 [details] The rendered Math After inserting the following MATHML-Code <math id="f1" xmlns="http://www.w3.org/1998/Math/MathML"><mtable><mtr><mtd><mi>c)</mi></mtd><mtd><mrow><mn>9</mn><mn>-9</mn><mo>(</mo><mi>d</mi><mo>+</mo><mn>7</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-2</mn><mi>d</mi><mo>+</mo><mn>6</mn><mn>-3</mn><mi>d</mi></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-9</mn><mi>d</mi><mn>-54</mn></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-5</mn><mi>d</mi><mo>+</mo><mn>6</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>+</mo><mn>5</mn><mi>d</mi><mo>+</mo><mn>54</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-4</mn><mi>d</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>60</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>:</mo><mo>(</mo><mrow><mn>-4</mn></mrow><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mi>d</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mn><mn>60</mn></mn><mn><mn>-4</mn></mn></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mi>d</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-15</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>linke Seite:</mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>9</mn><mn>-9</mn><mo>(</mo><mn>-15</mn><mo>+</mo><mn>7</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>9</mn><mn>-9</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mn>-8</mn></mrow><mo><mo>)</mo></mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>9</mn><mo>+</mo><mn>72</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>81</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>rechte Seite<mo>:</mo></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-2</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mn>-15</mn></mrow><mo><mo>)</mo></mo><mo>+</mo><mn>6</mn><mn>-3</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mn>-15</mn></mrow><mo><mo>)</mo></mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>30</mn><mo>+</mo><mn>6</mn><mo>+</mo><mn>45</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>81</mn></mrow></mtd></mtr><mtr><mtd><mi>d)</mi></mtd><mtd><mrow><mn>-6</mn><mo>(</mo><mn>-1</mn><mn>-10</mn><mi>x</mi><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-7,5</mn><mo>(</mo><mn>2</mn><mi>x</mi><mo>+</mo><mn>7</mn><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>60</mn><mi>x</mi><mo>+</mo><mn>6</mn></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-15</mn><mi>x</mi><mn>-52,5</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>+</mo><mn>15</mn><mi>x</mi><mn>-6</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>75</mn><mi>x</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-58,5</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>:</mo><mn>75</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mi>x</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mn><mn>-58,5</mn></mn><mn><mn>75</mn></mn></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mi>x</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-0,78</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>linke Seite:</mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-6</mn><mo>(</mo><mn>-1</mn><mn>-10</mn><mo>·</mo><mn>-0,78</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-6</mn><mo>(</mo><mn>-1</mn><mo>+</mo><mn>7,8</mn><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-6</mn><mo>·</mo><mn>6,8</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-40,8</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>rechte Seite<mo>:</mo></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-7,5</mn><mo>(</mo><mn>2</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mn>-0,78</mn></mrow><mo><mo>)</mo></mo><mo>+</mo><mn>7</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-7,5</mn><mo>(</mo><mn>-1,56</mn><mo>+</mo><mn>7</mn><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-7,5</mn><mo>·</mo><mn>5,44</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-40,8</mn></mrow></mtd></mtr><mtr><mtd><mi>e)</mi></mtd><mtd><mrow><mn>-4</mn><mn>-2</mn><mo>(</mo><mi>b</mi><mo>+</mo><mn>5</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-9</mn><mi>b</mi><mn>-8</mn><mn>-4</mn><mi>b</mi></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-2</mn><mi>b</mi><mn>-14</mn></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-13</mn><mi>b</mi><mn>-8</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>+</mo><mn>13</mn><mi>b</mi><mo>+</mo><mn>14</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>11</mn><mi>b</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>6</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>:</mo><mn>11</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mi>b</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mn><mn>6</mn></mn><mn><mn>11</mn></mn></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>linke Seite:</mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-4</mn><mn>-2</mn><mo>(</mo><mfrac><mrow><mn>6</mn></mrow><mrow><mn>11</mn></mrow></mfrac><mo>+</mo><mn>5</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-4</mn><mn>-2</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mn>5</mn><mfrac><mrow><mn>6</mn></mrow><mrow><mn>11</mn></mrow></mfrac></mrow><mo><mo>)</mo></mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-4</mn><mn>-11</mn><mfrac><mrow><mn>1</mn></mrow><mrow><mn>11</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-15</mn><mfrac><mrow><mn>1</mn></mrow><mrow><mn>11</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>rechte Seite<mo>:</mo></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>-9</mn><mo>·</mo><mfrac><mrow><mn>6</mn></mrow><mrow><mn>11</mn></mrow></mfrac><mn>-8</mn><mn>-4</mn><mo>·</mo><mfrac><mrow><mn>6</mn></mrow><mrow><mn>11</mn></mrow></mfrac></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-4</mn><mfrac><mrow><mn>10</mn></mrow><mrow><mn>11</mn></mrow></mfrac><mn>-8</mn><mn>-2</mn><mfrac><mrow><mn>2</mn></mrow><mrow><mn>11</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-15</mn><mfrac><mrow><mn>1</mn></mrow><mrow><mn>11</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd><mi>f)</mi></mtd><mtd><mrow><mo>(</mo><mi>r</mi><mo>+</mo><mn>2</mn><mo>)</mo><mo>(</mo><mn>4</mn><mi>r</mi><mn>-9</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>4</mn><mi>r</mi><msup><mn>2</mn></msup><mo>+</mo><mn>2</mn><mo>(</mo><mn>-4</mn><mi>r</mi><mn>-10</mn><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>4</mn><mi>r</mi><msup><mn>2</mn></msup><mn>-9</mn><mi>r</mi><mo>+</mo><mn>8</mn><mi>r</mi><mn>-18</mn></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>4</mn><mi>r</mi><msup><mn>2</mn></msup><mn>-8</mn><mi>r</mi><mn>-20</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mn>-4</mn><mi>r</mi><msup><mn>2</mn></msup></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mo form="prefix">−</mo><mi>r</mi><mn>-18</mn></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-8</mn><mi>r</mi><mn>-20</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>+</mo><mn>8</mn><mi>r</mi><mo>+</mo><mn>18</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>7</mn><mi>r</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-2</mn></mrow></mtd><mtd><mo>|</mo></mtd><mtd><mrow><mo>:</mo><mn>7</mn></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mi>r</mi></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mn><mn>-2</mn></mn><mn><mn>7</mn></mn></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>linke Seite:</mtd></mtr><mtr><mtd></mtd><mtd><mrow><mo>(</mo><mo form="prefix">−</mo><mfrac><mrow><mn>-2</mn></mrow><mrow><mn>7</mn></mrow></mfrac><mo>+</mo><mn>2</mn><mo>)</mo><mo>(</mo><mn>4</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mo form="prefix">−</mo><mfrac><mrow><mn>-2</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow><mo><mo>)</mo></mo><mn>-9</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>1</mn><mfrac><mrow><mn>5</mn></mrow><mrow><mn>7</mn></mrow></mfrac><mo>(</mo><mn>-1</mn><mfrac><mrow><mn>1</mn></mrow><mrow><mn>7</mn></mrow></mfrac><mn>-9</mn><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>1</mn><mfrac><mrow><mn>5</mn></mrow><mrow><mn>7</mn></mrow></mfrac><mo>·</mo><mn>-10</mn><mfrac><mrow><mn>1</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-17</mn><mfrac><mrow><mn>19</mn></mrow><mrow><mn>49</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow>Probe</mrow></mtd><mtd> </mtd><mtd>rechte Seite<mo>:</mo></mtd></mtr><mtr><mtd></mtd><mtd><mrow><mn>4</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mo form="prefix">−</mo><mfrac><mrow><mn>-2</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow><mo><mo>)</mo></mo><mo>·</mo><mo><mo>(</mo></mo><mrow><mo form="prefix">−</mo><mfrac><mrow><mn>-2</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow><mo><mo>)</mo></mo><mo>+</mo><mn>2</mn><mo>(</mo><mn>-4</mn><mo>·</mo><mo><mo>(</mo></mo><mrow><mo form="prefix">−</mo><mfrac><mrow><mn>-2</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow><mo><mo>)</mo></mo><mn>-10</mn><mo>)</mo></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mrow><mn>16</mn></mrow><mrow><mn>49</mn></mrow></mfrac><mo>+</mo><mn>2</mn><mo>(</mo><mn>1</mn><mfrac><mrow><mn>1</mn></mrow><mrow><mn>7</mn></mrow></mfrac><mn>-10</mn><mo>)</mo></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mrow><mn>16</mn></mrow><mrow><mn>49</mn></mrow></mfrac><mo>+</mo><mn>2</mn><mo>·</mo><mn>-8</mn><mfrac><mrow><mn>6</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mfrac><mrow><mn>16</mn></mrow><mrow><mn>49</mn></mrow></mfrac><mn>-17</mn><mfrac><mrow><mn>5</mn></mrow><mrow><mn>7</mn></mrow></mfrac></mrow></mtd></mtr><mtr><mtd></mtd><mtd><mrow></mrow></mtd><mtd><mo>=</mo></mtd><mtd><mrow><mn>-17</mn><mfrac><mrow><mn>19</mn></mrow><mrow><mn>49</mn></mrow></mfrac></mrow></mtd></mtr></mtable></math> the document is rendered as shown in the picture. I use the jquery-Command .append("<math....>"; Wenn I tried to print the page, the output was correct by the browser as shown in the second picture.

Attachments
The rendered Math (23.46 KB, image/tiff) 2011-01-14 05:56 PST, Eckhard Nees	no flags	Details
After generating a PDF-preview (24.49 KB, image/tiff) 2011-01-14 05:57 PST, Eckhard Nees	no flags	Details
A smaller example - after clicking on "move" to move the DOM subtree containing MathML, the X overlaps the previous expression (394 bytes, text/html) 2011-11-20 10:39 PST, Dave Barton	no flags	Details
Patch (588.32 KB, patch) 2012-01-29 18:33 PST, Dave Barton	no flags	Details Formatted Diff Diff
Patch for review only (70.53 KB, patch) 2012-04-14 11:37 PDT, Dave Barton	no flags	Details Formatted Diff Diff
Layout test file, with new test case at the end (2.23 KB, text/html) 2012-04-14 11:55 PDT, Dave Barton	no flags	Details
Patch (70.78 KB, patch) 2012-04-17 15:44 PDT, Dave Barton	no flags	Details Formatted Diff Diff
Patch (71.21 KB, patch) 2012-04-18 13:06 PDT, Dave Barton	no flags	Details Formatted Diff Diff
Patch (71.30 KB, patch) 2012-04-25 22:10 PDT, Dave Barton	no flags	Details Formatted Diff Diff
Patch (71.43 KB, patch) 2012-05-01 16:40 PDT, Dave Barton	no flags	Details Formatted Diff Diff
Patch (71.44 KB, patch) 2012-05-02 13:59 PDT, Dave Barton	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Eckhard Nees

Comment 1 2011-01-14 05:57:22 PST

Created attachment 78934 [details] After generating a PDF-preview

Dave Barton

Comment 2 2011-11-20 10:39:30 PST

Created attachment 115986 [details] A smaller example - after clicking on "move" to move the DOM subtree containing MathML, the X overlaps the previous expression

Dave Barton

Comment 3 2012-01-29 18:33:40 PST

Created attachment 124483 [details] Patch

Dave Barton

Comment 4 2012-01-29 18:47:18 PST

Sorry this patch is so large. As mentioned in the ChangeLog, I had to clean up and resolve pieces of formatting/layout code in various places, which affected (improved) a lot of tests slightly. This patch does also fix bugs 72834, 57695, and 47781.

Dave Barton

Comment 5 2012-01-30 11:22:10 PST

*** Bug 72834 has been marked as a duplicate of this bug. ***

Alex Milowski

Comment 6 2012-01-30 12:54:12 PST

I will try to take a look at this patch today or tomorrow.

Darin Adler

Comment 7 2012-02-06 17:09:18 PST

Comment on attachment 124483 [details] Patch There is no need to have the patch be this bug. There are quite a few things you can do to make this patch smaller. There’s no need to mix in the int/LayoutUnit changes. You should do those in a separate patch first. Similarly, the changes of type from Node* to Element* seem clearly like something that can go in a separate patch. And fixes to style, such as returning RefPtr when we should return PassRefPtr could go in a separate patch. Things like cleaning up includes could also go in separately. Changes to whitespace could go in one of those patches. Then you’d have a patch that limits itself to the substantive changes. I suspect the substantive changes can also be staged rather than landing them all at once. Please do this in smaller steps.

Dave Barton

Comment 8 2012-02-07 14:11:53 PST

Darin, thanks very much for the suggestions. I will split this patch into several smaller ones, starting with bug 78034.

Dave Barton

Comment 9 2012-04-14 11:37:06 PDT

Created attachment 137214 [details] Patch for review only

Dave Barton

Comment 10 2012-04-14 11:52:37 PDT

Comment on attachment 137214 [details] Patch for review only I am uploading this patch because I need guidance for my FIXME question in RenderMathMLBlock::sizeChildrenInWide(). As explained there, MathML basically often needs to do layout at computePreferredLogicalWidths() time, to calculate preferred logical heights. These are needed due to operator stretching and other layout fine points. These issues could affect other renderers of the future in principle, not just MathML. Any advice or pointers are greatly appreciated!

Dave Barton

Comment 11 2012-04-14 11:55:02 PDT

Created attachment 137217 [details] Layout test file, with new test case at the end I'm adding this so it's easy to view the new layout test case in existing WebKit.

Dave Barton

Comment 12 2012-04-17 15:44:31 PDT

Created attachment 137622 [details] Patch

Dave Barton

Comment 13 2012-04-17 15:47:00 PDT

I found and read the code for repaint() logic, and I think this patch is now ok.

Julien Chaffraix

Comment 14 2012-04-18 09:54:05 PDT

Comment on attachment 137622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137622&action=review > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:49 > + , m_preferredLogicalHeight(-1) We usually avoid magic constants: static LayoutUnit preferredLogicalHeightUnset = -1; > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:164 > + // Ensure a full repaint will happen eventually. > + setNeedsLayout(true, MarkOnlyThis); It really looks like you are abusing the existing logic to need to force a layout to properly repaint (our repaint logic is far from being obvious or unified unfortunately). I guess this is due to the fact that your order of operations is different from the normal case. I don't have an alternative solution though and would need to think about it so don't feel blocked by that. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:167 > + setLogicalWidth(15000); // an arbitrary large value, like RenderBlock.cpp BLOCK_MAX_WIDTH Again, we prefer to avoid magic constants. Should it be kept in sync with the value of BLOCK_MAX_WIDTH as FixedTableLayout.cpp's TABLE_MAX_WIDTH? > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:186 > +LayoutUnit RenderMathMLBlock::preferredLogicalHeightAfterSizing(RenderObject* child) I am not sure I understand the "after sizing" part. What do we size here? The children? > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:70 > + LayoutUnit preferredLogicalHeightIfOK() const { return preferredLogicalWidthsDirty() ? -1 : m_preferredLogicalHeight; } I am not a huge fan of that function. Your use case seems to be either to check that it is set (basically != -1) or to get the value. This means you could just have 2 new functions: * bool isPreferredLogicalHeightSet() const { return !preferredLogicalWidthsDirty() && m_preferredLogicalHeight != -1; } * LayoutUnit preferredLogicalHeight() const { ASSERT(!preferredLogicalWidthsDirty()); return m_preferredLogicalHeight; } > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:108 > + void sizeChildrenInWide(); The naming is not 100% to me. What does "in wide" means? Does it have a special meaning in MathML? > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:45 > + virtual void updateFromElement(); > + > + virtual RenderMathMLOperator* unembellishedOperator() { return this; } OVERRIDE? > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:67 > + if (child->isRenderMathMLBlock()) { > + RenderMathMLOperator* renderMo = toRenderMathMLBlock(child)->unembellishedOperator(); > + // FIXME: Only skip renderMo if it is stretchy. > + if (renderMo) > + continue; > } I am wondering if this shouldn't be part of your preferredLogicalHeightAfterSizing. That would likely make the code easier to read and you are already checking for some of those conditions anyway.

Dave Barton

Comment 15 2012-04-18 13:00:32 PDT

Comment on attachment 137622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137622&action=review >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:164 >> + setNeedsLayout(true, MarkOnlyThis); > > It really looks like you are abusing the existing logic to need to force a layout to properly repaint (our repaint logic is far from being obvious or unified unfortunately). I guess this is due to the fact that your order of operations is different from the normal case. I don't have an alternative solution though and would need to think about it so don't feel blocked by that. I'd say "extending" not "abusing". Not all layout is normal-flow inline or block. Those are just the easiest, especially for incremental layout (e.g. in the old days of slow downloads). Just like floats, absolute positioning, and tables, we need to allow for more arbitrary bottom-up layout. Doubtless we'll discuss this with others at the annual meeting tomorrow. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:167 >> + setLogicalWidth(15000); // an arbitrary large value, like RenderBlock.cpp BLOCK_MAX_WIDTH > > Again, we prefer to avoid magic constants. Should it be kept in sync with the value of BLOCK_MAX_WIDTH as FixedTableLayout.cpp's TABLE_MAX_WIDTH? I'll introduce named const variables as you suggest. This value does not need to be kept in sync with the others. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:186 >> +LayoutUnit RenderMathMLBlock::preferredLogicalHeightAfterSizing(RenderObject* child) > > I am not sure I understand the "after sizing" part. What do we size here? The children? I'll add a comment. This must be called after sizeChildrenInWide(), as it assumes the children have been sized (had their preferred logical heights calculated). >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:70 >> + LayoutUnit preferredLogicalHeightIfOK() const { return preferredLogicalWidthsDirty() ? -1 : m_preferredLogicalHeight; } > > I am not a huge fan of that function. Your use case seems to be either to check that it is set (basically != -1) or to get the value. This means you could just have 2 new functions: > * bool isPreferredLogicalHeightSet() const { return !preferredLogicalWidthsDirty() && m_preferredLogicalHeight != -1; } > * LayoutUnit preferredLogicalHeight() const { ASSERT(!preferredLogicalWidthsDirty()); return m_preferredLogicalHeight; } I'll basically do what you suggest. I like the word "Dirty" more than "not Set" because even if we have set m_preferredLogicalHeight previously, it's no longer valid if preferredLogicalWidthsDirty(). >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:108 >> + void sizeChildrenInWide(); > > The naming is not 100% to me. What does "in wide" means? Does it have a special meaning in MathML? It means what the comment says, set our logical width to a large value, and then compute our children's preferred logical heights. This is a protected function so its use is pretty specialized. This is the best name I could think of for this behavior. I could change it to setWideAndSizeChildren() or something else if you'd like. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:45 >> + virtual RenderMathMLOperator* unembellishedOperator() { return this; } > > OVERRIDE? Will do. >> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:67 >> } > > I am wondering if this shouldn't be part of your preferredLogicalHeightAfterSizing. That would likely make the code easier to read and you are already checking for some of those conditions anyway. We're only supposed to do (vertical) operator stretching inside an (explicit or implicit) mrow (or sometimes an mtable - that's not implemented yet). If one has e.g. a bare operator in a subscript or superscript or mroot index or etc., its parent renderer may need to know its preferred logical height, but whether it's a stretchy operator shouldn't matter, and so shouldn't be checked in preferredLogicalHeightAfterSizing().

Dave Barton

Comment 16 2012-04-18 13:06:09 PDT

Created attachment 137750 [details] Patch

Julien Chaffraix

Comment 17 2012-04-23 12:49:15 PDT

Comment on attachment 137750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137750&action=review Sorry for the long delay. More comments. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:160 > +static const LayoutUnit largeLogicalWidth = 15000; There is not really a convention on how to name static variable or constant. However it's usually preferred to somehow prefix it so that it's clear that it is a constant. Some people would use cLargeLogicalWidth (for constant) or sLargeLogicalWidth (for static). I would go for the c-prefixed but that's your call. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:169 > + LayoutUnit oldAvailableLogicalWidth = everHadLayout() ? availableLogicalWidth() : 0; Shouldn't it just be oldAvailableLogicalWidth = availableLogicalWidth()? > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:183 > + child->layoutIfNeeded(); This would definitely need a comment as IIRC that's needed because you stretch your operators during layout. Is it stupid to wonder if we couldn't do that at computeLogicalWidths time? (maybe just as a FIXME here explaining why and what is preventing us for now) > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:194 > + ASSERT(!child->needsLayout()); Maybe worth moving it at the beginning of the function? > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:111 > + // Set our logical width to a large value, and compute our children's preferred logical heights. > + void sizeChildrenInWide(); I really don't understand the InWide part of the name. IMHO it can be just dropped and you could just call it prepareChildrenForLogicalWidthsComputation. > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:113 > + // This can only be called after children have been sized by sizeChildrenInWide(). > + static LayoutUnit preferredLogicalHeightAfterSizing(RenderObject* child); I don't think I understand the AfterSizing part, you don't enforce the constraint and AFAICT it would work at any time. I would just drop the AfterSizing part and call it preferredLogicalHeightForObject. > Source/WebCore/rendering/mathml/RenderMathMLRow.h:47 > + Trailing spaces.

Dave Barton

Comment 18 2012-04-24 21:17:10 PDT

Comment on attachment 137750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137750&action=review This is new, calling layout() inside computePreferredLogicalWidths(). I appreciate you thinking it through with me! I'll submit another patch. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:160 >> +static const LayoutUnit largeLogicalWidth = 15000; > > There is not really a convention on how to name static variable or constant. However it's usually preferred to somehow prefix it so that it's clear that it is a constant. Some people would use cLargeLogicalWidth (for constant) or sLargeLogicalWidth (for static). I would go for the c-prefixed but that's your call. Will do. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:169 >> + LayoutUnit oldAvailableLogicalWidth = everHadLayout() ? availableLogicalWidth() : 0; > > Shouldn't it just be oldAvailableLogicalWidth = availableLogicalWidth()? You're right. I was worried about uninitialized values, but m_frameRect/etc. are initialized to zeros. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:183 >> + child->layoutIfNeeded(); > > This would definitely need a comment as IIRC that's needed because you stretch your operators during layout. Is it stupid to wonder if we couldn't do that at computeLogicalWidths time? (maybe just as a FIXME here explaining why and what is preventing us for now) There's a comment in RenderMathMLBlock.h explaining the need for preferred logical heights at all, because of e.g. operator stretching. The whole purpose of this function is to compute each child's preferred logical height if necessary. I will add a comment saying that this layoutIfNeeded() is to do that. The RenderMathMLBlock.h comment also explains that this must happen at computePreferredLogicalWidths time, before computeLogicalWidths in this->layout(). >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:194 >> + ASSERT(!child->needsLayout()); > > Maybe worth moving it at the beginning of the function? Actually if the child is a RenderMathMLBlock which already had its preferredLogicalHeight set, then it wouldn't have been laid out again in sizeChildrenInWide() so it conceivably could need layout now. For instance, its parent's width may have just changed. If the child is just a RenderInline then I think it could need layout also. We just need its fontSize() here in that case. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:111 >> + void sizeChildrenInWide(); > > I really don't understand the InWide part of the name. IMHO it can be just dropped and you could just call it prepareChildrenForLogicalWidthsComputation. I kind of like your name, but it's at least part of the contract (comment) for this function that it sets this->logicalWidth() to a large value. This is used for instance at the end of RenderMathMLRow::computePreferredLogicalWidths(), where we do setLogicalWidth(maxPreferredLogicalWidth()); without marking our children as needing layout again. I'll add an ASSERT() there to make this clearer. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:113 >> + static LayoutUnit preferredLogicalHeightAfterSizing(RenderObject* child); > > I don't think I understand the AfterSizing part, you don't enforce the constraint and AFAICT it would work at any time. I would just drop the AfterSizing part and call it preferredLogicalHeightForObject. If you haven't called sizeChildrenInWide() first, then needed ASSERT()s may fail. If child->isRenderMathMLBlock(), then ASSERT(!isPreferredLogicalHeightDirty()) may fail in toRenderMathMLBlock(child)->preferredLogicalHeight(). Also, if just child->isBox(), then ASSERT(!child->needsLayout()) could fail. >> Source/WebCore/rendering/mathml/RenderMathMLRow.h:47 >> + > > Trailing spaces. Will fix.

Dave Barton

Comment 19 2012-04-25 22:10:55 PDT

Created attachment 138933 [details] Patch

Julien Chaffraix

Comment 20 2012-04-30 15:10:59 PDT

Comment on attachment 138933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138933&action=review > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:111 > + void sizeChildrenInWide(); As far as the contract for this function goes, if you change it, you would have to rename your function which would be annoying as what it does is only preparing your child. Note that I am a native speaker which may be why "size children in wide" doesn't speak at all to me. Please consider renaming to anything that makes more sense. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:67 > - if (height == m_stretchHeight) > + if (m_stretchHeight == static_cast<int>(height * gOperatorExpansion)) > return; > m_stretchHeight = static_cast<int>(height * gOperatorExpansion); You can factor this out to avoid 2 conversions: int operatorExpandedHeight = height * gOperatorExpansion; if (m_stretchHeight == operatorExpandedHeight) return; m_stretchHeight = operatorExpandedHeight;

Eric Seidel (no email)

Comment 21 2012-04-30 15:13:03 PDT

Is the bug title supposed to say MathML? (Instead of MATHM?)

Dave Barton

Comment 22 2012-04-30 19:18:21 PDT

Comment on attachment 138933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138933&action=review I think we are struggling with naming because this is a new kind of layout, bottom-up not top-down. So I think iterating on the names has been worth it. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:111 >> + void sizeChildrenInWide(); > > As far as the contract for this function goes, if you change it, you would have to rename your function which would be annoying as what it does is only preparing your child. Note that I am a native speaker which may be why "size children in wide" doesn't speak at all to me. Please consider renaming to anything that makes more sense. How about computePreferredLogicalHeightsForChildren or computeChildrenPreferredLogicalHeights? >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:67 >> m_stretchHeight = static_cast<int>(height * gOperatorExpansion); > > You can factor this out to avoid 2 conversions: > > int operatorExpandedHeight = height * gOperatorExpansion; > if (m_stretchHeight == operatorExpandedHeight) > return; > m_stretchHeight = operatorExpandedHeight; Will do.

Julien Chaffraix

Comment 23 2012-05-01 08:32:20 PDT

Comment on attachment 138933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138933&action=review >>> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:111 >>> + void sizeChildrenInWide(); >> >> As far as the contract for this function goes, if you change it, you would have to rename your function which would be annoying as what it does is only preparing your child. Note that I am a native speaker which may be why "size children in wide" doesn't speak at all to me. Please consider renaming to anything that makes more sense. > > How about computePreferredLogicalHeightsForChildren or computeChildrenPreferredLogicalHeights? I am fine with either as they reflect better what is done. Small preference for computeChildrenPreferredLogicalHeights as it's a bit shorter.

Dave Barton

Comment 24 2012-05-01 16:40:15 PDT

Created attachment 139709 [details] Patch

WebKit Review Bot

Comment 25 2012-05-01 16:42:43 PDT

Comment on attachment 139709 [details] Patch Rejecting attachment 139709 [details] from commit-queue. dbarton@mathscribe.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

WebKit Review Bot

Comment 26 2012-05-02 13:20:23 PDT

Comment on attachment 139709 [details] Patch Rejecting attachment 139709 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12610040

Dave Barton

Comment 27 2012-05-02 13:59:38 PDT

Created attachment 139880 [details] Patch

WebKit Review Bot

Comment 28 2012-05-02 14:59:22 PDT

Comment on attachment 139880 [details] Patch Clearing flags on attachment: 139880 Committed r115895: <http://trac.webkit.org/changeset/115895>

WebKit Review Bot

Comment 29 2012-05-02 14:59:40 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.