877563 - lspace/rspace should only have effect on embellished operators inside (inferred) mrow

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

Attachment: testcase

"The amount of horizontal space added around an operator (or embellished operator), when it occurs in an mrow, can be directly specified by the lspace and rspace attributes."

"Some renderers may choose to use no space around most operators appearing within subscripts or superscripts, as is done in TEX."

Currently, we add lspace/rspace around any embellished operator but that should probably be restricted to (inferred) mrow or at least that should not happen in scripts.

A concrete use case is chemistry where + and - and used in exponents to represent ions rather than binary operators. See the testcase.

Comment 1

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

A solution for this could be clear leadingSpace/trailingSpace in 

https://hg.mozilla.org/mozilla-central/file/923f1411f42f/layout/mathml/nsMathMLmoFrame.cpp#l846

when the parent is an mrow-like element. See bug 562460 for a suggestion about such IsMrowLike() function.

Comment 2

[James Kitchener (:jkitch)]

Attachment: Part 1: MathML frame changes

It turns out there already was some code to handle the case in the description, it just wasn't working.  The NS_MATHML_OPERATOR_EMBELLISH_ISOLATED flag is now set when the embellished operator is not within an (inferred) mrow, as determined by IsMrowLike.

The spot just after the operator dictionary lookup was chosen to allow user specified values for lspace/rspace to still work (at the very least it makes things easier to test).

(I'm aware that the check for 0 children in IsMrowLike is redundant given the method iscalled by a child frame, but is there in case we decide to call it elsewhere.)

Comment 3

[James Kitchener (:jkitch)]

Attachment: Part 2: tests

Static and dynamic tests

Comment 4

[James Kitchener (:jkitch)]

Attachment: Part 1: MathML frame changes

whitespace fix.

Comment 5

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

Comment on attachment 8400572 [details] [diff] [review]
Part 1: MathML frame changes

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

mpadded seems to be missing. Think merror and mstyle produces and nsMathMLmrowFrame?

http://www.w3.org/TR/MathML3/chapter3.html#presm.inferredmrow

::: layout/mathml/nsMathMLContainerFrame.h
@@ +544,5 @@
> +  bool
> +  IsMrowLike() MOZ_OVERRIDE {
> +    return mFrames.FirstChild() != mFrames.LastChild() ||
> +           !mFrames.FirstChild();
> +  }

I think you should also do that for nsMathMLmathBlockFrame and test

<math display="block">....</math>

::: layout/mathml/nsMathMLmfencedFrame.h
@@ +82,5 @@
>  
> +  virtual bool
> +  IsMrowLike() MOZ_OVERRIDE
> +  {
> +    // always treated as an mrow with > 1 child

Can you add a comment that it is because of the equivalence form http://www.w3.org/TR/MathML3/chapter3.html#presm.mfenced

where we will have

<mrow> <mo>open</mo> <mrow>...</mrow> <mo>close</mo> </mrow>

::: layout/mathml/nsMathMLmoFrame.cpp
@@ +366,5 @@
>        // Our fonts can be anything, so...)
> +      if (StyleFont()->mScriptLevel > 0 &&
> +          !NS_MATHML_OPERATOR_HAS_EMBELLISH_ANCESTOR(mFlags)) {
> +        mEmbellishData.leadingSpace /= 2;
> +        mEmbellishData.trailingSpace /= 2;

I would say we should remove this tuning to follow the MathML spec, but let's keep it for now.

Comment 6

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

Comment on attachment 8400571 [details] [diff] [review]
Part 2: tests

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

Thanks.

Could you add test for merror/mpadded and <math display="block"> too?

Comment 7

[James Kitchener (:jkitch)]

Attachment: Part 1: MathML frame changes

nsMathMLmathBlockFrame needs a separate call to do_QueryFrame, as allowing it to cast to a nsIMathMLFrame seems to break the <semantics> tag.

Comment 8

[James Kitchener (:jkitch)]

Attachment: Part 2: tests

Comment 9

[Carsten Book [:Tomcat]]

landed the patches on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05826bb1b9a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/86bb1d3444bf

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05826bb1b9a9