963079 - Apply font feature flac in MathML

Status: NEW

(Core :: MathML, enhancement)

Description

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

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

flac Flattened Accents over Capitals
This feature provides flattened forms of accents to be used over
high-rise bases such as capitals.
This feature should only change the shape of the accent and should
not move it in the vertical or horizontal direction. Moving of the
accents is done by the math handling client
Accents are flattened by the Math engine if their base is higher than
MATH.MathConstants. FlattenedAccentBaseHeight.

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.

I think we can always apply the dtls and flac font-feature in nsMathMLmunderover when ACCENTOVER is true (+ the condition on MATH.MathConstants. FlattenedAccentBaseHeight for flac). The gfx code will then automatically check substitution in the MATH font to decide which character is a "Capital" and which one has a "dotless form".

Comment 1

[James Kitchener (:jkitch)]

Attachment: Part 1: Code changes wip

Implementing 'flac' is problematic.

As far as I can tell, the condition about the base being higher can only be determined during Place, which is called during FinalizeReflow.  To then apply the font feature, the textRuns present in the descendants of the overscript frames need to be cleared and the frames invalidated.  

Platform specific test failures indicate that this invalidation process is incorrect and I am sceptical of the sanity of triggering an invalidation in the same method which later calls FinishReflowChild.

I am open to suggestions as to how it can be improved.

'dtls' on the other hand is much simpler to implement.

Comment 2

[James Kitchener (:jkitch)]

Attachment: Part 2: tests

Static and dynamic tests.

As a consequence of using the python script to create the new fonts, all the other fonts have been regenerated, but with no visible effect

Comment 3

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

(In reply to James Kitchener from comment #1)
> 'dtls' on the other hand is much simpler to implement.

We can maybe split the work for flac and dtls in separate bugs, and start with the latter which is easier.

> As a consequence of using the python script to create the new fonts, all the
> other fonts have been regenerated, but with no visible effect

Mmmh, but why did you include the changes in the patch then? I think you could just have "hg revert"-ed them.

Comment 4

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

(In reply to Frédéric Wang (:fredw) from comment #3)
> > As a consequence of using the python script to create the new fonts, all the
> > other fonts have been regenerated, but with no visible effect
> 
> Mmmh, but why did you include the changes in the patch then? I think you
> could just have "hg revert"-ed them.

Also, can you try to remove trailing whitespace from the new test files? (for example, emacs has a "delete-trailing-whitespace" command)

Comment 5

[James Kitchener (:jkitch)]

Attachment: Part 1: layout/math changes

Issues raised in comment 1 regarding the FlattenedAccentBaseHeight check still apply.  I am probably doing something very, very wrong.

The checking to see if invalidation is necessary may be overly complex/unnecessary, but I'm concerned about creating an infinite loop during the reflow process.

Comment 6

[James Kitchener (:jkitch)]

Attachment: Part 2: layout/generic changes

Near clone of attachment 8494450 [details] [diff] [review]

Comment 7

[James Kitchener (:jkitch)]

Attachment: Part 3: tests

Now with movablelimits tests and more more appropriate comments in flac-3.html

Comment 8

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

Comment on attachment 8495921 [details] [diff] [review]
Part 2: layout/generic changes

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

::: layout/generic/MathMLTextRunFactory.h
@@ +25,5 @@
>      // Style effects which may override single character <mi> behaviour
>      MATH_FONT_STYLING_NORMAL   = 0x1, // fontstyle="normal" has been set.
>      MATH_FONT_WEIGHT_BOLD      = 0x2, // fontweight="bold" has been set.
>      MATH_FONT_FEATURE_DTLS     = 0x4, // font feature dtls should be set
> +    MATH_FONT_FEATURE_FLAC     = 0x8, // font feature flac should be set

There is an extra comma here...

Comment 9

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

Comment on attachment 8495920 [details] [diff] [review]
Part 1: layout/math changes

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

I'm deferring to roc or karl for the review, as I'm not an expert on the reflow model.

One thing to note however, is that the Place method is used for two things:
1) To determine the preferred withs of MathML frames
2) To do the layout of MathML frames

During 1), bmBase.ascent & overDelta1 might not be well-defined (although they probably are in the use cases of simple text-only base we are interested in here) so we can not determine whether flac should be applied. I'm expecting that in practice the flattened form have the same widths as the normal form, so we it should be safe to ignore flac during 1). I'm not sure that will solve the invalidation issues in 2), though.

(A side note: this flac feature is probably not as important as the other work to do on stretchy operators & linebreaking, so I'd suggest not to spend too much time on it if it's likely to cause serious reflow issues).

::: layout/mathml/nsMathMLFrame.cpp
@@ +69,5 @@
>        mPresentationData.flags |= NS_MATHML_COMPRESSED;
>      }
>      // no else. the flag is sticky. it retains its value once it is set
>    }
> +  // This flag determine whether the dtls font feature setting should

determines

::: layout/mathml/nsMathMLTokenFrame.cpp
@@ +40,5 @@
> +    nsIFrame* ancestor = GetParent();
> +    nsIFrame* child = this;
> +    while (ancestor) {
> +      nsIContent* content = ancestor->GetContent();
> +      if (content) {

Can you please add a comment here, indicating what you are doing with the mover/munderover tag?

::: layout/mathml/nsMathMLmunderoverFrame.cpp
@@ +288,5 @@
>  }
>  
> +// Returns true if invalidation needed
> +// Invalidating only when necessary is performed due to the cornces of creating
> +// an infinite invalidaiton loop during reflow.

concerns
invalidation

Comment 10

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

I actually wonder if it's really necessary to specifically consider capital letters here. As I read the OpenType MATH spec, this is just an example and the only thing necessary is to compare the base height against the flattenedAccentBaseHeight parameter. I guess that would simplify a bit the patch.

Comment 11

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

Comment on attachment 8495922 [details] [diff] [review]
Part 3: tests

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

::: layout/reftests/fonts/math/generate.py
@@ +559,5 @@
> +f.addLookupSubtable("gsub", "gsub_n")
> +glyph = f["a"]
> +glyph.addPosSub("gsub_n", "b")
> +f.math.FlattenedAccentBaseHeight = 5 * em
> +saveMathFont(f)

So following my previous comment, I guess the only thing necessary here is to create one flac font with a given FlattenedAccentBaseHeight parameter and two characters 'A' and 'B' to be used as a base in the tests that would have height respectively smaller or greater than the FlattenedAccentBaseHeight.

Comment 12

[John Daggett (:jtd)]

Comment on attachment 8495921 [details] [diff] [review]
Part 2: layout/generic changes

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

::: layout/generic/MathMLTextRunFactory.cpp
@@ +567,2 @@
>          }
>        }

I don't think you really need to be this delicate. Just *prepend* the features into fontFeatureSettings. The feature merge code in the shaper will assure that user overrides are applied. This is equivalent to 'font-feature-settings: "flac" on, "flac" off', which is resolved the same way.

@@ +640,1 @@
>      }

As above, prepend this and you won't need to use the "found" logic.

Comment 13

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #12)
> ::: layout/generic/MathMLTextRunFactory.cpp
> @@ +567,2 @@
> >          }
> >        }

I love splinter...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8495920 [details] [diff] [review]
Part 1: layout/math changes

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

This does look a bit dodgy. For example. clearing the nsTextFrame textrun and allowing it to be recreated in a different state without doing a reflow in between seems dangerous.

I'm really tired and not really up to figuring out a good way to do this. Ideally we'd make Reflow only use the non-FLAC textrun and then create the FLAC textrun just for rendering (and, probably we should include it in overflow area calculation too, unioned with the non-FLAC textrun).

Comment 15

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

Comment on attachment 8495920 [details] [diff] [review]
Part 1: layout/math changes

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

I'm deferring to roc or karl for the review, as I'm not an expert on the reflow model.

One thing to note however, is that the Place method is used for two things:
1) To determine the preferred withs of MathML frames
2) To do the layout of MathML frames

During 1), bmBase.ascent & overDelta1 might not be well-defined (although they probably are in the use cases of simple text-only base we are interested in here) so we can not determine whether flac should be applied. I'm expecting that in practice the flattened form have the same widths as the normal form, so we it should be safe to ignore flac during 1). I'm not sure that will solve the invalidation issues in 2), though.

(A side note: this flac feature is probably not as important as the other work to do on stretchy operators & linebreaking, so I'd suggest not to spend too much time on it if it's likely to cause serious reflow issues).

::: layout/mathml/nsMathMLFrame.cpp
@@ +69,5 @@
>        mPresentationData.flags |= NS_MATHML_COMPRESSED;
>      }
>      // no else. the flag is sticky. it retains its value once it is set
>    }
> +  // This flag determine whether the dtls font feature setting should

determines

::: layout/mathml/nsMathMLTokenFrame.cpp
@@ +40,5 @@
> +    nsIFrame* ancestor = GetParent();
> +    nsIFrame* child = this;
> +    while (ancestor) {
> +      nsIContent* content = ancestor->GetContent();
> +      if (content) {

Can you please add a comment here, indicating what you are doing with the mover/munderover tag?

::: layout/mathml/nsMathMLmunderoverFrame.cpp
@@ +288,5 @@
>  }
>  
> +// Returns true if invalidation needed
> +// Invalidating only when necessary is performed due to the cornces of creating
> +// an infinite invalidaiton loop during reflow.

concerns
invalidation