WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
52444
After appending MathML with jquery the table renders with overlaps
https://bugs.webkit.org/show_bug.cgi?id=52444
Summary
After appending MathML with jquery the table renders with overlaps
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.
Top of Page
Format For Printing
XML
Clone This Bug