Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider removing explicit mo@maxsize = "infinity" attribute? #107

Closed
fred-wang opened this issue Jun 19, 2019 · 3 comments
Closed

Consider removing explicit mo@maxsize = "infinity" attribute? #107

fred-wang opened this issue Jun 19, 2019 · 3 comments
Labels
MathML Core Issues affecting the MathML Core specification

Comments

@fred-wang
Copy link

This is the same as not specifying the attribute at all, so I'm not sure it's really useful now that mstyle@maxsize is disallowed per #1

That way the attribute can just be defined as a length-percentage like lspace/rspace/minsize
@fred-wang fred-wang added MathML Core Issues affecting the MathML Core specification need implementation update need resolution Issues needing resolution at MathML Refresh CG meeting need specification update Issues requiring specification changes need tests Issues related to writing WPT tests labels Jun 19, 2019
@davidcarlisle
Copy link
Collaborator

I can't see how this is useful at all, I'd remove it from core.

Seems to have been added in mathml 2.0 but I don't recall why, I'll see if the mail from that time is still available (but only to see if it should stay in full, I think with the general movement to remove named lengths from core it does not belong there at all)

@fred-wang fred-wang mentioned this issue Aug 22, 2019
Closed
@fred-wang
Copy link
Author

  • This is already removed from MathML Core and there are no test for that.
  • infinity does not have any effect on mo since it's equal to the default (mstyle@maxsize removed and no maxsize in the operator dictionary) ; removing it make it invalid so again equal to the default. So this is not testable.
  • Gecko does not parse "infinity"
  • WebKit seems to parse "infinity" but this does not have any effect, we should just remove it.
  • I don't see minsize/maxsize support in our Chromium implementation

So I think there is nothing to do here. Except that we should probably write tests for minsize/maxsize for other lengths.

@fred-wang fred-wang removed need resolution Issues needing resolution at MathML Refresh CG meeting need specification update Issues requiring specification changes need tests Issues related to writing WPT tests labels Oct 9, 2019
@fred-wang
Copy link
Author

consensus was to remove "infinity" from core, consistently with other named lengths.

caiolima pushed a commit to caiolima/webkit that referenced this issue Apr 11, 2020
Remove unnecessary explicit parsing for mo@maxsize value "infinity"
cae4b6d 
https://bugs.webkit.org/show_bug.cgi?id=202720

Patch by Delan Azabani <dazabani@igalia.com> on 2020-04-09
Reviewed by Frédéric Wang.

In MathML 2, the default mo@maxsize was infinity [1], unless some other
default was given by mstyle@maxsize [2]. The sole purpose of "infinity"
was to give authors a way to set mo@maxsize to infinity when some other
mstyle@maxsize was set.

MathML Core removes mstyle@maxsize [3][4], such that "infinity" has the
same semantics as any other missing or invalid mo@maxsize, so the spec
has been simplified to make infinity an anonymous value [5][6].

No functional change, because WebKit has never supported mstyle@maxsize
anyway. To verify that there's no functional change:

1.  Search for references to LengthType::Infinity, and observe that the
    mo@maxsize parser in MathMLOperatorElement::maxSize is the only
    place where a Length of ::type infinity is created
2.  Search for references to that method, and observe that the only
    caller (RenderMathMLOperator::maxSize) passes intMaxForLayoutUnit
    (infinity) to toUserUnits as the referenceValue
3.  Go to the definition of toUserUnits, and observe that the refer-
    enceValue is used as the ParsingFailed default
4.  Step 1 shows that no other attributes would be affected by removing
    LengthType::Infinity, and steps 2 and 3 show that mo@maxsize treats
    invalid values as infinity, therefore it's safe to remove both the
    "infinity" parsing code and the underlying LengthType variant

[1] https://www.w3.org/TR/MathML2/chapter3.html#id.3.2.5.2
[2] https://www.w3.org/TR/MathML2/chapter3.html#presm.mstyle
[3] https://mathml-refresh.github.io/mathml-core/#style-change-mstyle
[4] w3c/mathml#1
[5] https://mathml-refresh.github.io/mathml-core/#dictionary-based-attributes
[6] w3c/mathml#107

No new tests, because no functional change.

* mathml/MathMLElement.h: Remove LengthType::Infinity.
* mathml/MathMLOperatorElement.cpp:
(WebCore::MathMLOperatorElement::maxSize): Remove explicit branch on "infinity". Replace what remains with an equivalent cachedMathMLLength call.
* rendering/mathml/RenderMathMLBlock.cpp:
(WebCore::toUserUnits): Remove explicit branch on LengthType::Infinity.
* rendering/mathml/RenderMathMLOperator.cpp:
(WebCore::RenderMathMLOperator::maxSize): Update comment to refer to the default value in the same way as the spec.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@259785 268f45cc-cd09-0410-ab3c-d52691b4dbfc
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
@delan @webkit-commit-queue
Remove unnecessary explicit parsing for mo@maxsize value "infinity"
21d6639 
https://bugs.webkit.org/show_bug.cgi?id=202720

Patch by Delan Azabani <dazabani@igalia.com> on 2020-04-09
Reviewed by Frédéric Wang.

In MathML 2, the default mo@maxsize was infinity [1], unless some other
default was given by mstyle@maxsize [2]. The sole purpose of "infinity"
was to give authors a way to set mo@maxsize to infinity when some other
mstyle@maxsize was set.

MathML Core removes mstyle@maxsize [3][4], such that "infinity" has the
same semantics as any other missing or invalid mo@maxsize, so the spec
has been simplified to make infinity an anonymous value [5][6].

No functional change, because WebKit has never supported mstyle@maxsize
anyway. To verify that there's no functional change:

1.  Search for references to LengthType::Infinity, and observe that the
    mo@maxsize parser in MathMLOperatorElement::maxSize is the only
    place where a Length of ::type infinity is created
2.  Search for references to that method, and observe that the only
    caller (RenderMathMLOperator::maxSize) passes intMaxForLayoutUnit
    (infinity) to toUserUnits as the referenceValue
3.  Go to the definition of toUserUnits, and observe that the refer-
    enceValue is used as the ParsingFailed default
4.  Step 1 shows that no other attributes would be affected by removing
    LengthType::Infinity, and steps 2 and 3 show that mo@maxsize treats
    invalid values as infinity, therefore it's safe to remove both the
    "infinity" parsing code and the underlying LengthType variant

[1] https://www.w3.org/TR/MathML2/chapter3.html#id.3.2.5.2
[2] https://www.w3.org/TR/MathML2/chapter3.html#presm.mstyle
[3] https://mathml-refresh.github.io/mathml-core/#style-change-mstyle
[4] w3c/mathml#1
[5] https://mathml-refresh.github.io/mathml-core/#dictionary-based-attributes
[6] w3c/mathml#107

No new tests, because no functional change.

* mathml/MathMLElement.h: Remove LengthType::Infinity.
* mathml/MathMLOperatorElement.cpp:
(WebCore::MathMLOperatorElement::maxSize): Remove explicit branch on "infinity". Replace what remains with an equivalent cachedMathMLLength call.
* rendering/mathml/RenderMathMLBlock.cpp:
(WebCore::toUserUnits): Remove explicit branch on LengthType::Infinity.
* rendering/mathml/RenderMathMLOperator.cpp:
(WebCore::RenderMathMLOperator::maxSize): Update comment to refer to the default value in the same way as the spec.

Canonical link: https://commits.webkit.org/223145@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@259785 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
MathML Core Issues affecting the MathML Core specification
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@fred-wang @davidcarlisle