In nsMathMLmpaddedFrame::Place, the height and depth of the mpadded element corresponds to ink metrics mBoundingMetrics.ascent and mBoundingMetrics.descent. It may be preferable to consider the logical metrics aDesiredSize.ascent and aDesiredSize.height - aDesiredSize.ascent instead.

Comment on attachment 623514 [details] [diff] [review] Proposed Patch for Bug 685628 - mpadded: height/depth should apply to logical metrics Review of attachment 623514 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking at this, Anjor. mpadded is described here: http://www.w3.org/TR/MathML/chapter3.html#presm.mpadded And our two kinds of metrics "logical" and "ink" discussed here: https://bugzilla.mozilla.org/show_bug.cgi?id=557474#c18 nsMathMLmpaddedFrame::Place has essentially three steps 1) Initialize nscoord height, depth with the inner frame metrics. 2) Calling UpdateValue to update these values. For example if <mpadded> has an attribute height="200%", then height should become twice larger. 3) Update the outer frame metrics with the new values. Currently your patch modifies 1) to use logical metrics instead of ink metrics. You have to do the same work for steps 2) and 3). I also suspect that you'll have to add an argument "const nsHTMLReflowMetrics& aDesiredSize" to the UpdateValue function in order to pass the logical metrics and use them to set the "scaler" variable.

Assigning to Hu Xuan.

A trial for bug fix as well as some redundant spaces removed.

(In reply to Xuan Hu from comment #4) > Created attachment 644348 [details] [diff] [review] > First trial for bug #685628 > > A trial for bug fix as well as some redundant spaces removed. Thank you, I'll look into this when I have time. FYI, Mozilla does not generally accept whitespace changes and clearly not when they are mixed with other changes of the code (that makes diff less friendly to review or to read in the mercurial history and these kinds of changes are more likely to conflict with others). Some of the lines in the MathML directory does not always respect Mozilla's coding style, but we leave them as they are unless we really modify the code.

Patch Updated removing changes related to whitespace.

Comment on attachment 644361 [details] [diff] [review] Trial Patch Review of attachment 644361 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, that seems good. Can you please follow the instructions here: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment I don't think your change will affect the horizontal metrics because mBoundingMetrics.width == aDesiredSize.width AFAIK. So you only need to write reftests for the vertical metrics. The idea is that some characters like _ have different ink (mBoundingMetrics) and logical metrics (aDesiredSize). Here the ink metrics is only the space occupied by the bar while the logical metrics is the whole rectangle occupied by the character. I think you can try to consider the rectangle <math> <mpadded mathbackground="red" height="100px" depth="100px" width=@1> <mphantom> <mtext mathvariant="monospace">@2</mtext> </mphantom> </mpadded> </math> 1) with various values "10height" or "10depth" for @1 and various characters (like _, X etc) for @2. I guess the width should not depend on the character @2. 2) for a fixed width @1 (say "100px") and various @2. I guess the actual height+depth of the rectangle should not depend on the character @2. Currently, the results of 1) and 2) depend on @2. ::: layout/mathml/nsMathMLmpaddedFrame.cpp @@ +428,5 @@ > width - initialWidth - lspace : lspace; > > aDesiredSize.ascent += dy; > aDesiredSize.width = mBoundingMetrics.width; > + aDesiredSize.height += dy + depth - aDesiredSize.height + aDesiredSize.ascent; This is not correct because aDesiredSize.ascent has already been updated two lines above. Anyway, what we did here was forcing mBoundingMetrics.ascent = height mBoundingMetrics.descent = depth and updating aDesiredSize to keep the deltas between the old aDesiredSize/mBoundingMetrics vertical metrics. What we want now is forcing aDesiredSize to have the specified vertical metrics height/depth. Not sure what we should do for mBoundingMetrics... either set them to the same size or leave them alone. Karl?

Bug fix according to fredw's suggestion. Still working on reftest. If there's any further problems, feel free to contact me.

Comment on attachment 644874 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw,karlt Review of attachment 644874 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLmpaddedFrame.cpp @@ +428,5 @@ > width - initialWidth - lspace : lspace; > > aDesiredSize.ascent += dy; > aDesiredSize.width = mBoundingMetrics.width; > + aDesiredSize.height += depth - aDesiredSize.height + aDesiredSize.ascent; This is equivalent to the simpler syntax: aDesiredSize.height = depth + aDesiredSize.ascent;

Sorry for the silly error.

Comment on attachment 644900 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt (In reply to Frédéric Wang (:fredw) from comment #7) > What we want now is forcing aDesiredSize to have the specified vertical > metrics height/depth. Not sure what we should do for mBoundingMetrics... > either set them to the same size or leave them alone. Karl? If the height and/or depth are absolute, then it probably makes sense to set mBoundingMetrics to the same as aDesiredSize. If the change is relative, then it would be best to make the same relative change to mBoundingMetrics and aDesiredSize. It may be somewhat more complicated to handle both, so I think that can be considered a future improvement, and the patch is OK as is if Frédéric is fine with that.

Comment on attachment 644900 [details] [diff] [review] Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt Review of attachment 644900 [details] [diff] [review]: ----------------------------------------------------------------- OK that sounds good. BTW, see also comment 557474 comment 18 for more information about ink/logical metrics. That could help you for reftests. ::: layout/mathml/nsMathMLmpaddedFrame.cpp @@ +422,5 @@ > mBoundingMetrics.width = width; > mBoundingMetrics.rightBearing = mBoundingMetrics.width; > } > > + nscoord dy = height - aDesiredSize.ascent; I think this variable is no longer needed. Just setting aDesiredSize.ascent = height below will work. r=me with this change.

Comment on attachment 645256 [details] [diff] [review] 685628-5.patch Thanks. If you need to discuss a bit more reftests, don't hesitate.

(In reply to Frédéric Wang (:fredw) from comment #7) > > I don't think your change will affect the horizontal metrics because > mBoundingMetrics.width == aDesiredSize.width AFAIK. So you only need to > write reftests for the vertical metrics. The idea is that some characters > like _ have different ink (mBoundingMetrics) and logical metrics > (aDesiredSize). Here the ink metrics is only the space occupied by the bar > while the logical metrics is the whole rectangle occupied by the character. > I think you can try to consider the rectangle > > <math> > <mpadded mathbackground="red" height="100px" depth="100px" width=@1> > <mphantom> > <mtext mathvariant="monospace">@2</mtext> > </mphantom> > </mpadded> > </math> > > 1) with various values "10height" or "10depth" for @1 and various characters > (like _, X etc) for @2. I guess the width should not depend on the character > @2. > 2) for a fixed width @1 (say "100px") and various @2. I guess the actual > height+depth of the rectangle should not depend on the character @2. > > Currently, the results of 1) and 2) depend on @2. > hi, Fredw, As you suggested in comment 7, I want to have two reftests about mpadded. The first is about the standalone width and the second is about the height. But I feel it hard to judge the equality between the specific dimension. For example, the two mpadded elements have same width but different height. After looking the doc of reftests, I found "script" as <type> may achieve this but I don't ensure whether is proper to do so. Is there any alternative methods? Wish your further suggestion.

Thx for karl's explanation. I found the reason of my mistake. I will be back after some progress.

Xuan, are you still working on this bug?

Hi Fredw： Sorry, I have completely forget this. I want to finish it if there is no others instead. It's that ok?

(In reply to Xuan Hu from comment #18) > I want to finish it if there is no others instead. > It's that ok? Sure. I just wanted to be sure, but glad to see you're still interested.

Xuan, could you confirm you're still working on this?

Sorry for the late coming patch. Just busy with school matters as well as appling for GSoC 2013 If there is any problem. feel free to tell me. :-)

Comment on attachment 742339 [details] [diff] [review] sixth try for bug fix 685628 Review of attachment 742339 [details] [diff] [review]: ----------------------------------------------------------------- I'll let Fred handle this, since I know nothing about this code.

Some minor error fixed compared to last uploaded patch. Should work now both with compile and reftests. If there is anything wrong, feel free to tell me. :-)

Now I'm wondering if that's a good idea to make width depends on height. That's a bit in conflict with the CSS model and I'm not sure mpadded works very well at the moment. So could you consider something like height="10height" instead? I think for "X" and "_" depth = 0, so you only want to consider the height. E.g <math> <mpadded mathbackground="red" height="10height" depth="0"> <mphantom> <mtext mathvariant="monospace">_</mtext> </mphantom> </mpadded> </math> Can you check if the test fails before and passes after.

Comment on attachment 742969 [details] [diff] [review] bug-685628-fix-7 See comment 24.

So my point was that you can keep mpadded-9.html but replace -8 and -7 by tests where depth or height are expressed in term of width, height or depth ; or where width is expressed in term of width (not in term of height or depth). For example the test I gave in comment 24 (height in term of height) but you may try to find other tests.

OK, I will deal with it when I have time. Currently I am busy with another GSoC project. Sorry for the delay. :-(

Hi, fredw, From the above comments, I think there should be three reftests. - height in term of height should not depend on the characters - height in term of width should not depend on the characters - width in term of width should not depend on the characters By following these three items I created the reftests as in the attachments. But I feel a little confused about the usage of reftests, cause with the reftest.list ``` == mpadded-7.html mpadded-7-ref.html == mpadded-8.html mpadded-8-ref.html == mpadded-9.html mpadded-9-ref.html ``` all test failed with following log information: ``` 0:07.77 reftest failed: 0:07.77 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled 0:07.77 reference to undefined property options.bytesREFTEST TEST-UNEXPECTED-FAIL | file:///Users/huxuan/Code/mozilla-central/layout/reftests/mathml/mpadded-7.html | image comparison (==), max difference: 255, number of differing pixels: 9190 0:07.77 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/huxuan/Code/mozilla-central/layout/reftests/mathml/mpadded-8.html | image comparison (==), max difference: 255, number of differing pixels: 7008 0:07.77 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/huxuan/Code/mozilla-central/layout/reftests/mathml/mpadded-9.html | image comparison (==), max difference: 255, number of differing pixels: 2541 ``` I am not sure where is wrong, wish you can give me some help.

I guess for 8 and 9, you want something to compare monospace characters like "|" and "_" that clearly have different ink widths but (I think) equal logical widths. Your tests are supposed to fail before your C++ patch is applied and to pass after it is applied (so be sure to have your C++ patch applied and to have built Firefox again before running the reftests). You can compare the results "by eye": open two mpadded-*.html and mpadded-*-ref.html into two Firefox tabs and switch between these two tabs. You should not see any difference. You can also open source/layout/tools/reftest/reftest-analyzer.xhtml and copy and paste the test results into it. This can help you to see the differences. It's possible that the width/height of the characters used in your tests are not exactly equal and the difference will become even larger when multiplied by 100. In that case the reftests won't work :-(

Hi, fredw, Sorry for the silly error, I just forget to rebuild firefox before reftests. :-) I have successfully make the reftests work as the diff in the attachment. But I still have two questions, The first one is that reftests still report some error with following log: ``` 0:07.32 REFTEST INFO | runreftest.py | Running tests: end. 0:07.34 reftest failed: 0:07.34 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled 0:07.34 make: *** [reftest] Error 1 ``` The second one is that in reftest -8 & -9, character `_` is wider than `X` and `|`, which means when I change one of the character to `_`, corresponding reftest will fail. It's so strange, isn't it?

(In reply to Xuan Hu from comment #30) > 0:07.32 REFTEST INFO | runreftest.py | Running tests: end. > 0:07.34 reftest failed: > 0:07.34 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled > 0:07.34 make: *** [reftest] Error 1 Are you sure this error comes from the mpadded-7? This seems unrelated to your test, something like a compilation option disabled on your set up... > ``` > > The second one is that in reftest -8 & -9, character `_` is wider than `X` > and `|`, which means when I change one of the character to `_`, > corresponding reftest will fail. It's so strange, isn't it? Yes, that's weird. I was not quite sure that it would be the case but I expected that monospace fonts use the same logical widths for all characters. I think you can keep '|' and 'X' if that works. It would be interesting to send your patches to the TryServer to see if the tests pass on all platforms: https://wiki.mozilla.org/Build:TryServer

(In reply to Xuan Hu from comment #30) > 0:07.32 REFTEST INFO | runreftest.py | Running tests: end. > 0:07.34 reftest failed: > 0:07.34 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled > 0:07.34 make: *** [reftest] Error 1 I don't think you need to worry about that. It might be something like your screen not having enough lines to run the tests in a particular way, but that is not important for these tests.

This is the new version of patch. If there is anything wrong, feel free to tell me.

Hi, fredw (In reply to Frédéric Wang (:fredw) from comment #31) > It would be interesting to send your patches to the TryServer to see if the > tests pass on all platforms: https://wiki.mozilla.org/Build:TryServer I tried to use TryServer according to the wiki page, but I found that I have no permission to it. The log just looks like the following: ``` $ hg push -f ssh://hg.mozilla.org/try/ pushing to ssh://hg.mozilla.org/try/ remote: Permission denied (publickey). abort: no suitable response from remote hg! ```

Hi, fredw & karl I have apply for level 1 access of try server via Bug 910070. Need one voucher - any other user with level 2 or above access. Wish you can help me. :-)

Xuan Hu: did you try to send your patch to the try server?

Hi, Fred Wang I have subbmit the patch to try server The result can be found here: https://tbpl.mozilla.org/?tree=Try&rev=9dccfd92fcfc Builds and logs can be found here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/i@huxuan.org-9dccfd92fcfc There seems to have some errors, any suggestions for debuging that? Just feel a little hard to start off.

(In reply to Xuan Hu from comment #37) > There seems to have some errors, any suggestions for debuging that? Just > feel a little hard to start off. These errors don't seem to be related to your changes.

Comment on attachment 795562 [details] [diff] [review] bug-685628-fix-8.patch Thank you for your contribution here.

Xuan, can you please finish the work here (check that the patch still applies on trunk and set 'checkin needed')?

Hi, fredw, I created a new patch after apply previous version of patch. It seems to work fine and it should be in `checkin needed`. If there is any problem, feel free to tell me.

@Xuan: If you didn't do any other change, please make "obsolete" all but the latest version and change the keyword to checkin needed.

I don't have level access 3 to commit the patch.

Hi, fredw. Sorry for misunderstanding your mean by keyword `checkin needed`, thx for your help.