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

Added support for \not #140

Merged
merged 5 commits into from Aug 23, 2017
Merged

Conversation

kevinbarabash
Copy link
Member

@kevinbarabash kevinbarabash commented Oct 1, 2014

Addresses issue #29. Requires a Huxley test which I will try to add tomorrow. Here's a screenshot:
screen shot 2014-09-30 at 9 31 54 pm

TODO:

  • rebase
  • switch the order so the solidus comes first in the span and is shifted over with CSS
  • figure out how to make firefox render \not{abc} correctly

src/buildTree.js Outdated
var not = buildCommon.mathrm(
"\\not", group.mode, options.getColor(), ["mrel"]);

return makeSpan(["mord mrel"], [inner, not]);
Copy link
Member Author

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.

@kevinbarabash
Copy link
Member Author

We'll handle this using macros.

@gagern
Copy link
Collaborator

gagern commented Jul 8, 2015

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 \not macro look like?

@kevinbarabash
Copy link
Member Author

I called the symbol the wrong thing, but I added a symbol for the long solidus. The not method in buildTree.js places one the long solidus after the inner symbol. It seems like a we could have a macro like \def\not#1{\longSolidus #1}. I just made up \longSolidus so maybe this isn't possible.

@gagern
Copy link
Collaborator

gagern commented Jul 9, 2015

\def\not#1{\longSolidus #1} could almost be abbreviated to \let\not=\longSolidus except for the fact that \not{ab} is essentially \longSolidus{a}b which shouldn't make a difference. In LaTeX the definition of \not is \not=\mathchar"3236 and it is of type \mathrel. Looking at it this way, just your modifications to symbols.js should be sufficient to emulate LaTeX. Anything more fancy might be well intended, but actually contrary to how LaTeX does things, leading to incompatibilities.

@kevinbarabash
Copy link
Member Author

In that case I'm going to re-open this.

@kevinbarabash kevinbarabash reopened this Jul 9, 2015
@sophiebits
Copy link
Contributor

Maybe-stupid question: why is this its own command type instead of just being one of the supported symbols?

@kevinbarabash
Copy link
Member Author

@spicyj the \not glyph gets rendered after the rel that follows it in the source code so there needs to be some processing in buildTree.js (now buildHTML.js) to handle this. The macro I was suggesting in a previous comment should've been \def\not#1{#1 \longSolidus}. Sorry about the confusion.

@sophiebits
Copy link
Contributor

Oh! I see.

@gagern
Copy link
Collaborator

gagern commented Jul 15, 2015

But if \longSolidus overlaps the preceding character, then it behaves differently from the \not in LaTeX. In particular it will be aligned to the right edge of the group it affects, not the left edge. Perhaps we should add kernings so that the glyph is shifted to affect the following character? Or edit the font to achieve that?

@kevinbarabash
Copy link
Member Author

@gagern because the \longSolidus occurs after the inner in the span it should already be right aligned. That glyph has funky metrics which shift it to the left by its width.

@gagern
Copy link
Collaborator

gagern commented Jul 15, 2015

… it should already be right aligned.

Yes, and that's not consistent with TeX. In LaTeX, \not{abc} will essentially be {\not a}bc, while your {abc}\longSolidus would look like ab{\not c}. Sure, with multi-character groups we could split up the group and make this a\longSolidus bc, but the same problem holds for symbols of different widths, and for more complicated things in that group following the \not.

@kevinbarabash
Copy link
Member Author

@gagern sorry, reading too quickly. I can flip the order and shift it forward.

@@ -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);
Copy link
Member Author

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>&#x29F8;</mtext>
    </mpadded>
  </mrow>
  <mrow class="MJX-TeXAtom-ORD">
    <mi>a</mi>
    <mi>b</mi>
    <mi>c</mi>
  </mrow>
</math>

@kevinbarabash
Copy link
Member Author

The order is correct now. It's too bad firefox doesn't want to render it correctly.

@xymostech
Copy link
Contributor

@kevinbarabash do you know why that is? Is it just failing to render the font correctly or something?

@kevinbarabash
Copy link
Member Author

@xymostech I'm not sure. It renders everything correctly except for \not{abc}. For some reason the solidus is missing in this case.

@sthoch
Copy link

sthoch commented Feb 23, 2016

The solidus is there in my Firefox 44.0.2 (OS X 10.9.5).

@kevinbarabash
Copy link
Member Author

@sthoch this pull request is to add it as a combining mark.

@sthoch
Copy link

sthoch commented Feb 29, 2016

Sorry, let me clarify: If I take your PR and use it to render \not{abc} with my Firefox, the solidus is there and not missing. I thought that was the problem.

@kevinbarabash
Copy link
Member Author

@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?

@sthoch
Copy link

sthoch commented Feb 29, 2016

Firefox 44.0.2 (OS X 10.9.5).

@kevinbarabash kevinbarabash force-pushed the not_command branch 2 times, most recently from 9da8bb6 to bafa6e7 Compare July 3, 2017 01:09
@@ -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
Copy link
Member Author

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>.

@kevinbarabash
Copy link
Member Author

I've changed the approach significantly. Instead having a specific not group type and \\not function, buildExpression processes \\not groups and makes necessary changes to the following group.

I left a number of TODOs for subsequent PRs.

@kevinbarabash
Copy link
Member Author

not

@kevinbarabash
Copy link
Member Author

The handling of \\not with ords can definitely be improved, but it seems like a bit of an edge case as \\cancel is a better option for ords.

// 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'.
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'll take care of these in a separate PR.

@kevinbarabash
Copy link
Member Author

@gagern could you have another look at this?

@kevinbarabash kevinbarabash dismissed gagern’s stale review August 11, 2017 22:34

to encourage other people with push access to review

@kevinbarabash
Copy link
Member Author

Looks like I'll have to update the screenshots for this.

Copy link
Member

@edemaine edemaine left a 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
Copy link
Member

Choose a reason for hiding this comment

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

it's -> its

Can this same approach be used for #802 and #735?

Copy link
Member Author

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.

Copy link
Member

@edemaine edemaine left a 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.

@edemaine
Copy link
Member

For some reason I'm unable to actually merge this. Feel free to do so if you can...

@kevinbarabash
Copy link
Member Author

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?

@edemaine
Copy link
Member

I did, but it wasn't clickable, which I hadn't seen before. Presumably a temporary glitch. Anyway, glad you fixed it.

@kevinbarabash kevinbarabash deleted the not_command branch September 15, 2017 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants