Bug 52444 - After appending MathML with jquery the table renders with overlaps
Summary: After appending MathML with jquery the table renders with overlaps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Alex Milowski
URL:
Keywords:
: 72834 (view as bug list)
Depends on: 78034 78180
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-14 05:56 PST by Eckhard Nees
Modified: 2012-05-02 14:59 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eckhard Nees 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.
Comment 1 Eckhard Nees 2011-01-14 05:57:22 PST
Created attachment 78934 [details]
After generating a PDF-preview
Comment 2 Dave Barton 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
Comment 3 Dave Barton 2012-01-29 18:33:40 PST
Created attachment 124483 [details]
Patch
Comment 4 Dave Barton 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.
Comment 5 Dave Barton 2012-01-30 11:22:10 PST
*** Bug 72834 has been marked as a duplicate of this bug. ***
Comment 6 Alex Milowski 2012-01-30 12:54:12 PST
I will try to take a look at this patch today or tomorrow.
Comment 7 Darin Adler 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.
Comment 8 Dave Barton 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.
Comment 9 Dave Barton 2012-04-14 11:37:06 PDT
Created attachment 137214 [details]
Patch for review only
Comment 10 Dave Barton 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!
Comment 11 Dave Barton 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.
Comment 12 Dave Barton 2012-04-17 15:44:31 PDT
Created attachment 137622 [details]
Patch
Comment 13 Dave Barton 2012-04-17 15:47:00 PDT
I found and read the code for repaint() logic, and I think this patch is now ok.
Comment 14 Julien Chaffraix 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.
Comment 15 Dave Barton 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().
Comment 16 Dave Barton 2012-04-18 13:06:09 PDT
Created attachment 137750 [details]
Patch
Comment 17 Julien Chaffraix 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.
Comment 18 Dave Barton 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.
Comment 19 Dave Barton 2012-04-25 22:10:55 PDT
Created attachment 138933 [details]
Patch
Comment 20 Julien Chaffraix 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;
Comment 21 Eric Seidel (no email) 2012-04-30 15:13:03 PDT
Is the bug title supposed to say MathML? (Instead of MATHM?)
Comment 22 Dave Barton 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.
Comment 23 Julien Chaffraix 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.
Comment 24 Dave Barton 2012-05-01 16:40:15 PDT
Created attachment 139709 [details]
Patch
Comment 25 WebKit Review Bot 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.
Comment 26 WebKit Review Bot 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
Comment 27 Dave Barton 2012-05-02 13:59:38 PDT
Created attachment 139880 [details]
Patch
Comment 28 WebKit Review Bot 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>
Comment 29 WebKit Review Bot 2012-05-02 14:59:40 PDT
All reviewed patches have been landed.  Closing bug.