669932 - munderover: incorrect computation of logical vertical metrics

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

Attachment: 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.

Comment 1

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

Attachment: munderover: consider munder vertical metrics during the munder attachment phase instead of the mover attachment phase.

Comment 2

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

Attachment: munderover: take into account the base child vertical metrics for the computation of the munderover ascent/descent.

Comment 3

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

Attachment: Add a reftest to verify the equivalence between munderover with empty scripts and the corresponding mover/munder/mrow constructs.

Comment 4

[Karl Tomlinson (:karlt)]

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!

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/593d94e9492e
http://hg.mozilla.org/mozilla-central/rev/98df4af3a858
http://hg.mozilla.org/mozilla-central/rev/1aad22c44bd0

Comment 6

[Dão Gottwald [:dao]]

Please run your tests before you request landing (or review, for that matter).
Backed out.

Comment 7

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

(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?

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: reftest failures

Comment 9

[Karl Tomlinson (:karlt)]

nsMathMLmunderoverFrame::Place is using under/overDelta2 in calculating logical bounds from ink bounds, even when there is no under/overFrame.

Comment 10

[Karl Tomlinson (:karlt)]

Usually we push changes to https://wiki.mozilla.org/Try before to the general repositories.

Comment 11

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

(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?

Comment 12

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

Attachment: munderover: do not add space above/below the over/under script when this one has its dimensions set to zero

Comment 13

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

Attachment: 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.

Comment 14

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

> 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

Comment 15

[Karl Tomlinson (:karlt)]

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.

Comment 16

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

http://tbpl.mozilla.org/?tree=Try&rev=9f3aacc80358

Comment 17

[Karl Tomlinson (: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.

Comment 18

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

http://tbpl.mozilla.org/?tree=Try&rev=9275783afb7d

Comment 19

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

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)?

Comment 20

[Karl Tomlinson (:karlt)]

This can land now.  It's probably better that they land separately, but if it's also OK if they end up landing together.

Comment 21

[Karl Tomlinson (:karlt)]

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

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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