1219068 - MathML vertical line is too big.

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[dalgakiran17]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151026170526

Steps to reproduce:

Use this code:
<math class="ma-block" display="block" alttext="|f(x)-f(x_0)|"><mrow><mo>|</mo><mrow><mrow><mi>f</mi><mrow><mo>(</mo><mi>x</mi><mo>)</mo></mrow></mrow><mo>−</mo><mrow><mi>f</mi><mrow><mo>(</mo><msub><mi>x</mi><mn>0</mn></msub><mo>)</mo></mrow></mrow></mrow><mo>|</mo></mrow></math>

Check the attached html file or the video on youtube (video id: xhBqMBEPWA8)

Firefox Version: 42.0
OS: Microsoft Windows 7 Ultimate x64


Actual results:

Vertical line looks taller than it should be.


Expected results:

The vertical lines should have the same height with the characters inside it.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8adf9f289057&tochange=ac4d26019611


Regresed by:
ac4d26019611	Frédéric Wang — Bug 947654 - Use font.*.x-math font preferences for MathML. r=karl, r=heycam
This also rearranges the list of default fonts and changes the preferred font to Latin Modern Math on all platforms.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression. Too late for 41 or 42 fixes but we could take a patch for beta 43.

Comment 3

[Frédéric Wang (:fredw)]

James: are you able to test what's happening on Windows?

Comment 4

[James Kitchener (:jkitch)]

I can reproduce this.  Occurs with Cambria Math, but not with Latin Modern Math.

Triggered by the presence of parentheses within the expression, still occurs if parentheses are placed in a non-<mo> token frame.

Selection highlight shows the correct dimensions, and there is no overlap with a subsequent line.

Comment 5

[James Kitchener (:jkitch)]

The problem lies within nsMathMlChar's stretching code (specifically StretchEnumContext::TryVariants).

The size of the first glyph tested is bigger than both the best size and the target size, sufficiently so that it fails the IsSizeBetter test.  The algorithm's response to that is to try an even bigger glyph which will naturally fail the same tests.  This process continues with increasingly bigger glyphs until the algorithm gives up - here the algorithm will return the last size tested, which it mistakenly thinks to be the closest match.

I'll look into tweaking the algorithm to return the first possible glyph/size should this situation be detected.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Too late for 42! And not critical enough to do a dot releasE.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfixing for 43 at this point. If you end up landing a patch and feel strongly about it being on 43, please let me know.

Comment 8

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2773c3852ec

Comment 9

[James Kitchener (:jkitch)]

Attachment: patch

The attached patch detects if the first character size tested exceeds the acceptable size range and that testing increasingly large sizes would only makes things worse.

In this situation, the first size/glyph tested is adopted.  Testing other fonts for better matches has been considered, but rendering in multiple fonts seems a bad idea - the fonts used will change with minor adjustments to sizing.

The rest of the stretching algorithm is otherwise untouched.  A nicer solution may be possible, but it would require considerable care to avoid further regressions.

Comment 10

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48067991affc

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8692918 [details] [diff] [review]
patch

(In reply to James Kitchener (:jkitch) from comment #5)
> The problem lies within nsMathMlChar's stretching code (specifically
> StretchEnumContext::TryVariants).
> 
> The size of the first glyph tested is bigger than both the best size and the
> target size, sufficiently so that it fails the IsSizeBetter test.  The
> algorithm's response to that is to try an even bigger glyph which will
> naturally fail the same tests.  This process continues with increasingly
> bigger glyphs until the algorithm gives up - here the algorithm will return
> the last size tested, which it mistakenly thinks to be the closest match.

What return or sets the last size tested?

I expect the intention is to only set any glyph on the nsMathMLChar if it is
better than what has already been found.

(In reply to James Kitchener (:jkitch) from comment #9)
> The attached patch detects if the first character size tested exceeds the
> acceptable size range and that testing increasingly large sizes would only
> makes things worse.

That may be a sensible optimization, but I think the intention was to return
to the base glyph if no better was found.  I think that's what we want if the
stretchy font is only giving us large glyphs.

https://hg.mozilla.org/mozilla-central/annotate/7883e81f3c305078353ca27a6b1adb8c769d5904/layout/mathml/nsMathMLChar.cpp#l819

>+<div>
>+  <!-- The math should be completely obscured -->
>+  <math style="font-size:1em; z-index:1;" display="block">

What does the enclosing div provide?

>+<div style="position:fixed; top:0; left:0; z-index:10; background-color:green; 
>+            height:2em; width:100%; padding:2px;">

Please make the <math> position:fixed also, so that document padding is not
involved.

Comment 12

[James Kitchener (:jkitch)]

Right.  It seems the code then continues onto TryParts



https://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp#1280
ch[0] and ch[3] exist, leading to
bmdata[0], bmdata[3]:  ascent=2326 descent=1778
bmdata[1], bmdata[2]:  ascent=0, descent=0


Now in ComputeSizeFromParts:
https://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp#1328

aGlyphs[i] != aGlyphs[glue] only for i = 1 and 2.  This makes the sum nscood 0.

The sum then fails the minSize & maxSize inequalities, returning targetSize - in this case 972

We now reach "The computed size is the best we have found so far..."  Eventually
mBoundingMetrics.ascent is set to bmdata[0].ascent (2326)
mBoundingmetrics.descent is calculated using a combination of computedSize and mBoundingMetrics.ascent.

Finally we check whether the size is OK.  This is performed using the computed size and passes (naturally as it was set as equal to the target size) , so the dodgy bounding metrics are propagated back and used for stretching (but not for anything else).

Summary:
ComputeSizeFromParts fails to use metrics for characters with only glue glyphs when determining the computed size, falling back to the target size;  the glue metrics are then used to determine the bounding metrics used for stretching.

Detecting when this happens and returning early so "the next table can be tried" seems to fix it, but I need to test it further.  I also expect someone with better knowledge of this code would find a better solution.

Comment 13

[Karl Tomlinson (:karlt)]

Thanks.  Sounds like there may be an inconsistency between the drawing and measuring, if drawing of top and bottom glyphs does not clip to the desired size.

Perhaps this was not a problem before changes from bug 960115.

I'm not sure whether it is better to fix the drawing or measuring.
Perhaps fixing the measuring might be better if it causes fallback to the base size glyph with suitable thickness.

https://dxr.mozilla.org/mozilla-central/rev/faacb2337f8e/layout/mathml/nsMathMLChar.cpp#1280
https://dxr.mozilla.org/mozilla-central/rev/faacb2337f8e/layout/mathml/nsMathMLChar.cpp#1328

Comment 14

[James Kitchener (:jkitch)]

Attachment: patch

The attached patch changes ComputeSizeFromParts to use the glue size for all-glue glyphs (rather than 0).  This way, the computed size should better match the measured size and the algorithm is more capable of rejecting the glyph.

Also tried was changing the bounding metrics to match the computed size - the metrics are recalculated when painting, and it seems to be too late to correct for the size discrepancy if another character/font is needed.

The reftest mixes fixed and absolute positioning, as I was unable to get the test regressing in the before case when using the same for both testcases.  I expect that something worked out that one element was entirely obscured, and hence was never drawn.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Stop tracking as we already shipped several releases with it. However, happy to accept an uplift in aurora.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8702200 [details] [diff] [review]
patch

I'm happy with modifying ComputeSizeFromParts() to match the drawing, but I'm
not clear that this is matching the drawing.

>   for (int32_t i = first; i <= last; i++) {
>     if (aGlyphs[i] != aGlyphs[glue]) {
>       sum += aSizes[i];
>     }
>   }
> 
>+  // whether the character consists entirely of glue glyphs
>+  bool allGlue = sum == 0 && aSizes[glue] != 0;
>+

Wouldn't the same problem exist if only some of the top/middle/bottom parts
are glue?

I think ignoring parts matching glue in the sizing dates back to 2001
https://github.com/mozilla/gecko-dev/commit/24f525c998ef63fbb6d7984b6767a82ffe6357b8
but I don't know how to view that patch and the comment doesn't align.

I wonder whether the (aGlyphs[i] != aGlyphs[glue]) test could just be removed?

If that fails, then it might be simplest to add the glue height to sum if any
of top/middle/bottom were glue.

>-  nscoord minSize = NSToCoordRound(NS_MATHML_DELIMITER_FACTOR * sum);
>+  nscoord minSize = allGlue ? NSToCoordRound(NS_MATHML_DELIMITER_FACTOR * aSizes[glue]) :
>+                              NSToCoordRound(NS_MATHML_DELIMITER_FACTOR * sum);

I think this should be "sum - 2 * joins", to avoid overlapping by too much in
the drawing, but that can be a separate issue.

(In reply to James Kitchener (:jkitch) from comment #14)
> The reftest mixes fixed and absolute positioning, as I was unable to get the
> test regressing in the before case when using the same for both testcases. 
> I expect that something worked out that one element was entirely obscured,
> and hence was never drawn.

Ah, OK.  Perhaps the overflow metrics were wrong.

Comment 17

[James Kitchener (:jkitch)]

Attachment: patch

This simply removes the glue check.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8712097 [details] [diff] [review]
patch

(In reply to James Kitchener (:jkitch) from comment #14)
> The reftest mixes fixed and absolute positioning, as I was unable to get the
> test regressing in the before case when using the same for both testcases. 
> I expect that something worked out that one element was entirely obscured,
> and hence was never drawn.

Using different positioning may be working around the problem here through a deficiency in Gecko's occlusion culling.  The ideal solution might be to obscure only after a MozReftestInvalidate event, but that's an optional extra.
This patch is fine to land as is, thanks.
http://hg.mozilla.org/mozilla-central/annotate/c0ba5835ca48/layout/tools/reftest/README.txt#l433

Comment 19

[James Kitchener (:jkitch)]

Attachment: patch

That test change has been made, as the flaw the previous one relied on might be corrected at some stage.

Comment 20

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=209b8a5c52b2

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8329476b632

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8329476b632