-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added support for \not #140
Conversation
src/buildTree.js
Outdated
var not = buildCommon.mathrm( | ||
"\\not", group.mode, options.getColor(), ["mrel"]); | ||
|
||
return makeSpan(["mord mrel"], [inner, not]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes an assumption that group.value.body is a "rel". It really could be anything else. LaTeX and MathJax allow this. What they end up doing is placing the combining long solidus overlay glyph infront. Because this glyph already includes metrics that cause it to be rendered over the previous glyph, producing similar behaviour in these edge cases will require extra work.
778093b
to
0a28485
Compare
We'll handle this using macros. |
I don't see how this would be handled with macros, particularly the interaction with the atom type. Can you illustrate that idea? What would the |
I called the symbol the wrong thing, but I added a symbol for the long solidus. The |
|
In that case I'm going to re-open this. |
Maybe-stupid question: why is this its own command type instead of just being one of the supported symbols? |
@spicyj the |
Oh! I see. |
But if |
@gagern because the |
Yes, and that's not consistent with TeX. In LaTeX, |
@gagern sorry, reading too quickly. I can flip the order and shift it forward. |
0a28485
to
0f6fce9
Compare
src/buildMathML.js
Outdated
@@ -454,6 +454,11 @@ groupTypes.phantom = function(group, options, prev) { | |||
return new mathMLTree.MathNode("mphantom", inner); | |||
}; | |||
|
|||
groupTypes.not = function(group, options) { | |||
// TODO: it's an operator first and find the correct "not" version of that operator | |||
return buildGroup(group.value.body, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case where there isn't a "not" version of whatever's in the body, I'll add the solidus in manually. This is what MathJax does:
<math xmlns="http://www.w3.org/1998/Math/MathML">
<mrow class="MJX-TeXAtom-REL">
<mpadded width="0">
<mtext>⧸</mtext>
</mpadded>
</mrow>
<mrow class="MJX-TeXAtom-ORD">
<mi>a</mi>
<mi>b</mi>
<mi>c</mi>
</mrow>
</math>
0f6fce9
to
6467588
Compare
The order is correct now. It's too bad firefox doesn't want to render it correctly. |
@kevinbarabash do you know why that is? Is it just failing to render the font correctly or something? |
@xymostech I'm not sure. It renders everything correctly except for |
The solidus is there in my Firefox 44.0.2 (OS X 10.9.5). |
@sthoch this pull request is to add it as a combining mark. |
Sorry, let me clarify: If I take your PR and use it to render |
@sthoch I didn't realize that you had tried this pull request out. I'll give it another try. It might have been a problem with the specific version of Firefox that I was using. What version are you using? |
|
6467588
to
36faf98
Compare
9da8bb6
to
bafa6e7
Compare
@@ -639,6 +639,9 @@ const buildExpression = function(expression, options) { | |||
const group = expression[i]; | |||
groups.push(buildGroup(group, options)); | |||
} | |||
|
|||
// TODO(kevinb): combine \\not with mrels and mords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MathJax combines \\not
with certain relation operators and outputs the combined HTML entity inside an <mo>
and for mords, it puts the HTML entity for not
after the identifier inside an <mi>
.
bafa6e7
to
6da473d
Compare
I've changed the approach significantly. Instead having a specific I left a number of TODOs for subsequent PRs. |
6da473d
to
ba8a56a
Compare
The handling of |
// Process \\not commands within the group. | ||
// TODO(kevinb): Handle multiple \\not commands in a row. | ||
// TODO(kevinb): Handle \\not{abc} correctly. The \\not should appear over | ||
// the 'a' instead of the 'c'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take care of these in a separate PR.
@gagern could you have another look at this? |
to encourage other people with push access to review
Looks like I'll have to update the screenshots for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One typo in the comments. And need to fix the screenshots (and maybe run the --diff
option to make sure changes are OK).
src/buildHTML.js
Outdated
// \u0338 is a combining glyph so we could reorder the children so | ||
// that it comes after the other glyph. This works correctly on | ||
// most browsers except for Safari. Instead we absolutely position | ||
// the glyph and set it's right side to match that of the other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could almost be used for those but those accents are centered horizontally over their contents which complicates matters.
6c5040b
to
98181f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the screenshots fixed themselves? Anyway, thanks for revitalizing this old PR, @kevinbarabash! A very useful feature.
For some reason I'm unable to actually merge this. Feel free to do so if you can... |
@edemaine the branch needed to be updated. Do you see an "Update branch" in your UI? |
I did, but it wasn't clickable, which I hadn't seen before. Presumably a temporary glitch. Anyway, glad you fixed it. |
Addresses issue #29. Requires a Huxley test which I will try to add tomorrow. Here's a screenshot:
TODO: