518592 - Dotless i's and j's in <mi>

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[distler]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4pre) Gecko/20090902 SeaMonkey/2.0b2pre
Build Identifier: 

Single character <mi>s should be rendered in italics. Multi-character <mi>'s should be rendered in an upright font.

Instead, &imath; always renders in italics, and the equivalent NCR and utf-8 character renders upright.

A similar problem arises for the dotless j (U+0237). (In MathML 3, the &jmath; named entity will expand to this character.)

See the attached testcase.

Reproducible: Always

Comment 1

[distler]

Attachment: Testcase

Comment 2

[distler]

Attachment: Incorrect rendering of the testcase

Comment 3

[Karl Tomlinson (:karlt)]

> Single character <mi>s should be rendered in italics. Multi-character <mi>'s
> should be rendered in an upright font.

Only some characters should be rendered in italics when alone in an <mi>
as you pointed out (but don't like) here:
http://lists.w3.org/Archives/Public/www-math/2009Sep/0024.html

I also pointed this out here:
http://lists.w3.org/Archives/Public/www-math/2008Jan/0015.html

> Instead, &imath; always renders in italics, and the equivalent NCR and utf-8
> character renders upright.

So the NCR and utf-8 characters are rendering correctly.

&imath; renders differently (as would &jmath;) and probably incorrectly, though perhaps more as expected.  This is because, at the time I was updating Mozilla's entity tables, both Mozilla's and MathML's entity tables had a number of imperfections and so were being updated.  I set up these entities based on the characters that http://www.unicode.org/reports/tr25/tr25-9.html recommended for ISOAMSO entities imath and jmath and the [TeX] macros \imath and \jmath.

http://lists.w3.org/Archives/Public/www-math/2008Jan/0013.html
http://lists.w3.org/Archives/Public/www-math/2008Apr/0218.html

Apparently the MathML WG decided not to follow the Unicode documents and decided to use the historic character for imath (and perhaps the choice for jmath was for consistency reasons).

If the imath and jmath entities are not going to change, then Mozilla should align its definitions with http://www.w3.org/TR/xml-entity-names/

Comment 4

[distler]

>Only some characters should be rendered in italics when alone in an <mi>
>as you pointed out (but don't like) here...

Actually, that comment applied to the Spec prose about @mathvariant.

That's a different issue from the one here (though my example was related -- I have &imath; on the brain...).

>So the NCR and utf-8 characters are rendering correctly.

Umh. Not quite.

In the testcase, the NCR and utf-8 character always render in their upright variants (even when they should render in their italic variant).

Conversely, the &imath; entity always renders as italic, even when it should render as upright.

>Apparently the MathML WG decided not to follow the Unicode documents and
>decided to use the historic character for imath (and perhaps the choice for
>jmath was for consistency reasons).

My understanding of their rationale is that the "base character" is the upright glyph. The renderer is somehow responsible for substituting the italic (or bold or whatever) variant, when appropriate.

Thus &imath; should point to U+0131. The italic variant can come either from U+1D6A4, or from the STIXGeneral-italic variant of U+0131 (these seem to be exactly the same glyph, anyway).

>If the imath and jmath entities are not going to change, then Mozilla should
align its definitions with http://www.w3.org/TR/xml-entity-names/

There are quite a number of changes in that document from MathML2 (mostly reflecting changes in Unicode during the intervening years).

It would be good to align Mozilla's definitions with it.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to comment #4)
> In the testcase, the NCR and utf-8 character always render in their upright
> variants (even when they should render in their italic variant).

In MathML2, mathvariant (explicit or implicit) should not cause them to render as italic.  I've asked for clarification in MathML3:
http://lists.w3.org/Archives/Public/www-math/2009Sep/0034.html

Comment 6

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

So I'm not sure what is the decision on this, but I tend to agree with Jacques that dotless i (0x0131) and dotless j ( 0x0237) that have mathvariant=italic (explicit or implicit) should map to their italic forms 0x1D6A4 and 0x1D6A5. That's what James' patch seems to do, so I'm adding a dependency.

Comment 7

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

Attachment: Screenshot of the testcase with the mathvariant patches

screenshot

Comment 8

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

This is now fixed.

Comment 9

[James Kitchener (:jkitch)]

Attachment: test.diff

I do not believe this exact scenario was in the test suite, although several tests which come close are present.  

The attached test assumes that <mi> and <mn> render identically (other than the single char special case of course).  A nicer test that doesn't rely on this assumption is blocked by bug 930504.

Comment 10

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

Comment on attachment 8385279 [details] [diff] [review]
test.diff

Review of attachment 8385279 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good thanks. But note that <mi> and <mn> might lead to different spacing of the formula.

Comment 11

[James Kitchener (:jkitch)]

Attachment: patch.diff

It turns out the spacing is meant to be different, but the tests were passing due to a bug.

The NS_FRAME_IS_IN_SINGLE_CHAR_MI case in nsMathMLTokenFrame::GetMathMLFrameType() was never triggered because it was checking the wrong frame for the flag.

The patch sets the flag on the token frame as well as the text frame.  Alternatively I could inspect the grandchild frame in GetMathMLFrameType(), but this seemed the simpler option.

Comment 12

[James Kitchener (:jkitch)]

Attachment: patch.diff

Typo in commit message fixed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Impressive that this was VERIFIED FIXED when it actually wasn't.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4344d87e75

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest failures. Looks like the bottom box is misshapen.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce32d1eb6d1

https://tbpl.mozilla.org/php/getParsedLog.php?id=35999215&tree=Mozilla-Inbound

Comment 16

[James Kitchener (:jkitch)]

Attachment: patch

The test failure revealed another existing bug, and can be replicated in an unpatched build by an explicit mathvariant="italic" in the final <mi> of table-width-4.html

As far as I can tell, the test failure is caused by there being multiple ways of calculating the width of the expression, with one method including interframe spacing and the other excluding it (at least the discrepancy happened to match the value returned by GetInterFrameSpacingFor()).  If the expression is wrapped in an <mrow> both methods include the additional spacing.

I'm not sure if GetMinWidth/GetPrefWidth is the most appropriate location to inject the additional width, but partial consideration of the effects of FixInterframeSpacing is already included in those methods.

GetInterFrameSpacingFor was made a private method as the compiler complained otherwise.  

An additional side effect of these changes is that I needed to increment the right padding of the table cell by 1 pixel to avoid some very faint sub-pixel antialiasing.

Comment 17

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

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

Review of attachment 8390454 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me, but I prefer to get Karl's confirmation about this FixInterFrameSpacing stuff.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Why do you need sec-approval+ to land a non-security bug on trunk?

Comment 19

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

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

Review of attachment 8390454 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I didn't set the flag on purpose.

Comment 20

[Karl Tomlinson (:karlt)]

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

This looks correct, but please add a static forward declaration instead of
making GetInterFrameSpacingFor() a private class method.  See, for example
http://hg.mozilla.org/mozilla-central/annotate/f073b3d6db1f/widget/gtk/nsWindow.cpp#l149

Or if you are willing to refactor things a bit, I think the same code can be
shared between the different metric calculations as follows:

* Change finalize reflow to move FixInterFrameSpacing to after
  "aDesiredSize.mBoundingMetrics = mBoundingMetrics;".

* Change FixInterFrameSpacing() to operate on aDesiredSize.mBoundingMetrics
  instead of mBoundingMetrics.

* Move all of FixInterFrameSpacing except the childFrame loop into a new
  method.

* Use the new method from FixInterFrameSpacing, GetMinWidth, and GetPrefWidth

Comment 21

[James Kitchener (:jkitch)]

Attachment: patch

The refactoring suggested in comment 20 has been applied.



https://tbpl.mozilla.org/?tree=Try&rev=363a4e8aa0f7
Green try run.

Comment 22

[Karl Tomlinson (:karlt)]

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

Thank you!

>   nsBoundingMetrics bm = desiredSize.mBoundingMetrics;
>-  // We include the overflow to compensate for FixInterFrameSpacing.
>-  result = std::max(bm.width, bm.rightBearing) - std::min(0, bm.leftBearing);
>+  result = std::max(bm.width, bm.rightBearing);
>+
>+  // Include the additional width added by FixInterFrameSpacing to ensure
>+  // consistent width calculations.
>+  result += AddInterFrameSpacingToSize(desiredSize, this);
>+
>   return result;

If this is all working as intended, then the return value of AddInterFrameSpacingToSize and the bm copy are no longer needed here, as AddInterFrameSpacingToSize will modify desiredSize appropriately.
The function can return desiredSize.Width() (which should be the same as desiredsize.mBoundingMetrics.width).

>+    // see if we should shift our children to account for the correction

Please remove "see if we should" here, as this is now after "if (gap)".

Comment 23

[James Kitchener (:jkitch)]

Attachment: patch

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1028c462d120

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e1703e8e9f

https://tbpl.mozilla.org/php/getParsedLog.php?id=36393003&tree=Mozilla-Inbound

Given the DISPLAY_MIN_WIDTH line, it wasn't clear to me what the right solution really is for that warning/error.

Comment 26

[Daniel Holbert [:dholbert]]

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

> /* virtual */ nscoord
> nsMathMLContainerFrame::GetPrefWidth(nsRenderingContext *aRenderingContext)
> {
>   nscoord result;
>   DISPLAY_MIN_WIDTH(this, result);
>   nsHTMLReflowMetrics desiredSize(GetWritingMode());
>   GetIntrinsicWidthMetrics(aRenderingContext, desiredSize);
>-  nsBoundingMetrics bm = desiredSize.mBoundingMetrics;
>-  // We include the overflow to compensate for FixInterFrameSpacing.
>-  result = std::max(bm.width, bm.rightBearing) - std::min(0, bm.leftBearing);
>-  return result;
>+
>+  // Include the additional width added by FixInterFrameSpacing to ensure
>+  // consistent width calculations.
>+  AddInterFrameSpacingToSize(desiredSize, this);
>+
>+  return desiredSize.Width();


So DISPLAY_MIN_WIDTH() is just a debugging macro which (given the right configuration) instantiates an object that caches a reference to the thing you pass in ('result'), and then prints out its final value when the function returns.

For this to continue working correctly, you'll can't directly return desiredSize.Width(). Instead, you should be setting 'result = desiredSize.Width()' and then return result.

This applies to both GetPrefWidth and GetMinWidth.

(Also, GetPrefWidth should really have DISPLAY_PREF_WIDTH instead of DISPLAY_MIN_WIDTH.  It might be worth making that fix while you're here; it's a minor change that just affects the string that's logged.)

Comment 27

[James Kitchener (:jkitch)]

Attachment: patch

Fixes from comment 26 have been applied.

Try run.
https://tbpl.mozilla.org/?tree=Try&rev=a7b7bf5f931f

None of the failures appear to be MathML related (the busted B2G builds are attributable to Bug 985864).

Comment 28

[James Kitchener (:jkitch)]

Third time lucky I hope.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/135ccd1d6fb1

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/135ccd1d6fb1