Closed
Bug 1069929
Opened 5 years ago
Closed 5 years ago
Apply font features dtls
Categories
(Core :: MathML, enhancement)
Core
MathML
Not set
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jkitch, Assigned: jkitch, Mentored)
References
Details
(Keywords: devdoccomplete)
Attachments
(3 files, 3 obsolete files)
5.39 KB,
patch

Details  Diff  Splinter Review  
5.86 KB,
patch

roc
:
review+

Details  Diff  Splinter Review 
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 fontfeature 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•5 years ago


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 nonMathML children indiscriminately is desirable.
Attachment #8492614 
Flags: review?(fred.wang)
Assignee  
Comment 2•5 years ago


Implemented in a similar, but simpler way compared to the ssty font feature setting.
Assignee  
Comment 3•5 years ago


Static and dynamic tests.
Attachment #8492616 
Flags: review?(fred.wang)
Comment 4•5 years ago


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/dtls1ref.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/dtls1.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/dtls2ref.html @@ +11,5 @@ > + <body> > + > + <math> > + <mstyle style="fontfamily: 'dtls1';"> > + <mover accent="true" > extra space before > (and in other places too) ::: layout/reftests/mathml/dtls2.html @@ +10,5 @@ > + <body> > + > + <math> > + <mstyle style="fontfamily: 'dtls1';"> > + <mover accent="true" id="mover0"> 2 spaces
Attachment #8492616 
Flags: review?(fred.wang) → review+
Comment 5•5 years ago


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 readonly 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+
Comment 6•5 years ago


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: devdocneeded
Comment 7•5 years ago


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•5 years ago


Review comments addressed and fixed to account for movablelimits
Attachment #8492614 
Attachment is obsolete: true
Assignee  
Comment 9•5 years ago


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•5 years ago


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+
Assignee  
Comment 12•5 years ago


https://tbpl.mozilla.org/?tree=Try&rev=ec3f285762ed
Keywords: checkinneeded
Comment 13•5 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/91ed3a014b14 https://hg.mozilla.org/integration/mozillainbound/rev/8bbd9f499661 https://hg.mozilla.org/integration/mozillainbound/rev/3d5434e51b19
Flags: intestsuite+
Keywords: checkinneeded
Comment 14•5 years ago


https://hg.mozilla.org/mozillacentral/rev/91ed3a014b14 https://hg.mozilla.org/mozillacentral/rev/8bbd9f499661 https://hg.mozilla.org/mozillacentral/rev/3d5434e51b19
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution:  → FIXED
Target Milestone:  → mozilla35
Comment 15•5 years ago


https://developer.mozilla.org/enUS/Firefox/Releases/35 https://developer.mozilla.org/enUS/docs/Web/CSS/fontfeaturesettings https://wiki.mozilla.org/MathML:Open_Type_MATH_Table#Implementation_Status
Keywords: devdocneeded → devdoccomplete
You need to log in
before you can comment on or make changes to this bug.
Description
•