Bug 180029 – MathML Lengths should take zoom level into account

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

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

Created attachment 327623 [details]
Testcase (LayoutTests/mathml/presentation/mspace-units.html)

This is a particular case of bug 170229. If you zoom in and out the attached testcase (LayoutTests/mathml/presentation/mspace-units.html) then you see that the relative lengths of bars are inconsistent. This is not the case for the CSS-based reference (LayoutTests/mathml/presentation/mspace-units-expected.html). I believe RenderMathMLBlock's toUserUnits() should multiply some of the values by style.effectiveZoom().

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

Created attachment 327624 [details]
Patch

This patch demonstrates how mspace-units.html fails for zoomlevel=3. This gives a way to write a test for this bug.

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

@Minsheng: Did you get a chance to look into this? I suspect it should be relatively easy to address since it's just one function to modify and the test is already almost written ;-)

[Minsheng Liu]

Created attachment 331279 [details]
patch

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

Comment on attachment 331279 [details]
patch

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

> LayoutTests/ChangeLog:10
> +        * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor().

mspace-units does not have setPageZoomFactor() and I believe the test should actually *not* be modified as it is not affected by this patch (and please don't do whitespace only change as that makes the history more complicated).

> LayoutTests/mathml/presentation/mspace-units-with-zoom-expected.html:8
> +    }

That's interesting and seems better than using window.internals. It's the only difference you make with respect to the mspace-units.html test, right?

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:173
> +

Mmh, that seems complicated. Why don't you just do "* style.effectiveZoom()" in the 6 cases below? That would make things more readable.

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #4)
> Comment on attachment 331279 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331279&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor().
> 
> mspace-units does not have setPageZoomFactor() and I believe the test should
> actually *not* be modified as it is not affected by this patch (and please
> don't do whitespace only change as that makes the history more complicated).
> 
> > LayoutTests/mathml/presentation/mspace-units-with-zoom-expected.html:8
> > +    }
> 
> That's interesting and seems better than using window.internals. It's the
> only difference you make with respect to the mspace-units.html test, right?

Oops! I mistook the file you provided on this page with the one we have in the repository.

Note that the old test does not check the unit "mu". I agree you with that a significant change to whitespace is not history-friendly, something I have not thought of before. So I will submit a separate patch to amend that test later. For this patch, I will only include the new mspace-units-with-zoom.

And yes, other than the CSS property "zoom" and the check for "mu", there is nothing new.

> > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:173
> > +
> 
> Mmh, that seems complicated. Why don't you just do "* style.effectiveZoom()"
> in the 6 cases below? That would make things more readable.

I had a hard time deciding between code duplication and readability. If we are using Rust or other switch-as-expression languages, I will try to argue. However, since I am duplicating lots of "break" anyway, I agree with your proposal.

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

Comment on attachment 331279 [details]
patch

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

>>> LayoutTests/ChangeLog:10
>>> +        * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor().
>> 
>> mspace-units does not have setPageZoomFactor() and I believe the test should actually *not* be modified as it is not affected by this patch (and please don't do whitespace only change as that makes the history more complicated).
> 
> Oops! I mistook the file you provided on this page with the one we have in the repository.
> 
> Note that the old test does not check the unit "mu". I agree you with that a significant change to whitespace is not history-friendly, something I have not thought of before. So I will submit a separate patch to amend that test later. For this patch, I will only include the new mspace-units-with-zoom.
> 
> And yes, other than the CSS property "zoom" and the check for "mu", there is nothing new.

OK, I did not notice your addition of "mu" (that's another reason not to do whitespace-only changes as that makes review much harder). Note that "mu" is not a CSS property nor a MathML unit. It's just an internal property based on LaTeX to represent MathML's namedspace (see Source/WebCore/mathml/MathMLElement.h). So I don't think you need to test it but if you really want to, you should use namespaces like "verythickmathspace" not "mu".

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #6)
> Comment on attachment 331279 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331279&action=review
> 
> >>> LayoutTests/ChangeLog:10
> >>> +        * mathml/presentation/mspace-units-expected.html: Remove the call to setPageZoomFactor().
> >> 
> >> mspace-units does not have setPageZoomFactor() and I believe the test should actually *not* be modified as it is not affected by this patch (and please don't do whitespace only change as that makes the history more complicated).
> > 
> > Oops! I mistook the file you provided on this page with the one we have in the repository.
> > 
> > Note that the old test does not check the unit "mu". I agree you with that a significant change to whitespace is not history-friendly, something I have not thought of before. So I will submit a separate patch to amend that test later. For this patch, I will only include the new mspace-units-with-zoom.
> > 
> > And yes, other than the CSS property "zoom" and the check for "mu", there is nothing new.
> 
> OK, I did not notice your addition of "mu" (that's another reason not to do
> whitespace-only changes as that makes review much harder). Note that "mu" is
> not a CSS property nor a MathML unit. It's just an internal property based
> on LaTeX to represent MathML's namedspace (see
> Source/WebCore/mathml/MathMLElement.h). So I don't think you need to test it
> but if you really want to, you should use namespaces like
> "verythickmathspace" not "mu".

I will not add it then.

[Minsheng Liu]

Created attachment 331282 [details]
Patch

[Minsheng Liu]

Note that I reorder the switch-block to put physical units together. I hope this could improve readability a bit.

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

Comment on attachment 331282 [details]
Patch

Yes, I think that's better. Thanks!

[WebKit Commit Bot]

Comment on attachment 331282 [details]
Patch

Clearing flags on attachment: 331282

Committed r226936: <https://trac.webkit.org/changeset/226936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36495027>