The default bug view has changed. See this FAQ.

munderover: incorrect computation of logical vertical metrics

RESOLVED FIXED in mozilla8

Status

()

Core
MathML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

Trunk
mozilla8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(7 attachments)

(Assignee)

Description

6 years ago
Created attachment 544525 [details]
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)

Updated

6 years ago
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 544969 [details] [diff] [review]
munderover: consider munder vertical metrics during the munder attachment phase instead of the mover attachment phase.
Attachment #544969 - Flags: review?(karlt)
(Assignee)

Comment 2

6 years ago
Created attachment 544970 [details] [diff] [review]
munderover: take into account the base child vertical metrics for the computation of the munderover ascent/descent.
Attachment #544970 - Flags: review?(karlt)
(Assignee)

Comment 3

6 years ago
Created attachment 544971 [details] [diff] [review]
Add a reftest to verify the equivalence between munderover with empty scripts and the corresponding mover/munder/mrow constructs.
Attachment #544971 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 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 → ---
(Assignee)

Comment 7

6 years ago
(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?
Created attachment 545819 [details]
reftest failures
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.
(Assignee)

Comment 11

6 years ago
(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?
(Assignee)

Comment 12

6 years ago
Created 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
(Assignee)

Comment 13

6 years ago
Created attachment 546709 [details] [diff] [review]
munderover: do not add space above/below the over/under script when this one has its dimensions set to zero

Same patch as attachment 546513 [details] [diff] [review], to apply after bug 557476, bug 668204 and bug 669713.
(Assignee)

Comment 14

6 years ago
> 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.
(Assignee)

Comment 16

6 years ago
http://tbpl.mozilla.org/?tree=Try&rev=9f3aacc80358
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 18

6 years ago
http://tbpl.mozilla.org/?tree=Try&rev=9275783afb7d
(Assignee)

Comment 19

6 years ago
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
http://hg.mozilla.org/integration/mozilla-inbound/rev/a637f174ceab
http://hg.mozilla.org/integration/mozilla-inbound/rev/93bb936441c7
http://hg.mozilla.org/integration/mozilla-inbound/rev/5ea0f8810805
http://hg.mozilla.org/integration/mozilla-inbound/rev/8d5e0f4187bd
Whiteboard: [inbound]
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a637f174ceab
http://hg.mozilla.org/mozilla-central/rev/93bb936441c7
http://hg.mozilla.org/mozilla-central/rev/5ea0f8810805
http://hg.mozilla.org/mozilla-central/rev/8d5e0f4187bd
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.