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

Fix color support for stretchy, strikethrough, and fbox #792

Merged
merged 3 commits into from Aug 16, 2017

Conversation

xsznix
Copy link
Collaborator

@xsznix xsznix commented Aug 13, 2017

Summary:
Stuff like \red{\overbrace{AB}} works now in addition to \color{red}{\overbrace{AB}}. Strikethrough now respects color. The Firefox in the screenshotter doesn't seem to support background-image + mask, but I manually tested that the latest Firefox does.

Test plan:
Ran make, then tested in latest Chrome and Firefox to ensure color support was working, then ran make screenshots.

Summary:
Stuff like `\red{\overbrace{AB}}` works now in addition to `\color{red}{\overbrace{AB}}`. Strikethrough now respects color. The Firefox in the screenshotter doesn't seem to support `background-image` + `mask`, but I manually tested that the latest Firefox does.

Test plan:
Ran `make`, then tested in latest Chrome and Firefox to ensure color support was working, then ran `make screenshots`.
@khanbot
Copy link

khanbot commented Aug 13, 2017

CLA signature looks good 👍

@ronkok
Copy link
Collaborator

ronkok commented Aug 13, 2017

Interesting. I did not realize that KaTeX supported that syntax. It does, though. \red{A} renders a red A.

Are there any LaTeX packages that support this syntax?

@edemaine
Copy link
Member

Nor did I! Here's the code. And I guess there are matching katex-... colors in the CSS. I'm not aware of a LaTeX package implementing this -- probably a KA thing. IMO, these might make more sense as macros...

@ronkok
Copy link
Collaborator

ronkok commented Aug 13, 2017

Ah, I see. Well, @xymostech has written issue #28, which states that these custom colors were meant to be KA-only and not available for everyone. There is some push back in the discussion.

I suppose one approach would be to accept this PR, so that KaTeX has consistent behavior. Then, if a PR is ever written to resolve #28, it would have to address the whole issue.

@sophiebits
Copy link
Contributor

Yes, the intent was always to move those to macros as soon as was possible.

I think this change seems pretty unambiguously good especially in the existing presence of the options.getColor method although I don't understand the significance of the CSS change.

@ronkok
Copy link
Collaborator

ronkok commented Aug 13, 2017

The CSS change fixes a bug which has crept into function stretchy.encloseSpan(). At one time, I pushed mask onto the list of classes for the relevant span. Somehow, I let that item slip out. The CSS in this PR corrects for my error.

Thank you, @xsznix!

Also, Firefox 53 was the first Firefox release with full mask support. I suppose Screenshotter is using an older version.

@edemaine
Copy link
Member

In this case, perhaps we should accept this PR and then make a new one to rewrite \red etc. in terms of macros? I could use some clarification on whether those macros belong in KaTeX (e.g. for backwards compatibility) or in another KA project.

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. Thanks for the fixes, @xsznix!

@kevinbarabash
Copy link
Member

In this case, perhaps we should accept this PR and then make a new one to rewrite \red etc. in terms of macros?

I think that makes sense.

I could use some clarification on whether those macros belong in KaTeX (e.g. for backwards compatibility) or in another KA project.

As per the comments in #28, we definitely want to move KA custom colors out of KaTeX but in a thoughtful way.

@kevinbarabash
Copy link
Member

@xsznix thanks for the PR. @edemaine thanks for reviewing.

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.

None yet

6 participants