Skip to content

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

Merged
k4b7 merged 3 commits intoKaTeX:masterfrom
xsznix:master
Aug 16, 2017
Merged

Fix color support for stretchy, strikethrough, and fbox#792
k4b7 merged 3 commits intoKaTeX:masterfrom
xsznix:master

Conversation

@xsznix
Copy link
Copy Markdown
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
Copy Markdown

khanbot commented Aug 13, 2017

CLA signature looks good 👍

@ronkok
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

@k4b7
Copy link
Copy Markdown
Member

k4b7 commented Aug 16, 2017

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.

@k4b7
Copy link
Copy Markdown
Member

k4b7 commented Aug 16, 2017

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

6 participants