Bug 133845 – Implement an internal style property for displaystyle

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

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

In order to support the MathML displaystyle, the simplest and more reliable way would be to follow Gecko's behavior: implement an internal math-display CSS property or similar and use the value in the MathML code. I'm not familiar with the CSS code, so I can give more details without reading Source/WebCore/css/ first. Then the property can be used in

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/mathml/RenderMathMLOperator.h#L71

and, after bug 118737, in other places in the MathML code.

The user agent stylesheet of Gecko is http://mxr.mozilla.org/mozilla-central/source/layout/mathml/mathml.css and the relevant rules are:

math {
  -moz-math-display: inline;
}
math[mode="display"], math[display="block"] {
  -moz-math-display: block;
}
math[display="inline"] {
  -moz-math-display: inline;
}
math[displaystyle="false"] {
  -moz-math-display: inline;
}
math[displaystyle="true"] {
  -moz-math-display: block;
}

/**************************************************************************/
/* Controlling Displaystyle and Scriptlevel                               */
/**************************************************************************/

/*
  http://www.w3.org/Math/draft-spec/chapter3.html#presm.scriptlevel

  The determination of -moz-math-display for <math> involves the displaystyle
  and display attributes. See the <math> section above.
*/

/*
  Map mstyle@displaystyle to -moz-math-display.
*/
mstyle[displaystyle="false"] {
  -moz-math-display: inline;
}
mstyle[displaystyle="true"] {
  -moz-math-display: block;
}

/*  munder, mover and munderover change the scriptlevels of their children
   using -moz-math-increment-script-level because regular CSS rules are
   insufficient to control when the scriptlevel should be incremented. All other
   cases can be described using regular CSS, so we do it this way because it's
   more efficient and less code. */
:-moz-math-increment-script-level { -moz-script-level: +1; }

/*
   The mfrac element sets displaystyle to "false", or if it was already false
   increments scriptlevel by 1, within numerator and denominator.
*/   
mfrac > * {
    -moz-script-level: auto;
    -moz-math-display: inline;
}

/*
   The mroot element increments scriptlevel by 2, and sets displaystyle to
   "false", within index, but leaves both attributes unchanged within base.
   The msqrt element leaves both attributes unchanged within its argument.
*/
mroot > :not(:first-child) {
    -moz-script-level: +2;
    -moz-math-display: inline;
}

/*
   The msub element [...] increments scriptlevel by 1, and sets displaystyle to
   "false", within subscript, but leaves both attributes unchanged within base.

   The msup element [...] increments scriptlevel by 1, and sets displaystyle to
   "false", within superscript, but leaves both attributes unchanged within
   base.

   The msubsup element [...] increments scriptlevel by 1, and sets displaystyle
   to "false", within subscript and superscript, but leaves both attributes
   unchanged within base.

   The mmultiscripts element increments scriptlevel by 1, and sets displaystyle
   to "false", within each of its arguments except base, but leaves both
   attributes unchanged within base.
 */
msub > :not(:first-child),
msup > :not(:first-child),
msubsup > :not(:first-child),
mmultiscripts > :not(:first-child) {
    -moz-script-level: +1;
    -moz-math-display: inline;
}

/*
   The munder element [...] always sets displaystyle to "false" within the
   underscript, but increments scriptlevel by 1 only when accentunder is
   "false". Within base, it always leaves both attributes unchanged.

   The mover element [...] always sets displaystyle to "false" within
   overscript, but increments scriptlevel by 1 only when accent is "false".
   Within base, it always leaves both attributes unchanged.

   The munderover [..] always sets displaystyle to "false" within underscript
   and overscript, but increments scriptlevel by 1 only when accentunder or
   accent, respectively, are "false". Within base, it always leaves both
   attributes unchanged.
*/
munder > :not(:first-child),
mover > :not(:first-child),
munderover > :not(:first-child) {
    -moz-math-display: inline;
}

/*
   The displaystyle attribute is allowed on the mtable element to set the
   inherited value of the attribute. If the attribute is not present, the
   mtable element sets displaystyle to "false" within the table elements.
*/
mtable { -moz-math-display: inline; }
mtable[displaystyle="true"] { -moz-math-display: block; }

/*
   The mscarries element sets displaystyle to "false", and increments
   scriptlevel by 1, so the children are typically displayed in a smaller font.
   XXXfredw: This element is not implemented yet. See bug 534967.
mscarries {
  -moz-script-level: +1;
  -moz-math-display: inline;
}
*/

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

Created attachment 233053 [details]
testcase (includes WOFF as base64 data)

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

Created attachment 233054 [details]
Patch (pure CSS implementation, using a -webkit-math-display property)

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

Created attachment 233055 [details]
Screenshot of the testcase (WebKitGTK+)

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

Note: on Mac, largeop in displaystyle does not work because of bug 133569.

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

So it seems possible to access "-webkit-math-display" with e.g.

<math style="-webkit-math-display: block;"><mo>∑</mo></math>

I don't know what is WebKit's policy with internal property but Gecko does not allow users to set them. Perhaps I have missed something in the patch.

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

Created attachment 233302 [details]
testcase (includes WOFF as base64 data)

Only setting the font-family on the <mo>.

[Alejandro G. Castro]

(In reply to comment #5)
> So it seems possible to access "-webkit-math-display" with e.g.
> 
> <math style="-webkit-math-display: block;"><mo>∑</mo></math>
> 
> I don't know what is WebKit's policy with internal property but Gecko does not allow users to set them. Perhaps I have missed something in the patch.

I do not know if there is a policy for this or if this is something it has been accepted in the past. I'm adding some of the suggested reviewers to check if they have some comment about the proposed solution with the internal property because we are interested in finishing the patch.

[Darin Adler]

It’s true, we wouldn’t want an internal implementation detail to be visible to the page content.

[Alejandro G. Castro]

(In reply to comment #6)
> Created an attachment (id=233302) [details]
> testcase (includes WOFF as base64 data)
> 
> Only setting the font-family on the <mo>.

Fred the example is using displaystyle in the math element, is this something approved in the standard, could you point me to where it is. I have a patch for this implementing the displaystyle property and using an style class to encapsulate the logic about it, still some work pending but with the use of displaystyle in math like it is done in this test requires more logic.

[Alejandro G. Castro]

(In reply to comment #9)
> (In reply to comment #6)
> > Created an attachment (id=233302) [details] [details]
> > testcase (includes WOFF as base64 data)
> > 
> > Only setting the font-family on the <mo>.
> 
> Fred the example is using displaystyle in the math element, is this something approved in the standard, could you point me to where it is. I have a patch for this implementing the displaystyle property and using an style class to encapsulate the logic about it, still some work pending but with the use of displaystyle in math like it is done in this test requires more logic.

Answering myself, I found the bug where it is added in the Mozilla implementation, more information in that bug the other bugs it links:

https://bugzilla.mozilla.org/show_bug.cgi?id=669719

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

Yes, displaystyle on the <math> element was added in MathML3. More generally by

http://www.w3.org/TR/MathML3/chapter2.html#interf.toplevel.atts

"The math element accepts any of the attributes that can be set on Section 3.3.4 Style Change <mstyle>, including the common attributes specified in Section 2.1.6 Attributes Shared by all MathML Elements"

Note that the rule for displaystyle are a bit more complex, since it is related to scriptlevel and movablelimits (that are not implemented in WebKit yet and to be handled in followup bugs):

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

For now, the only visible effect if you want to write tests is to consider the size of large operators (using an OpenType MATH font) like in the testcase, where square glyphs are used for N-ARY SUMMATION U+2211.

[Alejandro G. Castro]

Created attachment 237951 [details]
Proposed patch, work in progress version

This is a different solution trying to avoid using a new CSS attribute, and encapsulating all the displaystyle logic in one class RenderMathMLStyle. In that class we could add the support to the scriptlevel more easily. The patch adds displaystyle attribute support to the mathml elements and uses the value when the renderers are already attached to resolve the displaystyle value in the math renderers tree. I had to add also some interfaces to other mathml renderers to implement the required logic, but they are simple: like checking if a node is the base of a root, or the base of a munder mover. Still needs to add tests or check if the relayout works as expected, other than that feedback is more than welcome :).

[Alejandro G. Castro]

(In reply to comment #11)
> Yes, displaystyle on the <math> element was added in MathML3. More generally by
> 
> http://www.w3.org/TR/MathML3/chapter2.html#interf.toplevel.atts
>

Thanks for the hints Fred!

[Alejandro G. Castro]

Created attachment 238186 [details]
Proposed patch WIP

I've fixed the rendering when value modified from javascript and refactored some other code. My plan now is to write a test for javascript updates and check if the element property is really necessary, after that I'll upload a patch for review.

[Alejandro G. Castro]

Created attachment 239207 [details]
WIP patch

Next version with some refactorings, I think it is almost ready but I want to solve a rendering issue with the roots that it is already there to make sure we test properly that.

[Alejandro G. Castro]

Created attachment 239947 [details]
patch

This is a new version of the patch, now using the is<> and downcast<> and rebased. I was planning to submit the patch for review but there are a couple of rendering issues already in the code that the we probably should fix first. The rendering of the underover does not work after r174540 (bug #137330), the root rendering does not work for the stretchy fonts (I have to check if bug #122297 patch fixes that) and I think there is some layout and render tree modification in layout which we probably should avoid.

[Alejandro G. Castro]

Created attachment 242235 [details]
Updated patch

I've rebased the patch over the last HEAD and the operator refactor (#138732), that fixed the underover issues but still some problems with the mtables, and with the defined math font.

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

Created attachment 274187 [details]
Patch

Here is an update of Alex's patch, applying on top of the patches for bug 153991 and its dependencies.

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

Created attachment 277378 [details]
Patch

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

Created attachment 278733 [details]
Patch

Just adding a test expectation for iOS too.

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

Created attachment 280901 [details]
Patch for EWS testing

[Build Bot]

Comment on attachment 280901 [details]
Patch for EWS testing

Attachment 280901 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1471473

New failing tests:
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

Created attachment 280905 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280901 [details]
Patch for EWS testing

Attachment 280901 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1471478

New failing tests:
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

Created attachment 280906 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280901 [details]
Patch for EWS testing

Attachment 280901 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1471479

New failing tests:
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

Created attachment 280907 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 280901 [details]
Patch for EWS testing

Attachment 280901 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1471491

New failing tests:
editing/selection/selection-in-iframe-removed-crash.html
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

Created attachment 280908 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

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

Created attachment 280909 [details]
Patch

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

Created attachment 282052 [details]
Patch

[Brent Fulgham]

Comment on attachment 282052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282052&action=review

Can you please figure out why this isn't applying? I think the patch and tests look good otherwise. r=me, but please don't land until you can confirm tests are okay.

> Source/WebCore/rendering/mathml/MathMLStyle.cpp:46
> +    : m_displayStyle(false)

Please make this a C++11 initialization.

> Source/WebCore/rendering/mathml/MathMLStyle.cpp:63
> +        m_displayStyle = downcast<RenderMathMLBlock>(renderer)->mathMLStyle()->displayStyle();

All of this manual dispatch is kind of ugly. It's too bad RenderMathMLTable and RenderMathMLBlock couldn't share some of this logic.

> Source/WebCore/rendering/mathml/MathMLStyle.cpp:72
> +            downcast<RenderMathMLBlock>(child)->mathMLStyle()->resolveMathMLStyle(child);

Ditto.

> Source/WebCore/rendering/mathml/MathMLStyle.cpp:129
> +

Please remove this blank line.

> Source/WebCore/rendering/mathml/MathMLStyle.cpp:133
> +        else if (attributeValue == "false")

Should this just be 'else', rather than looking for an explicit 'false'? Are there values other than 'true' or 'false' that might be retrieved here?

> Source/WebCore/rendering/mathml/MathMLStyle.h:27
> +#define MathMLStyle_h

#pragma once

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

Comment on attachment 282052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282052&action=review

>> Source/WebCore/rendering/mathml/MathMLStyle.cpp:63
>> +        m_displayStyle = downcast<RenderMathMLBlock>(renderer)->mathMLStyle()->displayStyle();
> 
> All of this manual dispatch is kind of ugly. It's too bad RenderMathMLTable and RenderMathMLBlock couldn't share some of this logic.

Yes, I've been discussing with my team about making RenderMathMLTable derive from RenderMathMLBlock which would help to implement MathML-specific table features and make math tables better integrated with the rest of MathML. But we have not decided about that yet. Let's just add a FIXME comment for now.

>> Source/WebCore/rendering/mathml/MathMLStyle.cpp:133
>> +        else if (attributeValue == "false")
> 
> Should this just be 'else', rather than looking for an explicit 'false'? Are there values other than 'true' or 'false' that might be retrieved here?

IIUC, I think this is because when the attribute is absent or the value is invalid it should just be ignored i.e. m_displayStyle should not be modified.

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

Created attachment 283056 [details]
Patch

[Brent Fulgham]

Comment on attachment 283056 [details]
Patch

Excellent! Looks like it's passing all tests. If we see a Windows failure, please let me know and Per and I can correct it.

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

Committed r202960: <http://trac.webkit.org/changeset/202960>