Last Comment Bug 669932 - munderover: incorrect computation of logical vertical metrics
: munderover: incorrect computation of logical vertical metrics
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
Depends on:
Blocks: 668204
  Show dependency treegraph
 
Reported: 2011-07-07 10:07 PDT by Frédéric Wang (:fredw)
Modified: 2011-08-08 05:51 PDT (History)
2 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.80 KB, text/html)
2011-07-07 10:07 PDT, Frédéric Wang (:fredw)
no flags Details
munderover: consider munder vertical metrics during the munder attachment phase instead of the mover attachment phase. (2.21 KB, patch)
2011-07-09 02:33 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
munderover: take into account the base child vertical metrics for the computation of the munderover ascent/descent. (2.04 KB, patch)
2011-07-09 02:34 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Add a reftest to verify the equivalence between munderover with empty scripts and the corresponding mover/munder/mrow constructs. (3.23 KB, patch)
2011-07-09 02:35 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
reftest failures (74.03 KB, text/plain)
2011-07-13 20:54 PDT, Karl Tomlinson (:karlt)
no flags Details
munderover: do not add space above/below the over/under script when this one has its dimensions set to zero (1.91 KB, patch)
2011-07-18 06:00 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
munderover: do not add space above/below the over/under script when this one has its dimensions set to zero (1.91 KB, patch)
2011-07-18 22:12 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2011-07-07 10:07:44 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2011-07-09 02:33:10 PDT
Created attachment 544969 [details] [diff] [review]
munderover: consider munder vertical metrics during the munder attachment phase instead of the mover attachment phase.
Comment 2 Frédéric Wang (:fredw) 2011-07-09 02:34:04 PDT
Created attachment 544970 [details] [diff] [review]
munderover: take into account the base child vertical metrics for the computation of the munderover ascent/descent.
Comment 3 Frédéric Wang (:fredw) 2011-07-09 02:35:53 PDT
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.
Comment 4 Karl Tomlinson (:karlt) 2011-07-12 22:50:55 PDT
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 6 Dão Gottwald [:dao] 2011-07-13 09:09:09 PDT
Please run your tests before you request landing (or review, for that matter).
Backed out.
Comment 7 Frédéric Wang (:fredw) 2011-07-13 09:42:44 PDT
(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) 2011-07-13 20:54:12 PDT
Created attachment 545819 [details]
reftest failures
Comment 9 Karl Tomlinson (:karlt) 2011-07-13 21:12:44 PDT
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) 2011-07-13 21:14:32 PDT
Usually we push changes to https://wiki.mozilla.org/Try before to the general repositories.
Comment 11 Frédéric Wang (:fredw) 2011-07-16 05:27:08 PDT
(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) 2011-07-18 06:00:48 PDT
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
Comment 13 Frédéric Wang (:fredw) 2011-07-18 22:12:40 PDT
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.
Comment 14 Frédéric Wang (:fredw) 2011-07-19 22:41:53 PDT
> 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) 2011-07-28 23:50:11 PDT
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) 2011-07-30 00:55:22 PDT
http://tbpl.mozilla.org/?tree=Try&rev=9f3aacc80358
Comment 17 Karl Tomlinson (:karlt) 2011-08-02 22:36:17 PDT
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) 2011-08-03 12:47:28 PDT
http://tbpl.mozilla.org/?tree=Try&rev=9275783afb7d
Comment 19 Frédéric Wang (:fredw) 2011-08-03 12:50:26 PDT
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) 2011-08-03 15:03:11 PDT
This can land now.  It's probably better that they land separately, but if it's also OK if they end up landing together.

Note You need to log in before you can comment on or make changes to this bug.