Bug 49309 – [MathML] Scriptlevel attribute implementation (via math-depth)

• Status: UNCONFIRMED
• Component: MathML

[François Sausset]

Scriptlevel attribute should be implemented. Other related attributes like: displaystyle, scriptminsize, scriptsizemultiplier, etc should be implemented at the same time.
It needs to refactor token element renderers.
Others adjustments in msup, msub msubsup, mfrac, mroot, etc are also needed.

[François Sausset]

Created attachment 73470 [details]
Patch

[François Sausset]

Created attachment 73481 [details]
Regenerated patch

The webcore xcodeproj file was updated just after the patch creation and before the patch submission.
This new patch should be OK.

[Kenneth Rohde Christiansen]

Comment on attachment 73481 [details]
Regenerated patch

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

> WebCore/mathml/MathMLInlineContainerElement.cpp:59
> -    if (hasLocalName(MathMLNames::mrowTag))
> +    if (hasLocalName(mrowTag))

This seems like a separate change which could be in its own patch, to make it more clear what was changed.

> WebCore/mathml/MathMLMathElement.cpp:51
> +void MathMLMathElement::attributeChanged(Attribute* attr, bool preserveDecls)

I would write out preserveDeclarations.

> WebCore/mathml/MathMLMathElement.cpp:54
> +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());

remove the space between > and (

> WebCore/mathml/MathMLStyleElement.cpp:53
> +    if (renderer()) {

I would do

if (!renderer())
    return;

It makes it more clear, it is common webkit style and the code doesnt get unnecessarily indented

> WebCore/mathml/MathMLStyleElement.cpp:54
> +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());

remove space between > and (

> WebCore/mathml/MathMLStyleElement.cpp:55
> +        if (attr->name() == displaystyleAttr) {

why are these attributes named like that? why not displayStyleAttr ?

> WebCore/mathml/MathMLStyleElement.cpp:59
> +            if (attr->value() == "true")
> +                block->setDisplayStyle(true);
> +            else if (attr->value() == "false" || attr->value().isEmpty())
> +                block->setDisplayStyle(false);

What is someone set something wrong here? Shouldn't that be handled?

> WebCore/mathml/MathMLStyleElement.cpp:64
> +                block->setScriptSizeMultiplier(0.71);

Where does 0.71 come from? comment?

> WebCore/mathml/MathMLStyleElement.cpp:70
> +                block->setScriptMinSize(newLengthArray("8pt", size)[0].value());

8pt? I don't like hardcoding these things

> WebCore/mathml/RenderMathMLBlock.cpp:47
> +    , m_scriptSizeMultiplier(0.71)

You seem to be using this 0.71 in more places.

> WebCore/mathml/RenderMathMLBlock.cpp:50
> +    m_scriptMinSize = newLengthArray("8pt", size)[0].value();

And the 8pt as well

> WebCore/mathml/RenderMathMLRoot.cpp:94
> +            index->setScriptLevel(scriptLevel() + 2);

why +2 ? comment

> WebCore/mathml/RenderMathMLRoot.cpp:98
> +            // always add to the index

Comments starts with capital, ends with dot

> WebCore/mathml/RenderMathMLStyle.cpp:53
> +        String displayStyle = style->getAttribute(displaystyleAttr);
> +        if (equalIgnoringCase(displayStyle, "true"))
> +            setDisplayStyle(true);
> +        else if (equalIgnoringCase(displayStyle, "false"))
> +            setDisplayStyle(false);
> +        

What if it is neither true or false?

> WebCore/mathml/RenderMathMLStyle.cpp:73
> +        if (!level.isEmpty()) {
> +            if (level.startsWith("+")) {
> +                level.remove(0);
> +                setScriptLevel(scriptLevel() + level.toInt());
> +            } else if (level.startsWith("-")) {
> +                level.remove(0);
> +                setScriptLevel(scriptLevel() - level.toInt());
> +            } else
> +            setScriptLevel(level.toInt());
> +        }

I tihnk I saw very similar code elsewhere in the patch

> WebCore/mathml/RenderMathMLSubSup.cpp:124
> +            // Adjust the script placement after we stretch

Add dot at the end

> WebCore/mathml/RenderMathMLSubSup.cpp:163
> +    RenderObject* base = firstChild();
> +    if (base) {

Why not 

if (RenderObject* base = firstChild()) {

> WebCore/mathml/RenderMathMLSubSup.cpp:202
> +//            script->setNeedsLayout(true);

Don't commit outcommented code

[François Sausset]

(In reply to comment #3)
> (From update of attachment 73481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73481&action=review
> 
> > WebCore/mathml/MathMLInlineContainerElement.cpp:59
> > -    if (hasLocalName(MathMLNames::mrowTag))
> > +    if (hasLocalName(mrowTag))
> 
> This seems like a separate change which could be in its own patch, to make it more clear what was changed.
I'll fill a separate bug.

> > WebCore/mathml/MathMLMathElement.cpp:51
> > +void MathMLMathElement::attributeChanged(Attribute* attr, bool preserveDecls)
> 
> I would write out preserveDeclarations.
Done

> > WebCore/mathml/MathMLMathElement.cpp:54
> > +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());
> 
> remove the space between > and (
Done

> > WebCore/mathml/MathMLStyleElement.cpp:53
> > +    if (renderer()) {
> 
> I would do
> 
> if (!renderer())
>     return;
Done
 
> > WebCore/mathml/MathMLStyleElement.cpp:54
> > +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());
> 
> remove space between > and (
Done
 
> > WebCore/mathml/MathMLStyleElement.cpp:55
> > +        if (attr->name() == displaystyleAttr) {
> 
> why are these attributes named like that? why not displayStyleAttr ?
I followed the style used in mathattrs.in. But I agree we could change it as you suggested to be more compliant with the general WebKit style.
I think that it should be done in a separate patch.

> > WebCore/mathml/MathMLStyleElement.cpp:59
> > +            if (attr->value() == "true")
> > +                block->setDisplayStyle(true);
> > +            else if (attr->value() == "false" || attr->value().isEmpty())
> > +                block->setDisplayStyle(false);
> 
> What is someone set something wrong here? Shouldn't that be handled?
A wrong value is ignored and the default value (false) set in the renderMathBlock constructor is used.

> > WebCore/mathml/MathMLStyleElement.cpp:64
> > +                block->setScriptSizeMultiplier(0.71);
> 
> Where does 0.71 come from? comment?
> 
> > WebCore/mathml/MathMLStyleElement.cpp:70
> > +                block->setScriptMinSize(newLengthArray("8pt", size)[0].value());
> 
> 8pt? I don't like hardcoding these things
> 
> > WebCore/mathml/RenderMathMLBlock.cpp:47
> > +    , m_scriptSizeMultiplier(0.71)
> 
> You seem to be using this 0.71 in more places.
> 
> > WebCore/mathml/RenderMathMLBlock.cpp:50
> > +    m_scriptMinSize = newLengthArray("8pt", size)[0].value();
> 
> And the 8pt as well
These values come from the w3c MathML 3 Recommendation.
Comments added and simplification of the code.

> > WebCore/mathml/RenderMathMLRoot.cpp:94
> > +            index->setScriptLevel(scriptLevel() + 2);
> 
> why +2 ? comment
This value comes from the w3c MathML 3 Recommendation.
Comment added.

> > WebCore/mathml/RenderMathMLRoot.cpp:98
> > +            // always add to the index
> 
> Comments starts with capital, ends with dot
Corrected

> > WebCore/mathml/RenderMathMLStyle.cpp:53
> > +        String displayStyle = style->getAttribute(displaystyleAttr);
> > +        if (equalIgnoringCase(displayStyle, "true"))
> > +            setDisplayStyle(true);
> > +        else if (equalIgnoringCase(displayStyle, "false"))
> > +            setDisplayStyle(false);
> > +        
> 
> What if it is neither true or false?
The default value (false) set in the RenderMathMLBlock constructor is used.

> > WebCore/mathml/RenderMathMLStyle.cpp:73
> > +        if (!level.isEmpty()) {
> > +            if (level.startsWith("+")) {
> > +                level.remove(0);
> > +                setScriptLevel(scriptLevel() + level.toInt());
> > +            } else if (level.startsWith("-")) {
> > +                level.remove(0);
> > +                setScriptLevel(scriptLevel() - level.toInt());
> > +            } else
> > +            setScriptLevel(level.toInt());
> > +        }
> 
> I tihnk I saw very similar code elsewhere in the patch
Right. I factorized the code.

> > WebCore/mathml/RenderMathMLSubSup.cpp:124
> > +            // Adjust the script placement after we stretch
> 
> Add dot at the end
Done

> > WebCore/mathml/RenderMathMLSubSup.cpp:163
> > +    RenderObject* base = firstChild();
> > +    if (base) {
> 
> Why not 
> 
> if (RenderObject* base = firstChild()) {
Done

> > WebCore/mathml/RenderMathMLSubSup.cpp:202
> > +//            script->setNeedsLayout(true);
> 
> Don't commit outcommented code
Corrected

[François Sausset]

Created attachment 73973 [details]
Revised patch

[Eric Seidel (no email)]

Comment on attachment 73973 [details]
Revised patch

There is a huge psychological barrier (if nothing else) to reviewing a 1MB patch.  In this case, it might be better to separate out the test changes.  The code changes look rather large too though, so this may just need to be broken down in general.

[Brent Fulgham]

This patch is very old. Is it still relevant? At the very least, it should be rebased so that it can apply against current sources.

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

(In reply to Brent Fulgham from comment #7)
> This patch is very old. Is it still relevant? At the very least, it should
> be rebased so that it can apply against current sources.

Probably the way forward will be to implement math-script-level/math-style and just map scriptlevel/displaystyle to these CSS properties. See https://github.com/w3c/csswg-drafts/issues/3746

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

*** Bug 118738 has been marked as a duplicate of this bug. ***

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

MathML Core describes scriptlevel here, it's based on math-depth and interacts with the math-style attribute implemented in bug 216702:

https://w3c.github.io/mathml-core/#the-displaystyle-and-scriptlevel-attributes
https://w3c.github.io/mathml-core/#the-math-script-level-property