603106 - mfrac linethickness="medium" does not match the default linethickness

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

Attachment: 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 (Open Web Docs)]

Attachment: 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)]

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)]

Florian, please let me know if you are still working on this and need more hints.

Comment 4

[Florian Scholz (Open Web Docs)]

Attachment: 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)]

> 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 (Open Web Docs)]

Attachment: 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 (Open Web Docs)]

Attachment: patch with real reftest

Comment 8

[Karl Tomlinson (:karlt)]

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 (Open Web Docs)]

Attachment: 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 (:karlt)]

Comment on attachment 540818 [details] [diff] [review]
patch with reftest

Thanks!

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/47fb7e9c7c6a