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

RESOLVED FIXED in mozilla31

Status

()

Core
MathML
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: distler, Assigned: jkitch)

Tracking

unspecified
mozilla31
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 402593 [details]
Testcase
(Reporter)

Comment 2

8 years ago
Created attachment 402594 [details]
Incorrect rendering of the testcase
> 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/
(Reporter)

Comment 4

8 years ago
>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.
(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
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.
Depends on: 114365
Created attachment 8341313 [details]
Screenshot of the testcase with the mathvariant patches

screenshot
This is now fixed.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Assignee: nobody → jkitch.bug
Target Milestone: --- → mozilla28
Status: RESOLVED → VERIFIED
status-firefox28: --- → verified
Flags: in-testsuite?
(Assignee)

Comment 9

3 years ago
Created attachment 8385279 [details] [diff] [review]
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.
Attachment #8385279 - Flags: review?(fred.wang)
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.
Attachment #8385279 - Flags: review?(fred.wang) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8388135 [details] [diff] [review]
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.
Attachment #8385279 - Attachment is obsolete: true
Attachment #8388135 - Flags: review?(fred.wang)
Attachment #8388135 - Flags: review?(fred.wang) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 12

3 years ago
Created attachment 8389601 [details] [diff] [review]
patch.diff

Typo in commit message fixed.
Attachment #8388135 - Attachment is obsolete: true
Impressive that this was VERIFIED FIXED when it actually wasn't.
Status: VERIFIED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: mozilla28 → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4344d87e75
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
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
status-firefox28: verified → ---
(Assignee)

Comment 16

3 years ago
Created attachment 8390454 [details] [diff] [review]
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.
Attachment #8389601 - Attachment is obsolete: true
Attachment #8390454 - Flags: review?(fred.wang)
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.
Attachment #8390454 - Flags: sec-approval?
Attachment #8390454 - Flags: review?(karlt)
Attachment #8390454 - Flags: review?(fred.wang)
Attachment #8390454 - Flags: feedback+
Why do you need sec-approval+ to land a non-security bug on trunk?
Comment on attachment 8390454 [details] [diff] [review]
patch

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

Sorry, I didn't set the flag on purpose.
Attachment #8390454 - Flags: sec-approval?
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
Attachment #8390454 - Flags: review?(karlt) → review+
(Assignee)

Comment 21

3 years ago
Created attachment 8391844 [details] [diff] [review]
patch

The refactoring suggested in comment 20 has been applied.



https://tbpl.mozilla.org/?tree=Try&rev=363a4e8aa0f7
Green try run.
Attachment #8390454 - Attachment is obsolete: true
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)".
(Assignee)

Comment 23

3 years ago
Created attachment 8392832 [details] [diff] [review]
patch
Attachment #8391844 - Attachment is obsolete: true
Attachment #8392832 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1028c462d120
Keywords: checkin-needed
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 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.)
(Assignee)

Comment 27

3 years ago
Created attachment 8394627 [details] [diff] [review]
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).
(Assignee)

Updated

3 years ago
Attachment #8392832 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Third time lucky I hope.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/135ccd1d6fb1
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/135ccd1d6fb1
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.