838506 - Refactor implementation of displaystyle using a -moz-display-style property

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

Descriptions from the MathML spec:

http://www.w3.org/TR/MathML/chapter3.html#presm.scriptlevel
http://www.w3.org/TR/MathML/chapter2.html#interf.toplevel

The work here is essentially the same as in bug 764996, except that one has to add a -moz-display-style CSS property.

* In layout/style/*, add all the stuff to implement the -moz-display-style property. attachment 295888 [details] [diff] [review] might help, but here we just want a boolean property so that can be simpler. Ask to CSS folks on IRC for more details.

* In layout/mathml/*
  replace all the references to displaystyle (or DISPLAYSTYLE etc) to use -moz-display-style instead. (attachment 692250 [details] [diff] [review] might be helpful)

* In content/mathml/content/src/nsMathMLElement.cpp 
  map math@displaystyle & mstyle@displaystyle to -moz-display-style (attachment 691943 [details] [diff] [review] might be helpful)

* In layout/mathml/mathml.css
  add a rule math[display="block"] { -moz-display-style: true }

Rationale:
- http://lists.w3.org/Archives/Public/www-math/2012Sep/0002.html
- https://wiki.mozilla.org/MathML:mstyle
- and in particular, this will hopefully fix 832800.

Comment 1

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

In layout/mathml/mathml.css, I guess -moz-display-style should be reset to false on the math element,  unless it is math[display="block"].

Comment 2

[Bablu Kumar]

I want to work on this bug.I am well acquainted with the knowledge of c/c++.I think it this first bug would be a great opportunity for me.

Comment 3

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

Looking at Bablu Kumar's Bugzilla activity, he commented on several bugs during one week of March, offering to work on them but never got time to work on them. So I guess I can take this bug.

Comment 4

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

Attachment: Part 1 - CSS

Comment 5

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

Attachment: Part 2 - CSS mapping

Comment 6

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

Attachment: Part 3 - MathML frames

Comment 7

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

Attachment: Part 4 - cleanup

Comment 8

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

Attachment: Part 5 - tests

Comment 9

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

The patches fix bug 713606 and bug 832800, issues with displaystyle on math & mtable and also allows to compute script-level on mfrac with pure CSS rules. However, an assertion shows up in the new test displaystyle-3.html, so I'll have to analyze why.

Comment 10

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

So the new assertion mentioned in comment 9 is in nsStyleChangeList::AppendChange with "Shouldn't be trying to restyle non-elements directly". It seems that this ends up being called with the text frames of token MathML elements.

There is an XXX comment from Boris:

// XXXbz we should make this take Element instead of nsIContent

I wonder if fixing this would help here. However, I'm not sure at all why this restyle is called on the text frames. Since script level might be involved here, I wonder if that's related to this other comment from Robert:

// XXXroc this does a ContentStatesChanged, is it safe to call here? If
// not we should do it in a post-reflow callback.

Can you give me more hints about how to try to do these changes so that I see if that solves my problem here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> However, I'm not sure at all why this restyle is called on the text frames.

That's the real question, yes.

I don't even see any changes to nsStyleFont::CalcDifference to take the new member into account, so why would we think a restyle is needed on those text frames?

Comment 12

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

(In reply to Boris Zbarsky [:bz] from comment #11)
> > However, I'm not sure at all why this restyle is called on the text frames.
> 
> That's the real question, yes.
> 
> I don't even see any changes to nsStyleFont::CalcDifference to take the new
> member into account, so why would we think a restyle is needed on those text
> frames?

OK, I just tried with a fresh build and for some reason the assertion no longer shows up. There was still an issue with the rendering not updated in displaystyle-3.html, but updating nsStyleFont::CalcDifference solved that. So all these new tests pass now. I've sent the patches to try server.

Comment 13

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

Attachment: Part 3 - MathML frames

unbitrot this patch

Comment 14

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

Attachment: Part 1 - CSS

Just doing the change for nsStyleFont::CalcDifference...

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&rev=ddcf8b6a8403

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8356527 [details] [diff] [review]
Part 1 - CSS

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

r=me with comments addressed.

::: layout/style/nsCSSPropList.h
@@ +3356,5 @@
>      // property when mUnsafeRulesEnabled is set.
>      CSS_PROPERTY_PARSE_VALUE,
>      "",
> +    // script-level can take Auto, Integer and Number values, but only Auto
> +    // ("increment if parent is not in displaystyle") and Integer

I'll assume this logic ("increment if parent is not in displaystyle") makes sense.

::: layout/style/nsCSSProps.cpp
@@ +1359,5 @@
> +const int32_t nsCSSProps::kDisplayStyleKTable[] = {
> +  eCSSKeyword_none, NS_MATHML_DISPLAYSTYLE_NONE,
> +  eCSSKeyword_true, NS_MATHML_DISPLAYSTYLE_TRUE,
> +  eCSSKeyword_UNKNOWN,-1
> +};

"none" and "true" aren't good values for this property; we should avoid "true/false" values, and "none" and "true" don't seem to be complementary.  How about "display" and "text" make sense (to match \displaystyle and \textstyle mentioned in the spec)?

::: layout/style/nsRuleNode.cpp
@@ +1916,1 @@
>             // MathML is not in use. Therefore if all but four

s/four/five/

@@ +3462,5 @@
>      // "absolute"
>      aFont->mScriptLevel = ClampTo8Bit(int32_t(scriptLevelValue->GetFloatValue()));
>    }
> +  else if (eCSSUnit_Auto == scriptLevelValue->GetUnit()) {
> +    // auto

I think this -- and the eCSSUnit_Integer case above -- should be setting aCanStoreInRuleTree to false since it relies on the inherited value.

::: layout/style/nsStyleConsts.h
@@ +553,5 @@
>  #define NS_MATHML_MATHVARIANT_TAILED                  16
>  #define NS_MATHML_MATHVARIANT_LOOPED                  17
>  #define NS_MATHML_MATHVARIANT_STRETCHED               18
>  
> +// See nsStyleFont

nsStyleFont::mDisplayStyle

Comment 17

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

(In reply to Cameron McCormack (:heycam) from comment #16)
> ::: layout/style/nsCSSProps.cpp
> @@ +1359,5 @@
> > +const int32_t nsCSSProps::kDisplayStyleKTable[] = {
> > +  eCSSKeyword_none, NS_MATHML_DISPLAYSTYLE_NONE,
> > +  eCSSKeyword_true, NS_MATHML_DISPLAYSTYLE_TRUE,
> > +  eCSSKeyword_UNKNOWN,-1
> > +};
> 
> "none" and "true" aren't good values for this property; we should avoid
> "true/false" values, and "none" and "true" don't seem to be complementary. 
> How about "display" and "text" make sense (to match \displaystyle and
> \textstyle mentioned in the spec)?
 
The MathML concept is really displaystyle="true" or displaystyle="false". These correspond to display=block and display=inline for the <math> root, so perhaps these can be used to avoid introducing new keywords. Otherwise, I'm fine with introducing keywords "displaystyle" and "textstyle" although they are more TeX concepts (e.g. \scriptstyle and \scriptscriptstyle  are also displaystyle=false). What do you prefer?

Comment 18

[Cameron McCormack (:heycam)]

Yeah, -moz-display-style: {block,inline} sounds OK to me.  I worry slightly that -moz-display-style might be confusing -- it's not really obvious that it's MathML specific -- but not enough to suggest a name that doesn't match the attribute.

Comment 19

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

Attachment: Part 1 - CSS

Comment 20

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

Attachment: Part 2 - CSS mapping

changing none/true => inline/block for -moz-display-style

Comment 21

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

(In reply to Cameron McCormack (:heycam) from comment #18)
> Yeah, -moz-display-style: {block,inline} sounds OK to me.  I worry slightly
> that -moz-display-style might be confusing -- it's not really obvious that
> it's MathML specific -- but not enough to suggest a name that doesn't match
> the attribute.

I am. How about '-moz-math-display'? 'style' is redundant. It doesn't really matter that it doesn't match the attribute, it's close enough.

Comment 22

[Cameron McCormack (:heycam)]

WFM

Comment 24

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

Attachment: Part 1 - CSS

Comment 25

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

Attachment: Part 2 - CSS mapping

Comment 26

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

Attachment: Part 3 - MathML frames

Comment 27

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

Comment on attachment 8359144 [details] [diff] [review]
Part 3 - MathML frames

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

::: layout/mathml/nsMathMLmencloseFrame.cpp
@@ +350,5 @@
>    if (IsToDraw(NOTATION_LONGDIV) || IsToDraw(NOTATION_RADICAL)) {
>        nscoord phi;
>        // Rule 11, App. G, TeXbook
>        // psi = clearance between rule and content
> +      if (StyleFont()->mMathDisplay)

You should check == NS_MATHML_DISPLAYSTYLE_BLOCK, much clearer than using a boolean.

::: layout/mathml/nsMathMLmfracFrame.cpp
@@ +241,5 @@
>      nscoord denShift1, denShift2;
>  
>      GetNumeratorShifts(fm, numShift1, numShift2, numShift3);
>      GetDenominatorShifts(fm, denShift1, denShift2);
> +    if (StyleFont()->mMathDisplay) {

Same here and everywhere else in this patch

::: layout/mathml/nsMathMLmoFrame.cpp
@@ +561,5 @@
>  }
>  
>  static uint32_t
>  GetStretchHint(nsOperatorFlags aFlags, nsPresentationData aPresentationData,
> +               bool aIsVertical, bool aIsDisplayStyle)

Should be a uint8_t. Or to be even clearer, pass an nsStyleFont so we can get mMathDisplay from it.

Comment 28

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

Attachment: Part 3 - MathML frames

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72f18544d8ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/3703497837ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9323ed1e70
https://hg.mozilla.org/integration/mozilla-inbound/rev/eebf26497204
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb77025a3196

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/72f18544d8ec
https://hg.mozilla.org/mozilla-central/rev/3703497837ee
https://hg.mozilla.org/mozilla-central/rev/0e9323ed1e70
https://hg.mozilla.org/mozilla-central/rev/eebf26497204
https://hg.mozilla.org/mozilla-central/rev/eb77025a3196