557476 - (munderover-align) [MathML3] munder, mover, munderover: add support for the align attribute

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

MathML3 adds a new attribute "align" to specify whether the scripts are aligned left, center, or right under/over the base.

Comment 1

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

As in bug 557474, we can break down the work into three steps:

1) Add parsing for the align attribute.
2) Update Place to position the under/over scripts accordingly.
3) Add reftests to test the effect of the align attribute. We may also extend the set of mstyle-*.xhtml tests, to verify the inheritance of the align attributes.

Here, there are three mathml Frames to update (nsMathMLmunderFrame, nsMathMLmoverFrame, nsMathMLmunderoverFrame) so we have to wonder whether we want to share the implementation. This does not really seem possible for the Place function, but we may want this for the parsing part. I guess we can use an mAlign member indicating the alignment of scripts, which is set in InheritAutomaticData (at init and when a parent mstyle changes) and in AttributeChanged (when the align attribute is updated). Otherwise, we can directly use GetAttribute in the Place function, but this may be less efficient.

Karl, what do you think about that?

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> Here, there are three mathml Frames to update (nsMathMLmunderFrame,
> nsMathMLmoverFrame, nsMathMLmunderoverFrame) so we have to wonder whether we
> want to share the implementation. This does not really seem possible for the
> Place function, but we may want this for the parsing part.

It would be nice if we could merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame.  I haven't looked at the Place functions but I would have expected them to be similar.

> I guess we can
> use an mAlign member indicating the alignment of scripts, which is set in
> InheritAutomaticData (at init and when a parent mstyle changes) and in
> AttributeChanged (when the align attribute is updated).  Otherwise, we can
> directly use GetAttribute in the Place function, but this may be less
> efficient.

I don't mind which way is used.  GetAttribute may be simpler.

If using InheritAutomaticData, bear in mind that it will not be called if the munderover does not have a mathml ancestor frame (because there is nothing to call RebuildAutomaticDataForChildren).  We don't need to get this pathological case right (it's not conforming); just make sure we don't have uninitialized variables causing random behavior.

Comment 3

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

> It would be nice if we could merge nsMathMLmunderFrame and
> nsMathMLmoverFrame into nsMathMLmunderoverFrame.  I haven't looked at the
> Place functions but I would have expected them to be similar.

Do you mean removing nsMathMLmunderFrame and nsMathMLmunderFrame and doing special cases in nsMathMLmunderoverFrame, using mContent->Tag()?

Comment 4

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

or maybe making nsMathMLmunderFrame and nsMathMLmunderFrame inherit from nsMathMLmunderoverFrame, and reimplement the Place function?

Comment 5

[Karl Tomlinson (:karlt)]

Yes, either of those options.  I haven't looked closely enough to work out which is better.  Probably the special cases in one class would be best if it is mostly skipping child frames for munder/mover, but the inheritance would be appropriate if implementations of methods need to be quite different.

Comment 6

[Jonathan Hage]

Attachment: Code

Comment 7

[Jonathan Hage]

Attachment: Reftests

Comment 8

[Jonathan Hage]

Attachment: Code v2

Comment 9

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

Comment on attachment 546529 [details] [diff] [review]
Code v2

>+
>+  enum Align_Attribute {
>+    center,
>+    left,
>+    right,
>+  } align_position;
>+

Maemo build fails because of the comma after the "right" item.

/builds/slave/try-lnx-maemo5-gtk/build/layout/mathml/nsMathMLmunderoverFrame.cpp: In member function 'virtual nsresult nsMathMLmunderoverFrame::Place(nsRenderingContext&, PRBool, nsHTMLReflowMetrics&)':
/builds/slave/try-lnx-maemo5-gtk/build/layout/mathml/nsMathMLmunderoverFrame.cpp:544: error: comma at end of enumerator list
make[6]: *** [nsMathMLmunderoverFrame.o] Error 1

Comment 10

[Jonathan Hage]

Attachment: Code v3

Comment 11

[Jonathan Hage]

Attachment: Code v4

Comment 12

[Jonathan Hage]

Attachment: Code v4

Comment 13

[Jonathan Hage]

Attachment: Code v4

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 546512 [details] [diff] [review]
Reftests

Looks good, thanks.

Should probably change colspan="6" to "4" in
munderover-align-accent-*-ref.html to match the test.

If the headings for the last two columns in munder-mover-* were changed from
Over to Under that would be helpful.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 551711 [details] [diff] [review]
Code v4

>   //////////
>   // pass 1, do what <mover> does: attach the overscript on the base
> 
>   // Ad-hoc - This is to override fonts which have ready-made _accent_
>   // glyphs with negative lbearing and rbearing. We want to position
>   // the overscript ourselves
>+  PRBool getattribute =

The "Ad-hoc" comment belongs with the "overWidth" code below.  The "pass 1"
comment would be better there too (after the new shared align code).

Renaming "getattribute" to "gotAttribute" would be clearer.

>+  enum Align_Attribute {
>+    center,
>+    left,
>+    right
>+  } align_position;

Can you an anonymous enum be used here?
i.e.

  enum {
    center,
    left,
    right
  } align_position;

>+  if(getattribute) {

Need a space before "(". 

>+  mBoundingMetrics.width = NS_MAX (bmBase.width, bmUnder.width);

There's some complicated and not so well documented logic already in this
function, which makes the alignment logic difficult to follow here.  Some
comments about the overall strategy might help, but considering bmUnder at this point in the algorithm seems quite a different strategy to the existing strategy using bmAnonymousBase and I don't really follow how the two are meant to work together.

>+  mBoundingMetrics.width = NS_MAX (mBoundingMetrics.width, bmOver.width);
>   nscoord overWidth = bmOver.width;
>   if (!overWidth && (bmOver.rightBearing - bmOver.leftBearing > 0)) {
>     overWidth = bmOver.rightBearing - bmOver.leftBearing;
>     dxOver = -bmOver.leftBearing;
>   }
> 
>   if (NS_MATHML_EMBELLISH_IS_ACCENTOVER(mEmbellishData.flags)) {
>-    mBoundingMetrics.width = bmBase.width; 

The existing code intentionally doesn't let an accent over affect the
positioning metrics of the base, but this code change is removing that
behavior.  I don't know why this was done for over but not under, but I'm not
sure that we want to remove it.  If we do want to remove it, then that should
be done in a separate bug with examples to indicate why it should be removed.

>+    if (align_position == center) {
>+      dxOver += correction + (mBoundingMetrics.width - overWidth)/2;
>+      dxBase = (mBoundingMetrics.width - bmBase.width)/2;
>+    } else if (align_position == left) {
>+      dxOver += correction;
>+      dxBase = 0;
>+    } else {
>+      dxOver += correction + (mBoundingMetrics.width - overWidth);
>+      dxBase = mBoundingMetrics.width - bmBase.width;
>+    }
>   }
>   else {
>+    if (align_position == center) {
>+      dxOver += correction/2 + (mBoundingMetrics.width - overWidth)/2;
>+      dxBase = (mBoundingMetrics.width - bmBase.width)/2;
>+    } else if (align_position == left) {
>+      dxOver += correction/2;
>+      dxBase = 0;
>+    } else {
>+      dxOver += correction/2 + (mBoundingMetrics.width - overWidth);
>+      dxBase = mBoundingMetrics.width - bmBase.width;
>+    }

It looks like the alignment variations are orthogonal to the accentover
variations, and so this could be simplified to two un-nested sets of condition
tests: one set considering accentover and another considering align_position.

>+    if (align_position == center) {
>+      dxUnder += (maxWidth - underWidth)/2;
>+      dxBase = (mBoundingMetrics.width - bmBase.width)/2;
>+    } else if (align_position == left) {
>+      dxUnder += 0;
>+      dxBase = 0;
>+    } else {
>+      dxUnder += (mBoundingMetrics.width - underWidth);
>+      dxBase = (mBoundingMetrics.width - bmBase.width);
>+    }  
>   }
>   else {
>+    if (align_position == center) {
>+      dxUnder += -correction/2 + (maxWidth - underWidth)/2;
>+      dxBase = (mBoundingMetrics.width - bmBase.width)/2;
>+    } else if (align_position == left) {
>+      dxUnder += -correction/2;
>+      dxBase = 0;
>+    } else {
>+      dxUnder += -correction/2 + (mBoundingMetrics.width - underWidth);
>+      dxBase = mBoundingMetrics.width - bmBase.width;
>+    }  
>   }
>   nscoord dxAnonymousBase = (maxWidth - bmAnonymousBase.width)/2;

I don't understand why dxBase is being set again here.
Would it make sense to adjust dxAnonymousBase according to align_position?
Would that make the early "mBoundingMetrics.width = NS_MAX (bmBase.width,
bmUnder.width);" unnecessary?

Comment 16

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

(In reply to Karl Tomlinson (:karlt) from comment #14)
> Comment on attachment 546512 [details] [diff] [review]
> Reftests
> 
> Looks good, thanks.
> 
> Should probably change colspan="6" to "4" in
> munderover-align-accent-*-ref.html to match the test.
> 
> If the headings for the last two columns in munder-mover-* were changed from
> Over to Under that would be helpful.

Also, the TABLE elements can maybe be changed to lowercase.

Comment 17

[Jonathan Hage]

Attachment: Reftests v2

Comment 18

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

Attachment: experimental patch

Comment 19

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

Attachment: Reftests v3

Comment 20

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

Attachment: Code v5

Comment 21

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

I'm trying to understand the logic behind the italic correction. Doing dxOver += correction may make sense, if we want to align the left side of the over and base elements, but bmOver.leftBearing seems to be ignored. I don't really understand why we use a 1/2 factor in the non-accentover case. I understand even less the case of munder. As said above, no correction is done in the accentunder contrary to mover. Moreover I can't see why the correction becomes negative.

I've updated Jonathan's patches. TABLE is replaced by table in the reftests. The patch for the code should hopefully preserved the current behavior when align=center but I'm not sure what is done for left/right is correct with respect to the italic correction.

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to Frédéric Wang from comment #21)
> I'm trying to understand the logic behind the italic correction.

File history at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLmunderoverFrame.cpp&rev=1.30
leads to bug 135940.

I don't know whether bz understood it, but rbs said "the code is pretty
obscure without consulting the TeX manual", though I don't see TeX references
for this particular code so perhaps he was referring to vertical offsets.

It looks to me like the italic correction is a heuristic estimate of the slant
on italic characters, and there is an implicit assumption that the slant to
the right, i.e. an LTR script.

> Doing dxOver += correction may make sense [...]

This seems to be moving the accent to the right as far as the estimate of the
font's lean to the right.

> I don't really understand why we use a 1/2 factor in the non-accentover case.

Me neither.  A non-accent over would be further away from the base, and so I'd
expect a bigger adjustment (~1.5) to follow the slant of the font.  Perhaps, the factor might be compromise because, if it is not an accent, perhaps the position should not always follow the slant of the font.

> I understand even less the case of munder. As said above, no correction is
> done in the accentunder contrary to mover. Moreover I can't see why the
> correction becomes negative.

I'm guessing no correction is done for accentunder, based on an assumption
that the accent is likely to be near the baseline and the effect of the font
slant on the position of the character at the baseline is minimal.

For the non-accent under, the offset to the left seems to follow the slant of
the font and the factor of 1/2 seems about right for a roughly 0.5 em distance from the baseline.

> The patch for the code should hopefully preserved the current behavior when
> align=center but I'm not sure what is done for left/right is correct with
> respect to the italic correction.

I don't know what would be best for left/right.
Keeping a similar correction as for center is one option.
Another option is to have no correction for left/right.

Comment 23

[Karl Tomlinson (:karlt)]

For the file history link, I meant
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLmunderoverFrame.cpp&rev=1.62

Comment 24

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

(In reply to Karl Tomlinson (:karlt) from comment #22)
> > The patch for the code should hopefully preserved the current behavior when
> > align=center but I'm not sure what is done for left/right is correct with
> > respect to the italic correction.
> 
> I don't know what would be best for left/right.
> Keeping a similar correction as for center is one option.
> Another option is to have no correction for left/right.

I would probably suggest the latter. I see this correction as heuristic rules to try to automatically position the over/under just above/below the top/bottom of the base drawing. When @align=left/right, the author just wants the left/right sides to align, so I don't think the correction is necessary.

We could also suppress this correction when dir=RTL (I seem to remember having read somewhere that Arabic don't use italic).

I don't plan to work on this right now. I've asked Jonathan whether he wants to finish this.

Comment 25

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

Attachment: Patch V6

https://tbpl.mozilla.org/?tree=Try&rev=6afdec995e00

Comment 26

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e771f3c2cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/505df7671545

Comment 27

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/505df7671545
https://hg.mozilla.org/mozilla-central/rev/44d3202f73e0

Comment 28

[Tom S. (please needinfo tschuster)]

I added a short note to https://developer.mozilla.org/en/Firefox_12_for_developers.
And just added the "Requires Gecko 12.0" instead of the bug number in
https://developer.mozilla.org/en/MathML/Element/munder
https://developer.mozilla.org/en/MathML/Element/mover
https://developer.mozilla.org/en/MathML/Element/munderover