Closed Bug 1069929 Opened 10 years ago Closed 10 years ago

Apply font features dtls

Categories

(Core :: MathML, enhancement)

Product:
Core
Component:
MathML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: jkitch, Assigned: jkitch, Mentored)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

Part 1: MathML Changes
5.39 KB, patch
Details | Diff | Splinter Review
Part 2: layout/generic changes
5.86 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 3: tests
28.57 KB, patch
Details | Diff | Splinter Review
+++ 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.
Attached patch Part 1: MathML Changes (obsolete) — — Splinter Review
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)
Attached patch Part 2: layout/generic changes (obsolete) — — Splinter Review
Implemented in a similar, but simpler way compared to the ssty font feature setting.
Attached patch Part 3: tests (obsolete) — — Splinter Review
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
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+
Review comments addressed and fixed to account for movablelimits
Attachment #8492614 - Attachment is obsolete: true
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)
Attached patch Part 3: tests — — Splinter Review
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: