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.

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

Update check for spacelike mml nodes to be more sensible. (mathjax/MathJax#3098) #1002

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Sep 14, 2023

This PR adjusts the way that mspace and mtext nodes are treated as spacelike. The spec says they are always spacelike, but that causes all kinds of problems, like making {\text{a}=\text{b}} being treated as an embellished operator. This interferes with line-breaking, for example, since a break at the equal sign will be moved to outside the text elements, which is not what is usually wanted. Similarly,

<math>
  <mtext>a</mtext>
  <mspace linebreak="newline">
  <mo>=</mo>
  <mtext>b</mtex>
</math>

is also an embellished operator, and so treated as a single unit.

This PR makes mtext be spacelike only if it consists only of spaces, and doesn't have styles or background colors. Similarly, mspace will not be spacelike if it has an explicit linebreak attribute (or if it is not breakable due to other attributes). This is not according to the spec, but makes more sense, particularly for how we use mtext and mspace in translating TeX to MathML.

Resolves part of issue mathjax/MathJax#3098.

@dpvc dpvc requested a review from zorkow September 14, 2023 15:04
@dpvc dpvc added this to the v4.0 milestone Sep 14, 2023
Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment.
O/w fine.

const spaces = !!this.getText().match(/^\s*$/);
return spaces && (attributes.getExplicit('mathbackground') === undefined &&
attributes.getExplicit('background') === undefined &&
attributes.getExplicit('style') === undefined);
Copy link
Member

@zorkow zorkow Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should those attributes go into a dedicated constant, in case we need more or need to change them in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of places where this type of check is being made, and I was thinking about making a service function to check them. I'll look into that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added hasOneOf() to the Attributes object to handle this, and made lists for the styles to use for space and mtext elements. I also added a hasExplicit() method to the Attributes object to make checking for explicit attributes more natural, and unset() to make removing an attribute easier. I edited a number of other files to use these new functions. So there are a bunch of new edits here. Do you want to review again?

Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@dpvc dpvc merged commit ca731d6 into develop Sep 19, 2023
@dpvc dpvc deleted the issue3098b branch September 19, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants