Last Comment Bug 557476 - (munderover-align) [MathML3] munder, mover, munderover: add support for the align attribute
(munderover-align)
: [MathML3] munder, mover, munderover: add support for the align attribute
Status: RESOLVED FIXED
[good second bug]
: dev-doc-complete, helpwanted
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Frédéric Wang (:fredw)
:
: Anthony Jones (:kentuckyfriedtakahe, :k17e)
Mentors:
http://www.w3.org/TR/MathML3/chapter3...
Depends on: 668204
Blocks: mathml-3 788698
  Show dependency treegraph
 
Reported: 2010-04-06 03:18 PDT by Frédéric Wang (:fredw)
Modified: 2012-09-05 13:32 PDT (History)
4 users (show)
karlt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Code (5.93 KB, patch)
2011-07-18 05:56 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Reftests (46.90 KB, patch)
2011-07-18 05:58 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
Code v2 (4.84 KB, patch)
2011-07-18 07:34 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code v3 (4.84 KB, patch)
2011-07-19 05:49 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code v4 (4.71 KB, patch)
2011-07-21 00:22 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code v4 (5.62 KB, patch)
2011-07-21 00:27 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code v4 (5.57 KB, patch)
2011-08-09 03:13 PDT, Jonathan Hage
karlt: review-
Details | Diff | Splinter Review
Reftests v2 (47.11 KB, patch)
2011-08-14 16:37 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
experimental patch (4.80 KB, patch)
2011-10-12 04:40 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Reftests v3 (47.23 KB, patch)
2011-12-20 12:30 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Code v5 (4.82 KB, patch)
2011-12-20 12:31 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V6 (5.36 KB, patch)
2012-01-04 13:29 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2010-04-06 03:18:02 PDT
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) 2011-06-22 13:19:37 PDT
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 (back Dec 13 :karlt) 2011-06-22 19:23:17 PDT
(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) 2011-06-22 22:28:11 PDT
> 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) 2011-06-22 22:31:09 PDT
or maybe making nsMathMLmunderFrame and nsMathMLmunderFrame inherit from nsMathMLmunderoverFrame, and reimplement the Place function?
Comment 5 Karl Tomlinson (back Dec 13 :karlt) 2011-06-22 23:00:39 PDT
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 2011-07-18 05:56:52 PDT
Created attachment 546511 [details] [diff] [review]
Code
Comment 7 Jonathan Hage 2011-07-18 05:58:06 PDT
Created attachment 546512 [details] [diff] [review]
Reftests
Comment 8 Jonathan Hage 2011-07-18 07:34:09 PDT
Created attachment 546529 [details] [diff] [review]
Code v2
Comment 9 Frédéric Wang (:fredw) 2011-07-18 22:04:36 PDT
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 2011-07-19 05:49:37 PDT
Created attachment 546759 [details] [diff] [review]
Code v3
Comment 11 Jonathan Hage 2011-07-21 00:22:41 PDT
Created attachment 547332 [details] [diff] [review]
Code v4
Comment 12 Jonathan Hage 2011-07-21 00:27:14 PDT
Created attachment 547333 [details] [diff] [review]
Code v4
Comment 13 Jonathan Hage 2011-08-09 03:13:56 PDT
Created attachment 551711 [details] [diff] [review]
Code v4
Comment 14 Karl Tomlinson (back Dec 13 :karlt) 2011-08-11 00:28:54 PDT
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 (back Dec 13 :karlt) 2011-08-12 02:58:28 PDT
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) 2011-08-13 01:27:29 PDT
(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 2011-08-14 16:37:17 PDT
Created attachment 553053 [details] [diff] [review]
Reftests v2
Comment 18 Frédéric Wang (:fredw) 2011-10-12 04:40:52 PDT
Created attachment 566497 [details] [diff] [review]
experimental patch
Comment 19 Frédéric Wang (:fredw) 2011-12-20 12:30:45 PST
Created attachment 583250 [details] [diff] [review]
Reftests v3
Comment 20 Frédéric Wang (:fredw) 2011-12-20 12:31:18 PST
Created attachment 583251 [details] [diff] [review]
Code v5
Comment 21 Frédéric Wang (:fredw) 2011-12-20 12:35:37 PST
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 (back Dec 13 :karlt) 2011-12-20 19:56:25 PST
(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 (back Dec 13 :karlt) 2011-12-20 19:58:25 PST
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) 2011-12-20 23:49:00 PST
(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) 2012-01-04 13:29:29 PST
Created attachment 585867 [details] [diff] [review]
Patch V6

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

Note You need to log in before you can comment on or make changes to this bug.