685628 - mpadded: height/depth should apply to logical metrics

Status: RESOLVED FIXED

(Core :: MathML, defect, P5)

Description

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

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 1

[Anjor]

Attachment: Proposed Patch for Bug 685628 - mpadded: height/depth should apply to logical metrics

Comment 2

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

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.

Comment 3

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

Assigning to Hu Xuan.

Comment 4

[Xuan Hu]

Attachment: First trial for bug #685628

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

Comment 5

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

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

Comment 6

[Xuan Hu]

Attachment: Trial Patch

Patch Updated removing changes related to whitespace.

Comment 7

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

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?

Comment 8

[Xuan Hu]

Attachment: Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw,karlt

Bug fix according to fredw's suggestion.

Still working on reftest.

If there's any further problems, feel free to contact me.

Comment 9

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

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;

Comment 10

[Xuan Hu]

Attachment: Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt

Sorry for the silly error.

Comment 11

[Karl Tomlinson (:karlt)]

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 12

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

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 13

[Xuan Hu]

Attachment: 685628-5.patch

Comment 14

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

Comment on attachment 645256 [details] [diff] [review]
685628-5.patch

Thanks. If you need to discuss a bit more reftests, don't hesitate.

Comment 15

[Xuan Hu]

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

Comment 16

[Xuan Hu]

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

Comment 17

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

Xuan, are you still working on this bug?

Comment 18

[Xuan Hu]

Hi Fredw：

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

Comment 19

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

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

Comment 20

[Josh Matthews [:jdm]]

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

Comment 21

[Xuan Hu]

Attachment: sixth try for bug fix 685628

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 22

[Josh Matthews [:jdm]]

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.

Comment 23

[Xuan Hu]

Attachment: bug-685628-fix-7

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

Comment 24

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

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 25

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

Comment on attachment 742969 [details] [diff] [review]
bug-685628-fix-7

See comment 24.

Comment 26

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

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.

Comment 27

[Xuan Hu]

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

Comment 28

[Xuan Hu]

Attachment: Diff of reftests

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.

Comment 29

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

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

Comment 30

[Xuan Hu]

Attachment: Diff of reftests - 2

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?

Comment 31

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

(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

Comment 32

[Karl Tomlinson (:karlt)]

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

Comment 33

[Xuan Hu]

Attachment: bug-685628-fix-8.patch

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

Comment 34

[Xuan Hu]

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!
```

Comment 35

[Xuan Hu]

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

Comment 36

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

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

Comment 37

[Xuan Hu]

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/[e-mail]-9dccfd92fcfc

There seems to have some errors, any suggestions for debuging that? Just feel a little hard to start off.

Comment 38

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

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

[Karl Tomlinson (:karlt)]

Comment on attachment 795562 [details] [diff] [review]
bug-685628-fix-8.patch

Thank you for your contribution here.

Comment 40

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

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

Comment 41

[Xuan Hu]

Attachment: bug-685628-fix-9.patch

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.

Comment 42

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

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

Comment 43

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

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

Comment 44

[Xuan Hu]

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07b7146e0b7d

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07b7146e0b7d