Closed Bug 669932 Opened 9 years ago Closed 9 years ago

munderover: incorrect computation of logical vertical metrics

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: fredw, Assigned: fredw)

References

Details

(Whiteboard: [inbound])

Attachments

(7 files)

Attached file testcase
The testcase demonstrates several cases with munderover. The transparent green rects are the logical metrics of the script children, the transparent blue rects those of the base children and the red rect the one computed for the element.

The second and third lines show that the logical metrics of mover/munder are not the same as those of moverunder with a under/over script of size 0. ?Note that Jonathan's patches on bug 668204, are essentially re-implementing the former using the latter.
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Summary: Incorrect computation of logical vertical metrics → munderover: incorrect computation of logical vertical metrics
Attachment #544970 - Flags: review?(karlt) → review+
Attachment #544969 - Flags: review?(karlt) → review+
Comment on attachment 544971 [details] [diff] [review]
Add a reftest to verify the equivalence between munderover with empty scripts and the corresponding mover/munder/mrow constructs.

Thanks!
Attachment #544971 - Flags: review?(karlt) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/593d94e9492e
http://hg.mozilla.org/mozilla-central/rev/98df4af3a858
http://hg.mozilla.org/mozilla-central/rev/1aad22c44bd0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Please run your tests before you request landing (or review, for that matter).
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #6)
> Please run your tests before you request landing (or review, for that
> matter).
> Backed out.

I did, but I don't have this failure on my laptop.

@Karl: It seems that the munderover bounding box is sometimes 1px higher than the corresponding msup/msub/mrow contruct. Do you have any idea about how this difference may come from?
nsMathMLmunderoverFrame::Place is using under/overDelta2 in calculating logical bounds from ink bounds, even when there is no under/overFrame.
Usually we push changes to https://wiki.mozilla.org/Try before to the general repositories.
(In reply to comment #9)
> nsMathMLmunderoverFrame::Place is using under/overDelta2 in calculating
> logical bounds from ink bounds, even when there is no under/overFrame.

under/over-Delta1 are set to 0 when the vertical ink metrics is zero. Should we do the same for under/over-Delta2?
Also, it seems that we should also set these Deltas to 0 when the horizontal ink metrics is zero?
> Same patch as attachment 546513 [details] [diff] [review] [review], to apply after bug
> 557476, bug 668204 and bug 669713.

Tryserver Result for the set of munderover patches:

http://tbpl.mozilla.org/?tree=Try&rev=e1f872ce1440
It would be nice to have a tryserver result without bug 668204, as I assume bug 668204 makes the reftests pass irrespective of what happens here.
Attachment #546513 - Flags: review?(karlt)
Comment on attachment 546513 [details] [diff] [review]
munderover: do not add space above/below the over/under script when this one has its dimensions set to zero

(In reply to comment #11)
> under/over-Delta1 are set to 0 when the vertical ink metrics is zero. Should
> we do the same for under/over-Delta2?

This still leaves the possibility of increasing the logical ascent, even when there is no overscript, if the ink ascent is greater than the logical ascent, but that's an outlier case that is probably not important.

> Also, it seems that we should also set these Deltas to 0 when the horizontal
> ink metrics is zero?

Hopefully the ink width will be zero when the height is zero.  I'm not sure, but let's look into that if/when a problem shows up.
Attachment #546513 - Flags: review?(karlt) → review+
Do you prefer to take the patches for this bug now or wait that the fixes for other munderover bugs are ready and push all in one go (comment 13)?
This can land now.  It's probably better that they land separately, but if it's also OK if they end up landing together.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.