Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla7

Status

()

Core
MathML
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: fredw, Assigned: fscholz)

Tracking

({helpwanted})

unspecified
mozilla7
helpwanted
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug], URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

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

Comment 3

6 years ago
Florian, please let me know if you are still working on this and need more hints.
(Assignee)

Comment 4

6 years ago
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.
Attachment #536261 - Attachment is obsolete: true
(Reporter)

Comment 5

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

Updated

6 years ago
Assignee: nobody → elchi3
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
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?
Attachment #540233 - Attachment is obsolete: true
Attachment #540308 - Flags: review?(karlt)
(Assignee)

Comment 7

6 years ago
Created attachment 540310 [details] [diff] [review]
patch with real reftest
Attachment #540308 - Attachment is obsolete: true
Attachment #540310 - Flags: review?(karlt)
Attachment #540308 - Flags: review?(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.
Attachment #540310 - Flags: review?(karlt)
Attachment #540310 - Flags: review-
Attachment #540310 - Flags: feedback+
(Assignee)

Comment 9

6 years ago
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.
Attachment #540310 - Attachment is obsolete: true
Attachment #540818 - Flags: review?(karlt)
Comment on attachment 540818 [details] [diff] [review]
patch with reftest

Thanks!
Attachment #540818 - Flags: review?(karlt) → review+
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
(Reporter)

Comment 11

6 years ago
http://hg.mozilla.org/mozilla-central/rev/47fb7e9c7c6a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.