Bug 77916 – Update LayoutUnit usage in RenderMathML* classes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Much of RenderMathML should use integers instead of LayoutUnits, as are currently specified.

[Levi Weintraub]

Created attachment 125736 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 125736 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Updating usage of LayoutUnits in the RenderMathML* classes. Many of these values
> +        were LayoutUnits but should have been integers.

Please explain why.

[Levi Weintraub]

Created attachment 126144 [details]
Patch

[Eric Seidel (no email)]

I want to review this, but I still dont' feel like you've expained why this is correct.  I have little doubt that it is, but these things should be obvious from teh ChangeLog or from some existing documentation about when we use LayoutUnit vs. Int. :)

[Levi Weintraub]

(In reply to comment #4)
> I want to review this, but I still dont' feel like you've expained why this is correct.  I have little doubt that it is, but these things should be obvious from teh ChangeLog or from some existing documentation about when we use LayoutUnit vs. Int. :)

It's actually very difficult to make this clear before all the roundedIntPoint and pixelSnappedIntRect changes go in, so I'm going to table this patch until this patch is clearer.

[Levi Weintraub]

Comment on attachment 126144 [details]
Patch

Removing this from the review queue pending some more sub-pixel patches landing.

[Levi Weintraub]

Created attachment 130645 [details]
Patch

[Dave Barton]

Perhaps I'm uninformed, but I'd have thought that the MathML code should be moving from ints to LayoutUnits, instead of the opposite direction. Even before iOS pinching and zooming, and perhaps even before the www existed, mathematics was routinely zoomed in e.g. MS Word's equation editor. Often greek letters and other delicate symbols are rendered, including at reduced point sizes in subscripts, superscripts, fractions, etc. I believe MathJax and MathPlayer both provide a contextmenu (right-click) command to zoom because of this. TeX's high quality has led people to have high expectations. Users will expect that when a fraction is zoomed (enlarged), the space above and below the bar will be visually identical, not rounded to CSS pixels. This is also true for the thickness of the fraction bar, sub/superscript placement, under/overscript placement, radical index placement, and general baseline position. Is it clear that even the height & width of MathML elements should be ints? I think the closest analogy in WebKit would be a diagram using SVG or <canvas> with a lot of labelled nested parts at arbitrary positions, and that's often viewed at magnification. Would it be good to round all such x and y positions to an integer number of CSS pixels? I feel I'm missing something basic here. Thanks in advance for any explanation.

[Levi Weintraub]

(In reply to comment #8)
> Perhaps I'm uninformed, but I'd have thought that the MathML code should be moving from ints to LayoutUnits, instead of the opposite direction. Even before iOS pinching and zooming, and perhaps even before the www existed, mathematics was routinely zoomed in e.g. MS Word's equation editor. Often greek letters and other delicate symbols are rendered, including at reduced point sizes in subscripts, superscripts, fractions, etc. I believe MathJax and MathPlayer both provide a contextmenu (right-click) command to zoom because of this. TeX's high quality has led people to have high expectations. Users will expect that when a fraction is zoomed (enlarged), the space above and below the bar will be visually identical, not rounded to CSS pixels. This is also true for the thickness of the fraction bar, sub/superscript placement, under/overscript placement, radical index placement, and general baseline position. Is it clear that even the height & width of MathML elements should be ints? I think the closest analogy in WebKit would be a diagram using SVG or <canvas> with a lot of labelled nested parts at arbitrary positions, and that's often viewed at magnification. Would it be good to round all such x and y positions to an integer number of CSS pixels? I feel I'm missing something basic here. Thanks in advance for any explanation.

Allow me to explain: with this patch, MathML rendering remains how it currently is regardless of the backend of LayoutUnits. It will still be able to be zoomed like all other web content. Directly improving MathML-specific sub-pixel rendering is desirable and -- after we convert LayoutUnits to a sub-pixel type -- quite possible. It's not, however, the primary goal of our conversion to sub-pixel layout, and the MathML rendering code will require further tweaking to get improved results. Once we've completed the transition to sub-pixel LayoutUnits, I'll welcome patches that improve how MathML uses them.

[Julien Chaffraix]

Comment on attachment 130645 [details]
Patch

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

Dave Barton maybe you have an opinion on this change.

> Source/WebCore/ChangeLog:10
> +        Subpixel positioning offers little benefit for MathML, and can result in
> +        unwanted rounding in highly nested contexts. Instead, we use pixel snapping
> +        to ensure things continue to look as they did.

I am in the same situation as Eric here. I don't think I do understand this comment, yet it is super important to properly review this patch.

AFAICT a lot of MathML code uses int so assumes we are doing pixels based manipulation and thus need this conversion is needed to avoid breaking the rendering in non-trivial ways. This looks like this is done by design and not really a limitation of MathML itself.  I understand the short-term goal of not breaking rendering in MathML but I wonder if this is such a good middle-term change.

[Dave Barton]

Levi: I understand that MathML could still be zoomed, my question is what's the best way for it to look after it is zoomed? Another way to say it is that I'm trying to do MathML tweaking in general to get improved results, as you say. Why is it necessary or good to use ints intead of LayoutUnits at this time? Are you suggesting that we land this patch, and then I submit a patch that undoes most or all of the changes here, as well as converting some other ints to LayoutUnits in the MathML code? Should I do this even for MathML element widths and heights? I am trying to explain MathML usage, and then defer to your design and knowledge. Thanks!

Julien: Thanks, your comment helps me a lot. I had submitted https://bugs.webkit.org/show_bug.cgi?id=78310 & its patch a while ago which is basically the opposite of this bug & patch. If we can agree on what is desired, I can go through the MathML code and make sure it doesn't rely on things just being ints, if you want me to. Or we could do this patch first & then its opposite. In general, I think we should move toward what we think is best and not be too paranoid about accidentally changing some existing behavior that a user may be relying on. Chrome doesn't even turn on MathML, and even in Safari/etc. it's so bad that no major MathML site uses it. (The few libraries & sites that use MathML much do browser sniffing and turn it off in Safari/WebKit.)

> Source/WebCore/ChangeLog:10
> +        Subpixel positioning offers little benefit for MathML, and can result in
> +        unwanted rounding in highly nested contexts. Instead, we use pixel snapping
> +        to ensure things continue to look as they did.

Unless I misunderstand, I think I disagree completely with this. Sub-CSS-pixel positioning in MathML seems ideal, especially for the frequent case of zooming. Can you give an example of the unwanted rounding you're trying to prevent in this case? I think we should prefer LayoutUnit instead of int in MathML elements since they may be highly nested, exactly to avoid this problem.

[Levi Weintraub]

(In reply to comment #11)
> Levi: I understand that MathML could still be zoomed, my question is what's the best way for it to look after it is zoomed? Another way to say it is that I'm trying to do MathML tweaking in general to get improved results, as you say. Why is it necessary or good to use ints intead of LayoutUnits at this time? Are you suggesting that we land this patch, and then I submit a patch that undoes most or all of the changes here, as well as converting some other ints to LayoutUnits in the MathML code? Should I do this even for MathML element widths and heights? I am trying to explain MathML usage, and then defer to your design and knowledge. Thanks!

There are 2 primary goals of the sub-pixel positioning project:
1) To enable a page designer to lay out a page at a higher than 1x resolution
2) To ensure that elements and groups of elements scale properly together (see attachment to https://bugs.webkit.org/show_bug.cgi?id=39884)

The gap between the line of a fraction and the text above and below is not related to this work. This can already be done properly in the current implementation.

My goal with this patch is to not regress current MathML performance, nor gate the transition to sub-pixel positioning on it. That is all. I invite you to check out the subpixellayout branch of WebKit to test your changes on and vette your changes to improve this there. In the meantime, going back and forth about this only delays the potential benefits.

> 
> Julien: Thanks, your comment helps me a lot. I had submitted https://bugs.webkit.org/show_bug.cgi?id=78310 & its patch a while ago which is basically the opposite of this bug & patch. If we can agree on what is desired, I can go through the MathML code and make sure it doesn't rely on things just being ints, if you want me to. Or we could do this patch first & then its opposite. In general, I think we should move toward what we think is best and not be too paranoid about accidentally changing some existing behavior that a user may be relying on. Chrome doesn't even turn on MathML, and even in Safari/etc. it's so bad that no major MathML site uses it. (The few libraries & sites that use MathML much do browser sniffing and turn it off in Safari/WebKit.)
> 
> > Source/WebCore/ChangeLog:10
> > +        Subpixel positioning offers little benefit for MathML, and can result in
> > +        unwanted rounding in highly nested contexts. Instead, we use pixel snapping
> > +        to ensure things continue to look as they did.
> 
> Unless I misunderstand, I think I disagree completely with this. Sub-CSS-pixel positioning in MathML seems ideal, especially for the frequent case of zooming. Can you give an example of the unwanted rounding you're trying to prevent in this case? I think we should prefer LayoutUnit instead of int in MathML elements since they may be highly nested, exactly to avoid this problem.

I'll change this comment.

[Levi Weintraub]

Created attachment 130733 [details]
Patch

[Dave Barton]

(In reply to comment #12)
> There are 2 primary goals of the sub-pixel positioning project:
> 1) To enable a page designer to lay out a page at a higher than 1x resolution
> 2) To ensure that elements and groups of elements scale properly together (see attachment to https://bugs.webkit.org/show_bug.cgi?id=39884)

These will be great for MathML. Many measurements would be best at better than CSS-pixel accuracy, even when not zoomed, especially if device pixels are smaller. Also, math is often zoomed, where the win will be even bigger.

> The gap between the line of a fraction and the text above and below is not related to this work. This can already be done properly in the current implementation.

Not if the code wants to specify that gap at better than CSS-pixel precision.

> My goal with this patch is to not regress current MathML performance, nor gate the transition to sub-pixel positioning on it. That is all. I invite you to check out the subpixellayout branch of WebKit to test your changes on and vette your changes to improve this there. In the meantime, going back and forth about this only delays the potential benefits.

If you're saying the best way to proceed is to use int not LayoutUnit in many places in MathML for now, so you can land your patch without re-baselining all the MathML tests, and then we can switch lots of things back to LayoutUnit later, then I agree completely. I think Eric & Julien & I were all confused because we thought you were saying that int is better than LayoutUnit in many cases for MathML, which I disagree with.

> > > Source/WebCore/ChangeLog:10
> > > +        Subpixel positioning offers little benefit for MathML, and can result in
> > > +        unwanted rounding in highly nested contexts. Instead, we use pixel snapping
> > > +        to ensure things continue to look as they did.
> > 
> > Unless I misunderstand, I think I disagree completely with this. Sub-CSS-pixel positioning in MathML seems ideal, especially for the frequent case of zooming. Can you give an example of the unwanted rounding you're trying to prevent in this case? I think we should prefer LayoutUnit instead of int in MathML elements since they may be highly nested, exactly to avoid this problem.
> 
> I'll change this comment.

Will you change the MathML entry in https://trac.webkit.org/wiki/LayoutUnit as well?

Look, if folks want to land this now and discuss MathML subpixel layout later, fine. But at some point we (I) need to know how to write code implementing MathML. The basic problems are exactly your 1) and 2) above. Specifically, in your https://trac.webkit.org/wiki/LayoutUnit design:

A. To avoid unwanted anti-aliasing, can we just round to device pixels instead of css pixels, e.g. at paint time? For things like thin lines in fractions when specified, is it better to just allow anti-aliasing?

B. "The line box tree uses floats for layout and painting, we snap to pixels when positioning and sizing the line box tree to ensure that text isn’t drawn outside the pixel snapped bounds of its block." Would it be analogous to snap the <math> element to pixels, but not snap parts of nested MathML elements? (This would undo much of your submitted patch.)

C. For "SVG Boxes in a RenderBlock", you specify "subpixel, enclosed - Smallest possible subpixel rectangle representation guaranteed to contain subtree." Should MathML subtree boxes behave like this? It sounds better than CSS pixels to me.

It would help me at least to get guidance, ideally from both WebKit and MathML/LaTeX experts, on these issues.

[Levi Weintraub]

(In reply to comment #14)
> (In reply to comment #12)
> > There are 2 primary goals of the sub-pixel positioning project:
> > 1) To enable a page designer to lay out a page at a higher than 1x resolution
> > 2) To ensure that elements and groups of elements scale properly together (see attachment to https://bugs.webkit.org/show_bug.cgi?id=39884)
> 
> These will be great for MathML. Many measurements would be best at better than CSS-pixel accuracy, even when not zoomed, especially if device pixels are smaller. Also, math is often zoomed, where the win will be even bigger.
> 
> > The gap between the line of a fraction and the text above and below is not related to this work. This can already be done properly in the current implementation.
> 
> Not if the code wants to specify that gap at better than CSS-pixel precision.
> 
> > My goal with this patch is to not regress current MathML performance, nor gate the transition to sub-pixel positioning on it. That is all. I invite you to check out the subpixellayout branch of WebKit to test your changes on and vette your changes to improve this there. In the meantime, going back and forth about this only delays the potential benefits.
> 
> If you're saying the best way to proceed is to use int not LayoutUnit in many places in MathML for now, so you can land your patch without re-baselining all the MathML tests, and then we can switch lots of things back to LayoutUnit later, then I agree completely. 

This is essentially my goal, yes. I'm not a MathML expert, and while I'd love to see MathML fully benefit from sub-pixel layout, it's not the primary goal of this conversion. Once the change goes in, it'll be much easier for one more familiar with this code to make the relevant changes.

I think Eric & Julien & I were all confused because we thought you were saying that int is better than LayoutUnit in many cases for MathML, which I disagree with.
> 
> > > > Source/WebCore/ChangeLog:10
> > > > +        Subpixel positioning offers little benefit for MathML, and can result in
> > > > +        unwanted rounding in highly nested contexts. Instead, we use pixel snapping
> > > > +        to ensure things continue to look as they did.
> > > 
> > > Unless I misunderstand, I think I disagree completely with this. Sub-CSS-pixel positioning in MathML seems ideal, especially for the frequent case of zooming. Can you give an example of the unwanted rounding you're trying to prevent in this case? I think we should prefer LayoutUnit instead of int in MathML elements since they may be highly nested, exactly to avoid this problem.
> > 
> > I'll change this comment.
> 
> Will you change the MathML entry in https://trac.webkit.org/wiki/LayoutUnit as well?
> 
> Look, if folks want to land this now and discuss MathML subpixel layout later, fine. But at some point we (I) need to know how to write code implementing MathML. The basic problems are exactly your 1) and 2) above. Specifically, in your https://trac.webkit.org/wiki/LayoutUnit design:
> 
> A. To avoid unwanted anti-aliasing, can we just round to device pixels instead of css pixels, e.g. at paint time? For things like thin lines in fractions when specified, is it better to just allow anti-aliasing?

You'll notice that borders remain integers in the Render Tree despite the conversion to sub-pixel. I'm not sure what the best behavior is for MathML specifically, but for now, my focus is to keep things the same to avoid making things worse.

> 
> B. "The line box tree uses floats for layout and painting, we snap to pixels when positioning and sizing the line box tree to ensure that text isn’t drawn outside the pixel snapped bounds of its block." Would it be analogous to snap the <math> element to pixels, but not snap parts of nested MathML elements? (This would undo much of your submitted patch.)

Again, changing how this behaves is not my focus in this patch. It's to maintain the current behavior.

> 
> C. For "SVG Boxes in a RenderBlock", you specify "subpixel, enclosed - Smallest possible subpixel rectangle representation guaranteed to contain subtree." Should MathML subtree boxes behave like this? It sounds better than CSS pixels to me.

SVG uses floats for layout and rendering. That sort of conversion would require a much larger change to the MathML rendering classes and is beyond the scope of this discussion.

> 
> It would help me at least to get guidance, ideally from both WebKit and MathML/LaTeX experts, on these issues.

[Dave Barton]

Levi, thanks for all the clarification.

In https://trac.webkit.org/wiki/LayoutUnit, for MathML using pixels you say:

"Using integers because subpixel positioning offers little benefit to MathML and can introduce uneven box sizes as they round."

Could you change this to something like "Using integers for now to ease conversion."? I would love to analyze the uneven box sizes comment, but if you don't want to discuss its pros & cons now then could you just remove it?

My only remaining quibble is in your ChangeLog comment you say "using pixel snapping when communicating with other Rendering classes". By "communicating" do you just mean painting, or is there more to it? (I'm not asking you to submit another patch, just give me a quick answer if possible.)

I'm not a reviewer or committer of course, but these are my only remaining questions. After that the patch is fine with me.

[Levi Weintraub]

(In reply to comment #16)
> Levi, thanks for all the clarification.
> 
> In https://trac.webkit.org/wiki/LayoutUnit, for MathML using pixels you say:
> 
> "Using integers because subpixel positioning offers little benefit to MathML and can introduce uneven box sizes as they round."

Changed to "Continuing to use pixels now for ease of conversion. MathML should be revisited to really take advantage of sub-pixel layout after these changes are in."

> 
> Could you change this to something like "Using integers for now to ease conversion."? I would love to analyze the uneven box sizes comment, but if you don't want to discuss its pros & cons now then could you just remove it?
> 
> My only remaining quibble is in your ChangeLog comment you say "using pixel snapping when communicating with other Rendering classes". By "communicating" do you just mean painting, or is there more to it? (I'm not asking you to submit another patch, just give me a quick answer if possible.)

I only mean in requesting its offsetHeight and width, where I'm deliberately using the pixelSnapped values.

> 
> I'm not a reviewer or committer of course, but these are my only remaining questions. After that the patch is fine with me.

[Dave Barton]

Excellent, thanks!

[Levi Weintraub]

(In reply to comment #18)
> Excellent, thanks!

Thanks for your feedback.

Now, pinging reviewers :)

[Levi Weintraub]

(In reply to comment #19)
> (In reply to comment #18)
> > Excellent, thanks!
> 
> Thanks for your feedback.
> 
> Now, pinging reviewers :)

And again :)

[Eric Seidel (no email)]

Comment on attachment 130733 [details]
Patch

LGTM.

[Levi Weintraub]

Comment on attachment 130733 [details]
Patch

Thanks Eric!

[WebKit Review Bot]

Comment on attachment 130733 [details]
Patch

Clearing flags on attachment: 130733

Committed r110496: <http://trac.webkit.org/changeset/110496>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.