Bug 133845

Summary: Implement an internal style property for displaystyle
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bfulgham, buildbot, cdumez, cfleizach, darin, gur.trio, kling, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 138732, 153991, 158884    
Bug Blocks: 108778, 118737, 155542, 155638, 155639, 159623    
Attachments:
Description Flags
testcase (includes WOFF as base64 data)
none
Patch (pure CSS implementation, using a -webkit-math-display property)
none
Screenshot of the testcase (WebKitGTK+)
none
testcase (includes WOFF as base64 data)
none
Proposed patch, work in progress version
none
Proposed patch WIP
none
WIP patch
none
patch
none
Updated patch
none
Patch
none
Patch
none
Patch
none
Patch for EWS testing
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Patch
bfulgham: review+
Patch bfulgham: review+

Description Frédéric Wang (:fredw) 2014-06-13 04:23:20 PDT
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;
}
*/
Comment 1 Frédéric Wang (:fredw) 2014-06-13 08:56:40 PDT
Created attachment 233053 [details]
testcase (includes WOFF as base64 data)
Comment 2 Frédéric Wang (:fredw) 2014-06-13 08:57:15 PDT
Created attachment 233054 [details]
Patch (pure CSS implementation, using a -webkit-math-display property)
Comment 3 Frédéric Wang (:fredw) 2014-06-13 09:00:38 PDT
Created attachment 233055 [details]
Screenshot of the testcase (WebKitGTK+)
Comment 4 Frédéric Wang (:fredw) 2014-06-13 09:01:51 PDT
Note: on Mac, largeop in displaystyle does not work because of bug 133569.
Comment 5 Frédéric Wang (:fredw) 2014-06-18 02:56:28 PDT
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.
Comment 6 Frédéric Wang (:fredw) 2014-06-18 03:03:18 PDT
Created attachment 233302 [details]
testcase (includes WOFF as base64 data)

Only setting the font-family on the <mo>.
Comment 7 Alejandro G. Castro 2014-08-06 13:17:40 PDT
(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.
Comment 8 Darin Adler 2014-08-08 09:30:06 PDT
It’s true, we wouldn’t want an internal implementation detail to be visible to the page content.
Comment 9 Alejandro G. Castro 2014-09-10 02:55:09 PDT
(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.
Comment 10 Alejandro G. Castro 2014-09-10 06:58:19 PDT
(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
Comment 11 Frédéric Wang (:fredw) 2014-09-10 10:09:25 PDT
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.
Comment 12 Alejandro G. Castro 2014-09-11 04:35:30 PDT
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 :).
Comment 13 Alejandro G. Castro 2014-09-11 04:37:34 PDT
(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!
Comment 14 Alejandro G. Castro 2014-09-16 10:41:51 PDT
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.
Comment 15 Alejandro G. Castro 2014-10-03 09:57:28 PDT
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.
Comment 16 Alejandro G. Castro 2014-10-16 07:44:42 PDT
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.
Comment 17 Alejandro G. Castro 2014-11-26 10:43:12 PST
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.
Comment 18 Frédéric Wang (:fredw) 2016-03-16 07:16:37 PDT
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.
Comment 19 Frédéric Wang (:fredw) 2016-04-26 09:01:12 PDT
Created attachment 277378 [details]
Patch
Comment 20 Frédéric Wang (:fredw) 2016-05-12 06:58:44 PDT
Created attachment 278733 [details]
Patch

Just adding a test expectation for iOS too.
Comment 21 Frédéric Wang (:fredw) 2016-06-09 02:02:34 PDT
Created attachment 280901 [details]
Patch for EWS testing
Comment 22 Build Bot 2016-06-09 02:53:03 PDT
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
Comment 23 Build Bot 2016-06-09 02:53:09 PDT
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
Comment 24 Build Bot 2016-06-09 02:55:38 PDT
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
Comment 25 Build Bot 2016-06-09 02:55:44 PDT
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
Comment 26 Build Bot 2016-06-09 03:01:47 PDT
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
Comment 27 Build Bot 2016-06-09 03:01:53 PDT
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
Comment 28 Build Bot 2016-06-09 03:05:10 PDT
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
Comment 29 Build Bot 2016-06-09 03:05:15 PDT
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
Comment 30 Frédéric Wang (:fredw) 2016-06-09 03:28:50 PDT
Created attachment 280909 [details]
Patch
Comment 31 Frédéric Wang (:fredw) 2016-06-25 00:38:03 PDT
Created attachment 282052 [details]
Patch
Comment 32 Brent Fulgham 2016-07-07 13:47:53 PDT
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
Comment 33 Frédéric Wang (:fredw) 2016-07-07 14:47:04 PDT
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.
Comment 34 Frédéric Wang (:fredw) 2016-07-07 14:59:35 PDT
Created attachment 283056 [details]
Patch
Comment 35 Brent Fulgham 2016-07-07 16:12:29 PDT
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.
Comment 36 Frédéric Wang (:fredw) 2016-07-07 22:41:08 PDT
Committed r202960: <http://trac.webkit.org/changeset/202960>