Closed Bug 877563 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: fredw, Assigned: jkitch)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 3 obsolete files)

Attached file 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.
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.
Assignee: nobody → jkitch.bug
Depends on: 947557
Attached patch Part 1: MathML frame changes (obsolete) — Splinter Review
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.)
Attachment #8400568 - Flags: review?(fred.wang)
Attached patch Part 2: tests (obsolete) — Splinter Review
Static and dynamic tests
Attachment #8400571 - Flags: review?(fred.wang)
Attached patch Part 1: MathML frame changes (obsolete) — Splinter Review
whitespace fix.
Attachment #8400568 - Attachment is obsolete: true
Attachment #8400568 - Flags: review?(fred.wang)
Attachment #8400572 - Flags: review?(fred.wang)
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.
Attachment #8400572 - Flags: review?(fred.wang) → review+
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?
Attachment #8400571 - Flags: review?(fred.wang) → review+
nsMathMLmathBlockFrame needs a separate call to do_QueryFrame, as allowing it to cast to a nsIMathMLFrame seems to break the <semantics> tag.
Attachment #8400572 - Attachment is obsolete: true
Attached patch Part 2: testsSplinter Review
Attachment #8400571 - Attachment is obsolete: true
Blocks: mrow-1-arg
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05826bb1b9a9
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.