Apply font features dtls

RESOLVED FIXED in mozilla35

Status

()

Core
MathML
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jkitch, Assigned: jkitch, Mentored)

Tracking

({dev-doc-complete})

Trunk
mozilla35
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #963079 +++

This will be similar to bug 442637: the MathML engine must set flags and the text frames will apply the font-feature settings.

dtls
Dotless Forms
This feature provides dotless forms for Math Alphanumeric
characters, such as U+1D422 MATHEMATICAL BOLD SMALL I,
U+1D423 MATHEMATICAL BOLD SMALL J, U+1D456
U+MATHEMATICAL ITALIC SMALL I, U+1D457 MATHEMATICAL ITALIC
SMALL J, and so on.
The dotless forms are to be used as base forms for placing
mathematical accents over them.
(Assignee)

Comment 1

4 years ago
Created attachment 8492614 [details] [diff] [review]
Part 1: MathML Changes

This flags all token frame descendants of a <mover> or <munderover> base frame as being eligible for the dtls font feature.

(Technically it gets set on all base children by reusing the PropagatePresentationDataFor method, but only token children do anything about the flag).

The alternative to using presentation flags is to use frame flags, but the effect would be the same unless applying the dtls font feature to non-MathML children indiscriminately is desirable.
Attachment #8492614 - Flags: review?(fred.wang)
(Assignee)

Comment 2

4 years ago
Created attachment 8492615 [details] [diff] [review]
Part 2: layout/generic changes

Implemented in a similar, but simpler way compared to the ssty font feature setting.
(Assignee)

Comment 3

4 years ago
Created attachment 8492616 [details] [diff] [review]
Part 3: tests

Static and dynamic tests.
Attachment #8492616 - Flags: review?(fred.wang)
Comment on attachment 8492616 [details] [diff] [review]
Part 3: tests

Review of attachment 8492616 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I wonder how important is the case of nested munderover constructions as I can't really see a concrete use case for it. But I guess the implementation is fine.

Perhaps we'd also like to test some things that are used in practice e.g.

<mover><mi>i</mi><mo>^</mo></mover>

(maybe check how current OpenType MATH fonts use dtls)

and related things like how this interacts with the implicit/explicit mathvariant transform of the base or with the implicit/explicit accent property of the operator.

::: layout/reftests/mathml/dtls-1-ref.html
@@ +93,5 @@
> +    <mover accent="false">
> +      <mn>c</mn>
> +      <mn>c</mn>
> +    </mover>
> +    <munderover  accent="false" accentunder="false">

2 spaces between munderover and accent

::: layout/reftests/mathml/dtls-1.html
@@ +95,5 @@
> +    <mover accent="false">
> +      <mn>a</mn>
> +      <mn>a</mn>
> +    </mover>
> +    <munderover  accent="false" accentunder="false">

2 spaces between munderover and accent

::: layout/reftests/mathml/dtls-2-ref.html
@@ +11,5 @@
> +  <body>
> +
> +  <math>
> +    <mstyle style="font-family: 'dtls-1';">
> +      <mover accent="true" >

extra space before > (and in other places too)

::: layout/reftests/mathml/dtls-2.html
@@ +10,5 @@
> +  <body>
> +
> +  <math>
> +    <mstyle style="font-family: 'dtls-1';">
> +      <mover accent="true"  id="mover0">

2 spaces
Attachment #8492616 - Flags: review?(fred.wang) → review+
Comment on attachment 8492614 [details] [diff] [review]
Part 1: MathML Changes

Review of attachment 8492614 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsIMathMLFrame.h
@@ +318,5 @@
>  #define NS_MATHML_IS_SPACE_LIKE(_flags) \
>    (NS_MATHML_SPACE_LIKE == ((_flags) & NS_MATHML_SPACE_LIKE))
>  
> +#define NS_MATHML_SET_DTLS(_flags) \
> +  (NS_MATHML_DTLS == ((_flags) & NS_MATHML_DTLS))

perhaps this should be called IS_DTLS_SET or HAS_DTLS to clarify that it is a read-only operation.

::: layout/mathml/nsMathMLFrame.cpp
@@ +56,5 @@
>  NS_IMETHODIMP
>  nsMathMLFrame::UpdatePresentationData(uint32_t        aFlagsValues,
>                                        uint32_t        aWhichFlags)
>  {
> +  NS_ASSERTION(aWhichFlags & (NS_MATHML_COMPRESSED | NS_MATHML_DTLS),

Maybe NS_MATHML_IS_COMPRESSED(aWhichFlags) || NS_MATHML_SET_DTLS(aWhichFlags) would be more readable?
Attachment #8492614 - Flags: review?(fred.wang) → review+
Another idea for the tests: an munderover element whose base has the movablelimits property should render as an msubsup element in inline style and so the dtls feature should not be applied.
Mentor: fred.wang@free.fr
Keywords: dev-doc-needed
Comment on attachment 8492615 [details] [diff] [review]
Part 2: layout/generic changes

Review of attachment 8492615 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/MathMLTextRunFactory.cpp
@@ +596,5 @@
>        }
> +      /*
> +        Apply the dtls font feature setting (dotless).
> +        This gets applied to the first token frame found within the base
> +        frame of mover and munderover elements.

I didn't see that covered by the tests of patch 3.

So you mean that for

<mover><mrow><mi>i</mi><mi>j</mi><mo>^</mo></mover>

the dtls will only be applied to the first <mi>i</mi>? I would have thought it would apply to the two <mi> tokens... (but again, I don't know if that case is very used in practice).
Attachment #8492615 - Flags: feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8494448 [details] [diff] [review]
Part 1: MathML Changes

Review comments addressed and fixed to account for movablelimits
Attachment #8492614 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8494450 [details] [diff] [review]
Part 2: layout/generic changes

Comment fixed.  All baseframe descendants get the dtls flag.

The approach in the patch is similar to the code path which sets the ssty font feature, although it is simpler as there are only two possible states.

It will have no effect on anything that isn't MathML.
Attachment #8492615 - Attachment is obsolete: true
Attachment #8494450 - Flags: review?(roc)
(Assignee)

Comment 10

4 years ago
Created attachment 8494456 [details] [diff] [review]
Part 3: tests

mathvariant and movable limit tests added
Attachment #8492616 - Attachment is obsolete: true
Comment on attachment 8494450 [details] [diff] [review]
Part 2: layout/generic changes

Review of attachment 8494450 [details] [diff] [review]:
-----------------------------------------------------------------

great!
Attachment #8494450 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/91ed3a014b14
https://hg.mozilla.org/mozilla-central/rev/8bbd9f499661
https://hg.mozilla.org/mozilla-central/rev/3d5434e51b19
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.