Last Comment Bug 717546 - Implement mspace with negative width
: Implement mspace with negative width
Status: RESOLVED FIXED
: dev-doc-complete, helpwanted
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla23
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
: 656429 (view as bug list)
Depends on: 433064 297464 716349
Blocks: mathml-in-mathjax
  Show dependency treegraph
 
Reported: 2012-01-12 02:28 PST by Frédéric Wang (:fredw)
Modified: 2013-04-26 11:27 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial support for negative mspace@width (3.15 KB, patch)
2012-11-29 11:32 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
testcase (506 bytes, text/html)
2012-11-29 11:35 PST, Frédéric Wang (:fredw)
no flags Details
Reftests (3.05 KB, patch)
2012-11-29 11:55 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Reftest (3.09 KB, patch)
2013-04-06 08:38 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch V2 (3.14 KB, patch)
2013-04-06 08:39 PDT, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
Patch V3 (2.05 KB, patch)
2013-04-12 06:33 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch Final Version (2.91 KB, patch)
2013-04-16 22:27 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2012-01-12 02:28:14 PST
Follow-up of bug 716349.
Comment 1 Frédéric Wang (:fredw) 2012-05-06 04:53:44 PDT
*** Bug 656429 has been marked as a duplicate of this bug. ***
Comment 2 Frédéric Wang (:fredw) 2012-11-29 11:32:46 PST
Created attachment 686709 [details] [diff] [review]
Partial support for negative mspace@width

A patch partially implementing negative spaces. It only enables negative spacing via mspace (not mpadded or mo) and within mrow-like elements (not math or mtd). A negative space moving a child to a negative X coordinate (I don't think that happens in practice) is treated as if the child was moved to 0.

I don't plan to do something more general at the moment. My goal is just to support MathJax's use cases where negative spaces are used to make a child closer to its previous sibling. MathJax always adds an <mrow> so no need to consider the cases of math & mtd, which seem more involved.
Comment 3 Frédéric Wang (:fredw) 2012-11-29 11:35:42 PST
Created attachment 686711 [details]
testcase
Comment 4 Frédéric Wang (:fredw) 2012-11-29 11:55:16 PST
Created attachment 686725 [details] [diff] [review]
Reftests
Comment 5 Frédéric Wang (:fredw) 2012-11-30 07:30:15 PST
If later we want to implement negative width for <mspace/> children of <math> or <mtd>, we should probably move the work of nsMathMLContainerFrame::FixInterFrameSpacing to Place before. Making this dependent on bug 433064.
Comment 6 Frédéric Wang (:fredw) 2012-11-30 08:25:37 PST
Mass change: setting priority to 3 for bugs preventing Gecko's Native MathML to be enabled by default in MathJax.
Comment 7 Bill Gianopoulos [:WG9s] 2013-01-16 12:21:22 PST
Comment on attachment 686709 [details] [diff] [review]
Partial support for negative mspace@width


>     mX += space * GetThinSpace(font);
>     return *this;
>   }
> 
>   nsIFrame* Frame() const { return mChildFrame; }
>-  nscoord X() const { return mX; }
>+  nscoord X() const { return NS_MAX(0, mX); }

NS_MAX needs to be changed to std::max here.

>   const nsHTMLReflowMetrics& ReflowMetrics() const { return mSize; }
>   nscoord Ascent() const { return mSize.ascent; }
>   nscoord Descent() const { return mSize.height - mSize.ascent; }
>   const nsBoundingMetrics& BoundingMetrics() const {
>     return mSize.mBoundingMetrics;
>   }
> 
> private:
>diff --git a/layout/mathml/nsMathMLmspaceFrame.cpp b/layout/mathml/nsMathMLmspaceFrame.cpp
>--- a/layout/mathml/nsMathMLmspaceFrame.cpp
>+++ b/layout/mathml/nsMathMLmspaceFrame.cpp
>@@ -55,16 +55,17 @@ nsMathMLmspaceFrame::ProcessAttributes(n
>   mWidth = 0;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::width,
>                value);
>   if (!value.IsEmpty()) {
>     ParseNumericValue(value, &mWidth,
>                       nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>                       aPresContext, mStyleContext);
>   }
>+  mPresentationData.negativeLeadingSpace = NS_MAX(0, -mWidth);


and here (see bug Bug 786533)
Comment 8 Frédéric Wang (:fredw) 2013-04-06 08:38:30 PDT
Created attachment 734252 [details] [diff] [review]
Reftest
Comment 9 Frédéric Wang (:fredw) 2013-04-06 08:39:48 PDT
Created attachment 734253 [details] [diff] [review]
Patch V2
Comment 10 Frédéric Wang (:fredw) 2013-04-06 08:43:45 PDT
I just unbitrot the patches. This still allows to address MathJax's use cases, to pass the reftest as well as the MathML Acid2 test:

http://fred-wang.github.io/AcidTestsMathML/acid2/
Comment 11 Karl Tomlinson (:karlt) 2013-04-09 15:57:33 PDT
Comment on attachment 734253 [details] [diff] [review]
Patch V2

>+  // negative leading space of the frame.
>+  nscoord negativeLeadingSpace;

Please adjust the comment here to clarify that this is a positive value when
the offset is negative.

>     nscoord space =
>       GetInterFrameSpacing(font->mScriptLevel,
>                            prevFrameType, mChildFrameType,
>                            &mFromFrameType, &mCarrySpace);
>+
>+    nsIMathMLFrame* mathMLFrame = do_QueryFrame(mChildFrame);
>+    if (mathMLFrame) {
>+      nsPresentationData presentationData;
>+      mathMLFrame->GetPresentationData(presentationData);
>+      mX -= presentationData.negativeLeadingSpace;
>+    }
>     mX += space * GetThinSpace(font);
>     return *this;

Please leave the "mX += space * GetThinSpace(font);" line adjacent to the code
that initializes "space".

>-  nscoord X() const { return mX; }
>+  nscoord X() const { return std::max(0, mX); 

Imagine:

<mrow>
  <mspace width="-1em"/>
  <mspace width="1em" mathbackground="red"/>
  <mspace width="1em" mathbackground="blue"/>

I think there is an effect here that the position of the blue element is
offset, but the position of the red element is not, which is probably not
expected.

Another situation is:

<mrow>
  <mspace width="2em" mathbackground="red"/>
  <mrow>
    <mspace> width="-1em"/>
    <mspace width="1em" mathbackground="blue"/>
  </mrow>
</mrow>

I'd expect width="-1em" to be effective here, which requires some propagation
of the negative space to the mrow.

Does anything go wrong if X() returns a negative nscoord?

Perhaps it is only aDesiredSize.width that need be non-negative?

Can mBoundingMetrics.width be used instead of
nsPresentationData::negativeLeadingSpace to store negative offsets?
Looks like nsBoundingMetrics::width is really an advance rather than a width (and perhaps would be better named "advance").
I expect aDesiredSize.width would still need to be non-negative.
Comment 12 Karl Tomlinson (:karlt) 2013-04-09 16:00:46 PDT
The comment "// XXXfredw Negative spaces are not implemented. See bug 717546" can be removed when this is fixed.
Comment 13 Frédéric Wang (:fredw) 2013-04-09 23:26:12 PDT
Yes, I considered the two cases you mention. I think that can get even more complicated in situations like

<mrow>
  <mspace width="2em" mathbackground="red"/>
  <mrow>
    <mspace width="1em"/>
    <mspace width="1em"/>
    <mspace width="-4em"/>
    <mspace width="2em" mathbackground="blue"/>
  </mrow>
</mrow>

where the blue rectangle should be over the red rectangle or when you combine that with several nested mrow's. I think I tried to propagate the negative space at some point but that didn't work very well. As I said in comment 2, my plan was really to consider the cases that happen in practice. Negative space are generally just used to adjust distance between two elements so basically you have

X <mspace width="-.3em"/> Y

where Y is never moves farther than X. Hence that seems a bit overkill to implement the whole negative spaces for MathJax purpose only (there would be other cases to consider like negative mpadded, negative mo@lspace/mo@rspace, how to handle the topmost math/mtd etc).

I'll try to reconsider this. Using negative mBoundingMetrics.width might help, but I need to check that we don't assume in some place that it is nonnegative. That could lead to more issues like bug 567718.

I'm still wondering if bug 433064 should be fixed before (Actually, I now agree adding an anonymous mrow frame as a child of the <math> element would solve many issues and make the code cleaner and more consistent but line breaking should probably be reimplemented before too, which will take some work).
Comment 14 Frédéric Wang (:fredw) 2013-04-12 06:33:08 PDT
Created attachment 736785 [details] [diff] [review]
Patch V3
Comment 15 Frédéric Wang (:fredw) 2013-04-12 06:33:47 PDT
Comment on attachment 734253 [details] [diff] [review]
Patch V2

https://tbpl.mozilla.org/?tree=Try&rev=e8bcce0a2c2d
Comment 16 Karl Tomlinson (:karlt) 2013-04-16 20:37:36 PDT
Comment on attachment 736785 [details] [diff] [review]
Patch V3

Now that mRowChildFrameIterator::X() can return -ve,
nsMathMLContainerFrame::Place() needs to ensure that aDesiredSize.width is
non-negative, or there will be assertions like bug 716349 when an mspace is the single child of an mrow.  r+ with a std::max(0, mBoundingMetrics.width) to address that.

Perhaps nsMathMLmspaceFrame::Reflow() should be ensuring that rightBearing >=
leftBearing, which would require that GetItalicCorrection handle empty metrics
specially, but we can see how this works out with rightBearing < leftBearing.
Comment 17 Frédéric Wang (:fredw) 2013-04-16 22:27:51 PDT
Created attachment 738326 [details] [diff] [review]
Patch Final Version

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