Closed
Bug 518592
Opened 16 years ago
Closed 11 years ago
Dotless i's and j's in <mi>
Categories
(Core :: MathML, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: distler, Assigned: jkitch)
References
Details
Attachments
(4 files, 6 obsolete files)
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, ı 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 ȷ named entity will expand to this character.)
See the attached testcase.
Reproducible: Always
Comment 3•16 years ago
|
||
> 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, ı always renders in italics, and the equivalent NCR and utf-8
> character renders upright.
So the NCR and utf-8 characters are rendering correctly.
ı renders differently (as would ȷ) 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/
>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 ı 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 ı 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 ı 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•16 years ago
|
||
(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•12 years ago
|
||
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: mathvariant
Comment 7•11 years ago
|
||
screenshot
Comment 8•11 years ago
|
||
This is now fixed.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Assignee: nobody → jkitch.bug
Target Milestone: --- → mozilla28
Assignee | ||
Comment 9•11 years ago
|
||
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 10•11 years ago
|
||
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•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8388135 -
Flags: review?(fred.wang) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•11 years ago
|
||
Typo in commit message fixed.
Attachment #8388135 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
Impressive that this was VERIFIED FIXED when it actually wasn't.
Status: VERIFIED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: mozilla28 → ---
Comment 14•11 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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 17•11 years ago
|
||
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+
Comment 18•11 years ago
|
||
Why do you need sec-approval+ to land a non-security bug on trunk?
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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•11 years ago
|
||
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 22•11 years ago
|
||
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•11 years ago
|
||
Attachment #8391844 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8392832 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 24•11 years ago
|
||
Keywords: checkin-needed
Comment 25•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Attachment #8392832 -
Attachment is obsolete: true
Comment 29•11 years ago
|
||
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•