Last Comment Bug 603106 - mfrac linethickness="medium" does not match the default linethickness
: mfrac linethickness="medium" does not match the default linethickness
Status: RESOLVED FIXED
[good first bug]
: helpwanted
Product: Core
Classification: Components
Component: MathML (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Florian Scholz [:fscholz] (MDN)
:
Mentors:
http://www.w3.org/Math/testsuite/buil...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-09 11:07 PDT by Frédéric Wang (:fredw)
Modified: 2011-06-23 11:41 PDT (History)
0 users
fred.wang: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (521 bytes, application/xhtml+xml)
2010-10-09 11:07 PDT, Frédéric Wang (:fredw)
no flags Details
patch with reftest (4.58 KB, patch)
2011-05-31 03:10 PDT, Florian Scholz [:fscholz] (MDN)
no flags Details | Diff | Review
patch with reftest (6.35 KB, patch)
2011-06-18 06:19 PDT, Florian Scholz [:fscholz] (MDN)
no flags Details | Diff | Review
patch with reftest (4.35 KB, patch)
2011-06-19 03:25 PDT, Florian Scholz [:fscholz] (MDN)
no flags Details | Diff | Review
patch with real reftest (6.27 KB, patch)
2011-06-19 03:35 PDT, Florian Scholz [:fscholz] (MDN)
karlt: review-
karlt: feedback+
Details | Diff | Review
patch with reftest (5.66 KB, patch)
2011-06-21 11:39 PDT, Florian Scholz [:fscholz] (MDN)
karlt: review+
Details | Diff | Review

Description Frédéric Wang (:fredw) 2010-10-09 11:07:11 PDT
Created attachment 482061 [details]
testcase

The spec says that they should be the same.
http://www.w3.org/TR/MathML3/chapter3.html#id.3.3.2.2

See the MathML testsuite: http://www.w3.org/Math/testsuite/build/main/Presentation/CSS/mfrac/mfrac-04-full.xhtml

and the attachment.
Comment 1 Florian Scholz [:fscholz] (MDN) 2011-05-31 03:10:55 PDT
Created attachment 536261 [details] [diff] [review]
patch with reftest

I just dived into the Mozilla code and I think this worth a try now. Also added a reftest. Thoughts?
Comment 2 Frédéric Wang (:fredw) 2011-05-31 03:45:54 PDT
Thanks Florian.

The reftest seems good.

For the C++ part, I think we should try to have something symmetric. With the new value of MEDIUM_FRACTION_LINE, thin is expected to be twice thinner and thick twice thicker, so maybe something like

MINIMUM_PIXELS: 1 (thin), 2 (medium) and 4 (thick)

and in CalcLineThickness the conditions

lineThickness <= defaultThickness - onePixel (thin)
lineThickness = defaultThickness             (medium)
lineThickness >= defaultThickness + onePixel (thick)

When aThicknessAttribute.IsEmpty(), the program should also follow the same path as "medium" (to set minimumThickness).

I don't know how often the minimum case happens. If the values above change the default rendering in an undesired way, maybe you can try

MINIMUM_PIXELS: 1 (thin), 1 (medium) and 2 (thick)

instead.
Comment 3 Frédéric Wang (:fredw) 2011-06-16 12:12:53 PDT
Florian, please let me know if you are still working on this and need more hints.
Comment 4 Florian Scholz [:fscholz] (MDN) 2011-06-18 06:19:16 PDT
Created attachment 540233 [details] [diff] [review]
patch with reftest

Thanks for your comments Frederic!

I think in order to make the thickness of linethickness="1" and linethickness="medium" identical, we should use MINIMUM_PIXELS: 1 (thin), 1 (medium) and 2 (thick). 

I've updated the reftest to test: default == medium, default == 1 and 1 == medium. Also updated the conditions in CalcLineThickness and added a statement for aThicknessAttribute.IsEmpty() to address comment 2.
Comment 5 Frédéric Wang (:fredw) 2011-06-19 02:03:18 PDT
> I think in order to make the thickness of linethickness="1" and
> linethickness="medium" identical, we should use MINIMUM_PIXELS: 1 (thin), 1
> (medium) and 2 (thick). 
> 
> I've updated the reftest to test: default == medium, default == 1 and 1 ==
> medium. Also updated the conditions in CalcLineThickness and added a
> statement for aThicknessAttribute.IsEmpty() to address comment 2.

Yes, the REC says that linethickness="1" is also the default in the description of mstyle:
http://www.w3.org/TR/MathML/chapter3.html#id.3.3.4.1

Your patch looks good to me, except that you can remove the following lines in the "medium" statement.

if (lineThickness < defaultThickness)
  lineThickness = defaultThickness;

because MEDIUM_FRACTION_LINE is now 1 and so lineThickness == defaultThickness.

(Actually, because MEDIUM_FRACTION_LINE_MINIMUM_PIXELS and MEDIUM_FRACTION_LINE are now 1, I think the cases medium and default are useless, but you can leave them to understand better what's happening.)

When you feel that your patch is ready, you can ask Karl to review it.
Comment 6 Florian Scholz [:fscholz] (MDN) 2011-06-19 03:25:31 PDT
Created attachment 540308 [details] [diff] [review]
patch with reftest

Removed unnecessary lines in the "medium" statement.

> (Actually, because MEDIUM_FRACTION_LINE_MINIMUM_PIXELS and
> MEDIUM_FRACTION_LINE are now 1, I think the cases medium and default are
> useless, but you can leave them to understand better what's happening.)
Yep, I think that's right. I kept them for now, though. 

So, what do you think about it Karl?
Comment 7 Florian Scholz [:fscholz] (MDN) 2011-06-19 03:35:55 PDT
Created attachment 540310 [details] [diff] [review]
patch with real reftest
Comment 8 Karl Tomlinson (ni?:karlt) 2011-06-19 22:17:45 PDT
Comment on attachment 540310 [details] [diff] [review]
patch with real reftest

Thank you for fixing this.

>+  else { // use "medium" if ThicknessAttribute is not specified
>+    lineThickness = NSToCoordRound(defaultThickness * MEDIUM_FRACTION_LINE);
>+    minimumThickness = onePixel * MEDIUM_FRACTION_LINE_MINIMUM_PIXELS;
>+  }

I'd prefer to not add this case.  As pointed out it is not necessary, and I think it is clearer to not have the case because there still needs to be default values set for when ParseNumericValue() fails.

>     else if (aThicknessAttribute.EqualsLiteral("medium")) {
>       lineThickness = NSToCoordRound(defaultThickness * MEDIUM_FRACTION_LINE);
>       minimumThickness = onePixel * MEDIUM_FRACTION_LINE_MINIMUM_PIXELS;

There is value in having the "medium" condition block: it makes it clear that nothing funny is going to happen in ParseNumericValue().
However, I think it is best to remove the lineThickness and minimumThickness assignments here to have just an empty block
with a comment saying that medium is default.

MEDIUM_FRACTION_LINE and MEDIUM_FRACTION_LINE_MINIMUM_PIXELS can also be removed then.
Comment 9 Florian Scholz [:fscholz] (MDN) 2011-06-21 11:39:16 PDT
Created attachment 540818 [details] [diff] [review]
patch with reftest

Yup, let's do it properly without the useless cases. I've updated the patch to address comment 8.
Comment 10 Karl Tomlinson (ni?:karlt) 2011-06-21 14:59:04 PDT
Comment on attachment 540818 [details] [diff] [review]
patch with reftest

Thanks!
Comment 11 Frédéric Wang (:fredw) 2011-06-23 11:41:06 PDT
http://hg.mozilla.org/mozilla-central/rev/47fb7e9c7c6a

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