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

RESOLVED FIXED in mozilla31

Status

()

Core
MathML
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: fredw, Assigned: jkitch)

Tracking

(Blocks: 1 bug)

Trunk
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 755792 [details]
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.
(Reporter)

Comment 1

3 years ago
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)

Updated

3 years ago
Assignee: nobody → jkitch.bug
(Assignee)

Updated

3 years ago
Depends on: 947557
(Assignee)

Comment 2

3 years ago
Created attachment 8400568 [details] [diff] [review]
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.)
Attachment #8400568 - Flags: review?(fred.wang)
(Assignee)

Comment 3

3 years ago
Created attachment 8400571 [details] [diff] [review]
Part 2: tests

Static and dynamic tests
Attachment #8400571 - Flags: review?(fred.wang)
(Assignee)

Comment 4

3 years ago
Created attachment 8400572 [details] [diff] [review]
Part 1: MathML frame changes

whitespace fix.
Attachment #8400568 - Attachment is obsolete: true
Attachment #8400568 - Flags: review?(fred.wang)
Attachment #8400572 - Flags: review?(fred.wang)
(Reporter)

Comment 5

3 years ago
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+
(Reporter)

Comment 6

3 years ago
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+
(Assignee)

Comment 7

3 years ago
Created attachment 8402129 [details] [diff] [review]
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.
Attachment #8400572 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8402130 [details] [diff] [review]
Part 2: tests
Attachment #8400571 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 557478
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
landed the patches on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05826bb1b9a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/86bb1d3444bf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05826bb1b9a9
Status: NEW → RESOLVED
Last Resolved: 3 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.