The default bug view has changed. See this FAQ.

mpadded: height/depth should apply to logical metrics

RESOLVED FIXED in mozilla28

Status

()

Core
MathML
P5
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: fredw, Assigned: Xuan Hu)

Tracking

(Blocks: 1 bug, {helpwanted})

Trunk
mozilla28
helpwanted
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=fredw][lang=c++])

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 687809
(Reporter)

Updated

5 years ago
Whiteboard: [good second bug] → [mentor=fredw][lang=c++]

Comment 1

5 years ago
Created attachment 623514 [details] [diff] [review]
Proposed Patch for Bug 685628 - mpadded: height/depth should apply to logical metrics
Attachment #623514 - Flags: feedback?(fred.wang)
(Reporter)

Comment 2

5 years ago
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.
Attachment #623514 - Flags: feedback?(fred.wang) → feedback+
(Reporter)

Comment 3

5 years ago
Assigning to Hu Xuan.
Assignee: nobody → i
(Assignee)

Comment 4

5 years ago
Created attachment 644348 [details] [diff] [review]
First trial for bug #685628

A trial for bug fix as well as some redundant spaces removed.
Attachment #644348 - Flags: review?(fred.wang)
(Reporter)

Comment 5

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

Comment 6

5 years ago
Created attachment 644361 [details] [diff] [review]
Trial Patch

Patch Updated removing changes related to whitespace.
Attachment #644348 - Attachment is obsolete: true
Attachment #644348 - Flags: review?(fred.wang)
Attachment #644361 - Flags: review?(fred.wang)
(Reporter)

Comment 7

5 years ago
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?
Attachment #644361 - Flags: review?(fred.wang)
Attachment #644361 - Flags: review-
Attachment #644361 - Flags: feedback+
(Reporter)

Updated

5 years ago
Attachment #644361 - Flags: feedback+ → feedback?(karlt)
(Reporter)

Updated

5 years ago
Attachment #644361 - Flags: feedback+
(Assignee)

Comment 8

5 years ago
Created attachment 644874 [details] [diff] [review]
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.
Attachment #644361 - Attachment is obsolete: true
Attachment #644361 - Flags: feedback?(karlt)
Attachment #644874 - Flags: review?(karlt)
Attachment #644874 - Flags: review?(fred.wang)
(Reporter)

Comment 9

5 years ago
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;
Attachment #644874 - Flags: review?(fred.wang)
Attachment #644874 - Flags: review-
Attachment #644874 - Flags: feedback+
(Assignee)

Comment 10

5 years ago
Created attachment 644900 [details] [diff] [review]
Bug 685628: Make mpadded apply height/depth to logical metrics. r=fredw f=karlt

Sorry for the silly error.
Attachment #644874 - Attachment is obsolete: true
Attachment #644874 - Flags: review?(karlt)
Attachment #644900 - Flags: review?(fred.wang)
Attachment #644900 - Flags: feedback?(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.
Attachment #644900 - Flags: feedback?(karlt) → feedback+
(Reporter)

Comment 12

5 years ago
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.
(Reporter)

Updated

5 years ago
Attachment #644900 - Flags: review?(fred.wang) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 645256 [details] [diff] [review]
685628-5.patch
Attachment #644900 - Attachment is obsolete: true
Attachment #645256 - Flags: review?(fred.wang)
(Reporter)

Comment 14

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

Thanks. If you need to discuss a bit more reftests, don't hesitate.
Attachment #645256 - Flags: review?(fred.wang) → review+
(Assignee)

Comment 15

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

Comment 16

5 years ago
Thx for karl's explanation. I found the reason of my mistake.
I will be back after some progress.
(Reporter)

Updated

4 years ago
Priority: -- → P5
(Reporter)

Comment 17

4 years ago
Xuan, are you still working on this bug?
Flags: needinfo?(i)
(Assignee)

Comment 18

4 years ago
Hi Fredw:

Sorry, I have completely forget this.
I want to finish it if there is no others instead.
It's that ok?
Flags: needinfo?(i)
(Reporter)

Comment 19

4 years ago
(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?
Flags: needinfo?(i)
(Assignee)

Comment 21

4 years ago
Created attachment 742339 [details] [diff] [review]
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. :-)
Attachment #645256 - Attachment is obsolete: true
Attachment #742339 - Flags: superreview?(fred.wang)
Attachment #742339 - Flags: review?(josh)
Flags: needinfo?(i)
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.
Attachment #742339 - Flags: superreview?(fred.wang)
Attachment #742339 - Flags: review?(josh)
Attachment #742339 - Flags: review?(fred.wang)
(Assignee)

Comment 23

4 years ago
Created attachment 742969 [details] [diff] [review]
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. :-)
Attachment #742339 - Attachment is obsolete: true
Attachment #742339 - Flags: review?(fred.wang)
Attachment #742969 - Flags: review?(fred.wang)
(Reporter)

Comment 24

4 years ago
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.
(Reporter)

Comment 25

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

See comment 24.
Attachment #742969 - Flags: review?(fred.wang) → feedback+
(Reporter)

Comment 26

4 years ago
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.
Flags: needinfo?(i)
(Assignee)

Comment 27

4 years ago
OK, I will deal with it when I have time.
Currently I am busy with another GSoC project.
Sorry for the delay. :-(
Flags: needinfo?(i)
(Assignee)

Comment 28

4 years ago
Created attachment 795138 [details]
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.
Attachment #795138 - Flags: feedback?(fred.wang)
(Reporter)

Comment 29

4 years ago
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 :-(
(Assignee)

Comment 30

4 years ago
Created attachment 795179 [details]
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?
Attachment #795138 - Attachment is obsolete: true
Attachment #795138 - Flags: feedback?(fred.wang)
Attachment #795179 - Flags: feedback?
(Reporter)

Comment 31

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

Comment 33

4 years ago
Created attachment 795562 [details] [diff] [review]
bug-685628-fix-8.patch

This is the new version of patch. If there is anything wrong, feel free to tell me.
Attachment #742969 - Attachment is obsolete: true
Attachment #795562 - Flags: review?(fred.wang)
(Assignee)

Comment 34

4 years ago
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!
```
(Assignee)

Comment 35

4 years ago
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. :-)
(Reporter)

Comment 36

4 years ago
Xuan Hu: did you try to send your patch to the try server?
(Assignee)

Comment 37

4 years ago
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.
(Reporter)

Comment 38

4 years ago
(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.
(Reporter)

Updated

4 years ago
Attachment #795562 - Flags: review?(fred.wang) → review?(karlt)
Comment on attachment 795562 [details] [diff] [review]
bug-685628-fix-8.patch

Thank you for your contribution here.
Attachment #795562 - Flags: review?(karlt) → review+
(Reporter)

Comment 40

4 years ago
Xuan, can you please finish the work here (check that the patch still applies on trunk and set 'checkin needed')?
Flags: needinfo?(i)
(Assignee)

Comment 41

3 years ago
Created attachment 830264 [details] [diff] [review]
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.
Attachment #795562 - Attachment is obsolete: true
Attachment #830264 - Flags: review?(fred.wang)
Flags: needinfo?(i)
(Reporter)

Updated

3 years ago
Attachment #830264 - Flags: review?(fred.wang)
(Reporter)

Updated

3 years ago
Attachment #795179 - Flags: feedback?
(Reporter)

Comment 42

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

Updated

3 years ago
Attachment #795179 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #830264 - Flags: checkin?(fred.wang)
(Reporter)

Comment 43

3 years ago
I don't have level access 3 to commit the patch.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #830264 - Flags: checkin?(fred.wang)
(Assignee)

Comment 44

3 years ago
Hi, fredw. Sorry for misunderstanding your mean by keyword `checkin needed`, thx for your help.
Attachment #623514 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b7146e0b7d
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07b7146e0b7d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.