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

RESOLVED FIXED in mozilla29

Status

()

Core
MathML
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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"].
(Assignee)

Updated

4 years ago
Blocks: 838509
(Assignee)

Updated

4 years ago
Depends on: 764996

Comment 2

4 years ago
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.
(Assignee)

Comment 3

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

Comment 4

3 years ago
Created attachment 8340339 [details] [diff] [review]
Part 1 - CSS
(Assignee)

Comment 5

3 years ago
Created attachment 8340340 [details] [diff] [review]
Part 2 - CSS mapping
(Assignee)

Comment 6

3 years ago
Created attachment 8340341 [details] [diff] [review]
Part 3 - MathML frames
(Assignee)

Comment 7

3 years ago
Created attachment 8340343 [details] [diff] [review]
Part 4  - cleanup
(Assignee)

Comment 8

3 years ago
Created attachment 8340344 [details] [diff] [review]
Part 5 - tests
(Assignee)

Comment 9

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

Updated

3 years ago
Depends on: 731667
(Assignee)

Comment 10

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

Comment 12

3 years ago
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 13

3 years ago
Created attachment 8356526 [details] [diff] [review]
Part 3 - MathML frames

unbitrot this patch
Attachment #8340341 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8356527 [details] [diff] [review]
Part 1 - CSS

Just doing the change for nsStyleFont::CalcDifference...
Attachment #8340339 - Attachment is obsolete: true
Attachment #8356527 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8340340 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
Attachment #8340343 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
Attachment #8340344 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
Attachment #8356526 - Flags: review?(karlt)
(Assignee)

Comment 15

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ddcf8b6a8403
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+
(Assignee)

Comment 17

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

Comment 19

3 years ago
Created attachment 8357111 [details] [diff] [review]
Part 1 - CSS
Attachment #8356527 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8357112 [details] [diff] [review]
Part 2 - CSS mapping

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.
WFM
Duplicate of this bug: 958914
Blocks: 442637
(Assignee)

Comment 24

3 years ago
Created attachment 8359139 [details] [diff] [review]
Part 1 - CSS
Attachment #8357111 - Attachment is obsolete: true
Attachment #8357112 - Attachment is obsolete: true
Attachment #8357112 - Flags: review?(karlt)
(Assignee)

Comment 25

3 years ago
Created attachment 8359141 [details] [diff] [review]
Part 2 - CSS mapping
Attachment #8359141 - Flags: review?(roc)
Attachment #8359141 - Flags: review?(karlt)
(Assignee)

Comment 26

3 years ago
Created attachment 8359144 [details] [diff] [review]
Part 3 - MathML frames
Attachment #8356526 - Attachment is obsolete: true
Attachment #8356526 - Flags: review?(karlt)
Attachment #8359144 - Flags: review?(roc)
Attachment #8359144 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
Attachment #8359141 - Attachment description: CSS mapping → Part 2 - CSS mapping
(Assignee)

Updated

3 years ago
Attachment #8340343 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8340344 - Flags: review?(roc)
Attachment #8359141 - Flags: review?(roc)
Attachment #8359141 - Flags: review?(karlt)
Attachment #8359141 - Flags: review+
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 #8340343 - Flags: review?(roc)
Attachment #8340343 - Flags: review?(karlt)
Attachment #8340343 - Flags: review+
Attachment #8340344 - Flags: review?(roc)
Attachment #8340344 - Flags: review?(karlt)
Attachment #8340344 - Flags: review+
(Assignee)

Comment 28

3 years ago
Created attachment 8359157 [details] [diff] [review]
Part 3 - MathML frames
Attachment #8359144 - Attachment is obsolete: true
Attachment #8359157 - Flags: review?(roc)
Attachment #8359157 - Flags: review?(roc) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

3 years ago
Depends on: 1011237
You need to log in before you can comment on or make changes to this bug.