522393 - Replacing "&ApplyFunction;" with "x" in <mathml:mo> does not work

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Jesse Ruderman]

Based on layout/mathml/crashtests/420420-1.xhtml

Comment 3

[Karl Tomlinson (:karlt back Feb 10)]

Just guessing: this might be a similar problem for all nsMathMLmoFrames where UseMathMLChar() returns true.

Comment 4

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

(In reply to comment #3)
> Just guessing: this might be a similar problem for all nsMathMLmoFrames where
> UseMathMLChar() returns true.

I rather suspect to happen only when replacing invisible characters, that are treated differently:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.cpp#943

I was only able to reproduce this bug with invisible-times.
Note that the x is well displayed after zooming.

Comment 5

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

The code contains the following comment:

>  play safe by not passing invisible operators to the font subsystem because
>  some platforms risk selecting strange glyphs for them and give bad inter-space

But is it still the case with recent fonts? what about removing this special handling for invisible operators?

Comment 6

[Karl Tomlinson (:karlt back Feb 10)]

(In reply to Frédéric Wang (:fredw) from comment #5)
> The code contains the following comment:
> 
> >  play safe by not passing invisible operators to the font subsystem because
> >  some platforms risk selecting strange glyphs for them and give bad inter-space
> 
> But is it still the case with recent fonts? what about removing this special
> handling for invisible operators?

If that is the only reason, then perhaps we can try removing the special handling.

Do most platforms have fonts installed by default that support these characters (via blank glyphs)?

If the fonts (perhaps correctly) include an advance (space) for the characters will that add to any space provided for the operator by the code?

Comment 7

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

> Do most platforms have fonts installed by default that support these
> characters (via blank glyphs)?

My guess is that now with Unicode fonts, these 3 invisible operators are now available.

> 
> If the fonts (perhaps correctly) include an advance (space) for the
> characters will that add to any space provided for the operator by the code?

The operator dictionary defines lspace = rspace = 0 for the three invisible operators, that's the comment I added here:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.cpp#347

That seems to be OK if the fonts define their own width.



I'll ask more info on the MathML mailing list later. Someone may know better the situation with these invisible operators...

Comment 8

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

Actually, I think I'll just cc' David Carlisle to this bug.

@David: What's your opinion on comment 6?

Comment 9

[David Carlisle]

(In reply to Frédéric Wang (:fredw) from comment #8)
> Actually, I think I'll just cc' David Carlisle to this bug.
> 
> @David: What's your opinion on comment 6?

I think if you are now basically assuming unicode layout it should be OK to treat the invisible characters as normal characters that just happen to be invisible. (That certainly wasn't the case when they were initially proposed).

Comment 10

[Mohit Sinha]

Attachment: patch

I have removed the special handling for invisible operators.
If it is correct I will do it for the rest of the file.

Comment 11

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

Comment on attachment 662077 [details] [diff] [review]
patch

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

Please also remove the comment above your change. Note that there are other places outside this file to remove too.

Comment 12

[Mohit Sinha]

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.cpp#43
Can I just edit it to  'return  eMathMLFrameType_OperatorOrdinary; '  ?

Comment 13

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

(In reply to Mohit Sinha from comment #12)
> http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.
> cpp#43
> Can I just edit it to  'return  eMathMLFrameType_OperatorOrdinary; '  ?

Yes, that sounds the right thing to do (and as a consequence, remove where eMathMLFrameType_OperatorInvisible is used and defined).

Comment 14

[Mohit Sinha]

Attachment: patch

I have removed the special handling for the invisible operators.

Comment 15

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

Comment on attachment 663242 [details] [diff] [review]
patch

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

::: layout/mathml/nsMathMLContainerFrame.cpp
@@ -1114,5 @@
> -    // encounter a visible frame
> -    if (*aFromFrameType == eMathMLFrameType_UNKNOWN) {
> -      *aFromFrameType = firstType;
> -      *aCarrySpace = space;
> -    }

I think this change won't compile.

But more importantly, I'm wondering if we should not keep this special handling. Can you try to with test pages containing invisible operators and see what is changed?

Karl: what do you think about this?

Comment 16

[Karl Tomlinson (:karlt back Feb 10)]

Yes, looks like this is a reason to keep eMathMLFrameType_OperatorInvisible, or the inter-space could be added twice or not at all in some situations.

Comment 17

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

Comment on attachment 663242 [details] [diff] [review]
patch

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

I think you can keep your change in nsMathMLmoFrame::UseMathMLChar ; the second one in nsMathMLmoFrame::ProcessOperatorData about the computation of lspace, rspace  ; the one in nsMathMLmoFrame::Stretch and the one in nsMathMLmoFrame::Reflow. Verify that horizontal alignment of the source code is correct after your changes, that your patch does fix the bug and that invisible operators remain invisible after your changes. You can write a reftest comparing a page with invisible <mo>'s against the same page without these operators. They should not influence spacing between other MathML elements.

https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests

You can also write a reftest comparing the dynamic and static pages attached to this bug. See layout/reftests/mathml/quotes-1.xhtml for an example of a dynamic page (note the class="reftest-wait" attribute and MozReftestInvalidate listener).

Comment 18

[Mohit Sinha]

Attachment: patch

I have made the necessary changes but kept the change mentioned in comment 13 above.

Comment 19

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

Comment on attachment 664755 [details] [diff] [review]
patch

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

::: layout/mathml/nsMathMLmoFrame.cpp
@@ +44,1 @@
>  }

I told you to remove this in comment 13 when I thought we could get rid of the invisible operator flag. But now I think you should not do this change. Sorry for the confusion.

@@ +214,5 @@
>    // Also remember the other special bits that we want to carry forward.
>    mFlags &= NS_MATHML_OPERATOR_MUTABLE |
>              NS_MATHML_OPERATOR_ACCENT | 
>              NS_MATHML_OPERATOR_MOVABLELIMITS |
> +            NS_MATHML_OPERATOR_CENTERED;

I don't think that this one should be changed. Otherwise that will clear the invisible operator flag that we still use for the frame spacing.

@@ +344,5 @@
>        // lookup the operator dictionary
>        nsAutoString data;
>        mMathMLChar.GetData(data);
>        nsMathMLOperators::LookupOperator(data, form, &mFlags, &lspace, &rspace);
> +    

Please decrease the indentation level of this block.

@@ +754,5 @@
>      nsresult rv = Place(aRenderingContext, true, aDesiredStretchSize);
>      if (NS_MATHML_HAS_ERROR(mPresentationData.flags) || NS_FAILED(rv)) {
>        // Make sure the child frames get their DidReflow() calls.
>        DidReflowChildren(mFrames.FirstChild());
>      }

Please decrease the indentation level of this block.

Comment 20

[Mohit Sinha]

Attachment: patch

Comment 21

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

Comment on attachment 666434 [details] [diff] [review]
patch

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

Thanks Mohit. Please tell me about what you have done regarding the verification/tests mentioned in comment 17.

::: layout/mathml/nsMathMLmoFrame.cpp
@@ +216,5 @@
>    // Also remember the other special bits that we want to carry forward.
>    mFlags &= NS_MATHML_OPERATOR_MUTABLE |
>              NS_MATHML_OPERATOR_ACCENT | 
>              NS_MATHML_OPERATOR_MOVABLELIMITS |
> +            NS_MATHML_OPERATOR_CENTERED;

As I said in comment 19, removing this is likely to clear the INVISIBLE flag so that's not what we want.

Comment 22

[Mohit Sinha]

I made the necessary changes and tried the dynamic testcase above.
'X' was clearly visible.

Comment 23

[Mohit Sinha]

Attachment: patch

Comment 24

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

(In reply to Mohit Sinha from comment #22)
> I made the necessary changes and tried the dynamic testcase above.
> 'X' was clearly visible.

Thanks, that's great! I believe you know how to write a == reftest comparing the dynamic and static testcases. This test will ensure that the bug is fixed.

We also want to be sure that we don't introduce regressions. So an == reftest like

  <mi>sin</mi><mo>&#x2061;</mo><mi>x</mi>

against

  <mi>sin</mi><mi>x</mi>
 
will, I think, allow to verify that the invisible operator does not change the spacing around it and that it is indeed invisible. You can do this for the four invisible operators and use the examples here:

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

As usual, these tests can be done in a separate patch and if you need more help, just ask.

Comment 25

[Mohit Sinha]

Attachment: reftests patch

Comment 26

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

Your test verifies the cases of

&#x2061;<!--FUNCTION APPLICATION-->
&#x2062;<!--INVISIBLE TIMES-->
&#x2063;<!--INVISIBLE SEPARATOR-->
&#x2064;<!-- INVISIBLE PLUS -->

while nsMathMLmoFrame::ProcessTextData() only consider

static const PRUnichar kInvisibleComma = PRUnichar(0x200B); // a.k.a. ZERO WIDTH SPACE
static const PRUnichar kApplyFunction  = PRUnichar(0x2061);
static const PRUnichar kInvisibleTimes = PRUnichar(0x2062);

I don't know where 0x200B has been taken from, but perhaps you should write a simple patch for nsMathMLmoFrame that adds

&#x2063;<!--INVISIBLE SEPARATOR-->
&#x2064;<!-- INVISIBLE PLUS -->

to the list of invisible operators. Currently, the two pages of the reftest do not match.

You also told me that your patch for this bug made the spacing worse, did you find the problem? As I said by mail, you will have to study precisely the MathML code to understand where the problem comes from (look at the parts modified in your second patch especially GetInterFrameSpacing and nsMathMLmoFrame::ProcessOperatorData).

Comment 27

[Mohit Sinha]

Attachment: patch

Comment 28

[Mohit Sinha]

Attachment: patch

Comment 29

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

Comment on attachment 671203 [details] [diff] [review]
patch

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

The two new changes look good, but can you please provide them in a separate patch? That will be more convenient to check that they make the reftest pass.

Comment 30

[Mohit Sinha]

Attachment: patch for defining kInvisibleSeparator(2063) & kInvisiblePlus(2064)

I have made the new patch as suggested in comment 26.

Comment 31

[Mohit Sinha]

Attachment: patch

Comment 32

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

Comment on attachment 671216 [details] [diff] [review]
patch for defining kInvisibleSeparator(2063) & kInvisiblePlus(2064)

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

I got failure when I tried to apply the patch. I would expect it to apply before the other patch of this bug. Otherwise, that looks good. Do not forget to update the commit message.

Comment 33

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

Comment on attachment 671163 [details] [diff] [review]
reftests patch

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

The test seems to pass with attachment 671216 [details] [diff] [review], except with the MathJax fonts (but we have similar issues with these fonts anyway). Please update reftest.list and check that the HTML pages validate with http://validator.w3.org/nu/.

Also, this

"diff --git a/patch.diff b/patch.diff"

at the end of the patch is weird and should be removed. Again, please do not forget to update the commit message

https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

Comment 34

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

> I would expect it to apply before the other patch of this bug.

I mean, the order of the patch should probably be

1) the reftests patch + attachment 671216 [details] [diff] [review] (the tests should pass modulo additional invisible ops)

before

2) the patch that fixes this bug (to check that there is no regression)

Comment 35

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

I believe Mohit is no longer working on this, so I have moved his changes to bug 969867.

The remaining work is:

1) Write reftests

  See https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests for general information for how to write and run the tests.
  The idea for the reftests would be to compare a page that is modified via Javascript against a static reference. See for example

  http://mxr.mozilla.org/mozilla-central/source/layout/reftests/mathml/displaystyle-4.html?force=1
  http://mxr.mozilla.org/mozilla-central/source/layout/reftests/mathml/displaystyle-4-ref.html?force=1

  You can use Jesse's testcase and generalize that to other invisible operators. 

2) Study layout/mathml/nsMathMLmoFrame.cpp and try to see how to force the update. A quick debug on Jesse's testcase shows that:

  - nsMathMLmoFrame::MarkIntrinsicWidthsDirty is called and the new data "x" is taken into account in ProcessTextData()
  - however, nsMathMLmoFrame::Reflow is not called so the rendering is not updated.

  I'm suspecting the solution would be to mark the right frame dirty but this requires further analysis.

Comment 36

[James Kitchener (:jkitch)]

Attachment: attempt 2

Actually it turned out to be the opposite problem.  Frames marked dirty when they should not have been.

The reflow of children of <mo> frames with invisible operators are never placed or finalised, so the dirty flags are set and left unchanged for the remainder of the reflow.

When the textnode is changed to a visible character, the dirty state starts to propagate up the frame tree but stops at the frame with dirty bits set from the previous reflow.  (This occurs due to an optimisation that assumes all ancestors of dirty frames are dirty.  The assertion that validates this assumption was explicitly disabled due to a bug).

The attached patch clears the dirty state of all children and calls Stretch for invisible operators within nsMathMLmoFrame::Reflow().  (The comments from that function suggest stretch should be called, but it seems to only be called from the codepath that invisible operators bypass.

Comment 37

[Jesse Ruderman]

Is there a bug on re-enabling the dirty bit assertion?

Comment 38

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

Comment on attachment 8388278 [details] [diff] [review]
attempt 2

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

That looks fine, but I wonder if we could try to make invisible operators follow the standard path to avoid this kind of unexpected layout behavior.

I believe invisible operators are treated differently because:

1) Some fonts do not contain these characters and so they will render with a replacement glyph instead of being invisible.
2) Even when the fonts contain the characters, they do not necessarily have zero width.

I think 1) can be ignored if we move to MATH fonts, but 2) is still true AFAIK.

For 2), perhaps we could just set the preferred widths and metrics to zero.
For 1), I wonder whether we could do something specific in nsMathMLChar so that nothing is painted.

Comment 39

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

Attachment: testcase.html

more test case

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Frédéric Wang (:fredw) from comment #38)
> That looks fine, but I wonder if we could try to make invisible operators
> follow the standard path to avoid this kind of unexpected layout behavior.
> 
> I believe invisible operators are treated differently because:
> 
> 1) Some fonts do not contain these characters and so they will render with a
> replacement glyph instead of being invisible.
> 2) Even when the fonts contain the characters, they do not necessarily have
> zero width.
> 
> I think 1) can be ignored if we move to MATH fonts, but 2) is still true
> AFAIK.

Are these problems hypothetical or real?

Comment 41

[James Kitchener (:jkitch)]

(In reply to Jesse Ruderman from comment #37)
> Is there a bug on re-enabling the dirty bit assertion?

I couldn't find an existing one, so bug 981536 was filed.

Comment 42

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Are these problems hypothetical or real?

They were certainly true when the specific code was added, since there is a comment "play safe by not passing invisible operators to the font subsystem because some platforms risk selecting strange glyphs for them and give bad inter-space". 1) depends on the fonts installed on the system, but should probably not happen by default on Desktop (Mac has STIX, Windows has Cambria Math and Linux has DejaVu). For 2) something like <p>_&#x2062;_</p> shows a space in WebKit but not in Gecko, so I'm not really sure if that's still relevant.

Anyway, we still need the invisible op flag to avoid adding inter frame spacing twice in expressions like <mi>x</mi><mo>&#x2062;</mo><mi>y</mi>.

Comment 43

[Khaled Hosny]

This shouldn’t be an issue with HarfBuzz as it ignores font glyphs for default ignorable characters and just render a zero width space for them. Since HarfBuzz is now used by default for OpenType in all platforms, I think the special MathML code can go (I don’t think anyone is going to use AAT fonts for math, but even then it is likely that Core Text does something similar).

Comment 44

[James Kitchener (:jkitch)]

Attachment: attempt 3 - remove reflow special case

On the basis of comment 43 that Harfbuzz handles invisible operators appropriately, the attached patch removes the special casing for such operators within reflow, but leaves the other instances of special behaviour in place.

The two attempts are mutually exclusive, so you will need to decide which one is preferable.

https://tbpl.mozilla.org/?tree=Try&rev=7ee9016995df
I've sent this through a complete try run and there do not appear to be any failures attributable to it.

Comment 45

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

Comment on attachment 8391843 [details] [diff] [review]
attempt 3 - remove reflow special case

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

OK, that looks better.

I think we go even further:

- We can remove INVISIBLE from nsMathMLmoFrame::UseMathMLChar(), so that the invisible operator will be "drawn" by the normal text layout (actually, I'm not sure why this was necessary, since the drawing was skipped anyway)

- We can remove the early return from nsMathMLmoFrame::ProcessTextData. Then the spacing will be read from the operator dictionary and we no longer need the special case in nsMathMLmoFrame::ProcessOperatorData.

Then the invisible operator property will only be used for the special case in GetInterFrameSpacing.

Comment 46

[James Kitchener (:jkitch)]

Although attempt 3 by itself works, incorporating the changes from comment 45 broke mo-invisibleoperators.html.

It appears that although the invisible characters have 0 width, they do not have 0 height.  Characters ⁡ through ⁤ have a height greater than that of the characters '1'-'5', changing the dimensions of the encapsulating mrow.

I suppose I could add a check in nsMathMLTokenFrame::Reflow() to zero out the appropriate metrics for invisible operators, but all this accomplishes is replacing one special case with another.

Comment 47

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

(In reply to James Kitchener (:jkitch) from comment #46)
> It appears that although the invisible characters have 0 width, they do not
> have 0 height.  Characters ⁡ through ⁤ have a height greater
> than that of the characters '1'-'5', changing the dimensions of the
> encapsulating mrow.

I'm not sure, but perhaps this height is added on purpose by the font designer, for example to get similar line height than with equivalent "visible" operators.

If so, an easy way to fix the mo-invisibleoperators.html reftest is to add an <mspace height="3em" depth="3em"/> (with height/depth large enough) so that the height of invisible operators do not affect the height of the <math> element.

Comment 48

[Khaled Hosny]

Glyphs do not have heights in OpenType/TrueType fonts (only in fonts built for vertical typesetting, but then it is just the vertical advance width). So the source of those heights should be somewhere in Gecko.

Comment 49

[James Kitchener (:jkitch)]

Attachment: attempt 3 - remove reflow special case

It turns out that MathJax_Main-Regular (or at least the version that I have installed) does not actually define glyphs for the invisible operators, so other fonts with different heights are used. (In my case Cambria and Segoe UI Symbol).

The attached patch follows the suggestion in comment 47 and will work providing the fallback fonts have a height less than 2em.  It also adds 0x200B/invisible comma/zero width space to the operator dictionary.   Please double check this - I'm not sure if any optional attributes (eg separator) are required for it.

Comment 50

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

(In reply to James Kitchener (:jkitch) from comment #49)
> It turns out that MathJax_Main-Regular (or at least the version that I have
> installed) does not actually define glyphs for the invisible operators, so
> other fonts with different heights are used. (In my case Cambria and Segoe
> UI Symbol).
> 
> The attached patch follows the suggestion in comment 47 and will work
> providing the fallback fonts have a height less than 2em.

Thanks, that makes sense. That's probably a minor problem, so we can take the patch as it. I expect we could drop the MathJax fonts in bug 947654 and only use MATH fonts that have these invisible operators.

>  It also adds
> 0x200B/invisible comma/zero width space to the operator dictionary.   Please
> double check this - I'm not sure if any optional attributes (eg separator)
> are required for it.

separator would make sense for an invisible comma, but anyway it does not have visual effect and our accessibility code does not use the op dict either (actually the accessibility code does not understand MathML at all).

However, I think this U+200B is actually "ZERO WIDTH SPACE" and should not be used as an <mo> operator. It is not in the "official" MathML operator dictionary. The appropriate character for "invisible comma" seems to be U+2063;. So I'd suggest to just remove U+200B from the list of invisible operators.

Comment 51

[James Kitchener (:jkitch)]

Attachment: attempt 3 - remove reflow special case

Zero width space is no longer an considered an invisible operator (and the operator dictionary change has been reverted).

Comment 52

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

Comment on attachment 8392829 [details] [diff] [review]
attempt 3 - remove reflow special case

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

Thank you.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76f6f3d6d8e3

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/92324150c47e

https://tbpl.mozilla.org/php/getParsedLog.php?id=36400677&tree=Mozilla-Inbound

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Good times, the bustage was actually from bug 950076 which was in the same push as this. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f1c40b476d

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9f1c40b476d

Comment 57

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2009-10-15 using the testcases.
Verified fixed 31.0a1 (2014-03-24), Win 7 x64.