Bug 557476 (munderover-align)

[MathML3] munder, mover, munderover: add support for the align attribute

RESOLVED FIXED in mozilla12

Status

()

Core
MathML
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

(Blocks: 1 bug, {dev-doc-complete, helpwanted})

Trunk
mozilla12
dev-doc-complete, helpwanted
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good second bug], URL)

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

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

Updated

7 years ago
Blocks: 534959
(Assignee)

Updated

6 years ago
Whiteboard: [good second bug]
(Assignee)

Updated

6 years ago
Assignee: nobody → hage.jonathan
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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?
(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.
(Assignee)

Comment 3

6 years ago
> 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()?
(Assignee)

Comment 4

6 years ago
or maybe making nsMathMLmunderFrame and nsMathMLmunderFrame inherit from nsMathMLmunderoverFrame, and reimplement the Place function?
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.

Updated

6 years ago
Depends on: 668204

Comment 6

6 years ago
Created attachment 546511 [details] [diff] [review]
Code

Comment 7

6 years ago
Created attachment 546512 [details] [diff] [review]
Reftests

Comment 8

6 years ago
Created attachment 546529 [details] [diff] [review]
Code v2
Attachment #546511 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
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

6 years ago
Created attachment 546759 [details] [diff] [review]
Code v3
Attachment #546529 - Attachment is obsolete: true

Comment 11

6 years ago
Created attachment 547332 [details] [diff] [review]
Code v4
Attachment #546759 - Attachment is obsolete: true

Comment 12

6 years ago
Created attachment 547333 [details] [diff] [review]
Code v4
Attachment #547332 - Attachment is obsolete: true

Updated

6 years ago
Attachment #546512 - Flags: review?(karlt)

Updated

6 years ago
Attachment #547333 - Flags: review?(karlt)

Comment 13

6 years ago
Created attachment 551711 [details] [diff] [review]
Code v4
Attachment #547333 - Attachment is obsolete: true
Attachment #551711 - Flags: review?(karlt)
Attachment #547333 - Flags: review?(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.
Attachment #546512 - Flags: review?(karlt) → review+
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?
Attachment #551711 - Flags: review?(karlt) → review-
(Assignee)

Comment 16

6 years ago
(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

6 years ago
Created attachment 553053 [details] [diff] [review]
Reftests v2
Attachment #546512 - Attachment is obsolete: true

Updated

6 years ago
Attachment #553053 - Flags: review?(karlt)
Attachment #553053 - Flags: review?(karlt) → review+
(Assignee)

Comment 18

6 years ago
Created attachment 566497 [details] [diff] [review]
experimental patch
(Assignee)

Comment 19

5 years ago
Created attachment 583250 [details] [diff] [review]
Reftests v3
(Assignee)

Comment 20

5 years ago
Created attachment 583251 [details] [diff] [review]
Code v5
Attachment #551711 - Attachment is obsolete: true
Attachment #553053 - Attachment is obsolete: true
Attachment #566497 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
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.
(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.
For the file history link, I meant
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLmunderoverFrame.cpp&rev=1.62
(Assignee)

Comment 24

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: hage.jonathan → fred.wang
(Assignee)

Comment 25

5 years ago
Created attachment 585867 [details] [diff] [review]
Patch V6

https://tbpl.mozilla.org/?tree=Try&rev=6afdec995e00
Attachment #583251 - Attachment is obsolete: true
Attachment #585867 - Flags: review?(karlt)
Attachment #585867 - Flags: review?(karlt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e771f3c2cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/505df7671545
Flags: in-testsuite+
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/505df7671545
https://hg.mozilla.org/mozilla-central/rev/44d3202f73e0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 788698
You need to log in before you can comment on or make changes to this bug.