Closed Bug 838506 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: fredw, Assigned: fredw)

References

Details

Attachments

(5 files, 8 obsolete files)

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.
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"].
Blocks: 838509
Depends on: 764996
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.
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.
Assignee: nobody → fred.wang
Blocks: 713606
Status: NEW → ASSIGNED
Keywords: helpwanted
Whiteboard: [mentor=fredw][lang=c++]
Attached patch Part 1 - CSS (obsolete) — Splinter Review
Attached patch Part 2 - CSS mapping (obsolete) — Splinter Review
Attached patch Part 3 - MathML frames (obsolete) — Splinter Review
Attached patch Part 4 - cleanupSplinter Review
Attached patch Part 5 - testsSplinter Review
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.
Depends on: 731667
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.
Flags: needinfo?(roc)
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
(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.
Attached patch Part 3 - MathML frames (obsolete) — Splinter Review
unbitrot this patch
Attachment #8340341 - Attachment is obsolete: true
Attached patch Part 1 - CSS (obsolete) — Splinter Review
Just doing the change for nsStyleFont::CalcDifference...
Attachment #8340339 - Attachment is obsolete: true
Attachment #8356527 - Flags: review?(cam)
Attachment #8340340 - Flags: review?(karlt)
Attachment #8340343 - Flags: review?(karlt)
Attachment #8340344 - Flags: review?(karlt)
Attachment #8356526 - Flags: review?(karlt)
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
Attachment #8356527 - Flags: review?(cam) → review+
(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?
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
Attached patch Part 1 - CSS (obsolete) — Splinter Review
Attachment #8356527 - Attachment is obsolete: true
Attached patch Part 2 - CSS mapping (obsolete) — Splinter Review
changing none/true => inline/block for -moz-display-style
Attachment #8340340 - Attachment is obsolete: true
Attachment #8340340 - Flags: review?(karlt)
Attachment #8357112 - Flags: review?(karlt)
(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.
Duplicate of this bug: 958914
Blocks: 442637
Attached patch Part 1 - CSSSplinter Review
Attachment #8357111 - Attachment is obsolete: true
Attachment #8357112 - Attachment is obsolete: true
Attachment #8357112 - Flags: review?(karlt)
Attachment #8359141 - Flags: review?(roc)
Attachment #8359141 - Flags: review?(karlt)
Attached patch Part 3 - MathML frames (obsolete) — Splinter Review
Attachment #8356526 - Attachment is obsolete: true
Attachment #8356526 - Flags: review?(karlt)
Attachment #8359144 - Flags: review?(roc)
Attachment #8359144 - Flags: review?(karlt)
Attachment #8359141 - Attachment description: CSS mapping → Part 2 - CSS mapping
Attachment #8340343 - Flags: review?(roc)
Attachment #8340344 - Flags: review?(roc)
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.
Attachment #8359144 - Flags: review?(roc)
Attachment #8359144 - Flags: review?(karlt)
Attachment #8359144 - Flags: review-
Attachment #8359144 - Attachment is obsolete: true
Attachment #8359157 - Flags: review?(roc)
Keywords: checkin-needed
Depends on: 1011237
You need to log in before you can comment on or make changes to this bug.