Implicit \color, explicitly grouped \textcolor #619

edemaine · 2017-01-10T22:37:02Z

I've implemented \color and \textcolor to behave as they do in LaTeX (specifically, the color package):

{\color{red}This is red.}
\textcolor{red}{This is also red}

Sadly, this differs from current KaTeX and MathJax which would only make the T red in the first example (as if \color were actually \textcolor). So this change may break some existing code. But I think it's for the better, as LaTeX should be the master reference here.

kevinbarabash · 2017-01-10T23:28:24Z

~~I didn't realize there was a color package.~~ I forgot that there's a color package. It's too bad about the difference in behavior. I think we want this, we just need to communicate the breaking change along with suggested workarounds.

kevinbarabash · 2017-01-10T23:31:41Z

color package reference: http://ctan.mirror.rafal.ca/macros/latex/required/graphics/color.pdf

edemaine · 2017-01-10T23:46:41Z

As I understand it, this is the only way to change color in LaTeX. There are other packages, e.g. xcolor, but they build on the same basic interface.

xymostech · 2017-01-11T00:06:09Z

This is admittedly selfish, but KA has a ton of math expressions that would break because of this change. Also, our previous \color function was compatible with MathJax, which was the reason we implemented it the way we did.

edemaine · 2017-01-11T00:12:08Z

FWIW, MathJax defines a color extension that changes the behavior to match LaTeX. I suppose KaTeX could do the same... though personally I think it makes more sense to match LaTeX and MathJax+color.

(Also FWIW, to fix those KA expression, you just need to replace \color with \textcolor... But I don't know how hard that is to do globally.)

kevinbarabash · 2017-01-11T02:53:13Z

@xymostech we're already preprocessing our math so it shouldn't be hard to replace \color with \textcolor.

edemaine · 2017-01-11T14:06:51Z

@xymostech Another thing we could do is add an oldColor or colorIsTextColor option to the options set, to enable backwards compatibility mode...

gagern · 2017-01-20T15:53:53Z

I guess you could also declare a macro \color which expands to \textcolor. That way you don't have to go poking around in some string literal which might one day contain things like \verb and \def and whatnot, but could still override \color to behave like \textcolor on a per-size basis.

Nevertheless, this change should come with a bump to 0.8.0, not in the 0.7 line of releases.

gagern · 2017-01-20T15:56:09Z

test/screenshotter/ss_data.yaml

    nolatex: different syntax and different scope
-ColorSpacing: \color{red}{\displaystyle \int x} + 1
+ColorSpacing: \textcolor{red}{\displaystyle \int x} + 1
+ColorImplicit: \color{red}red\color{blue}blue


I think this test should ensure that a) ending a group restores previous color and b) ending a \textcolor does return to the previous implicit color. Something like

bl{ack\color{red}red\textcolor{green}{green}red\color{blue}blue}black

Good point, and nice test! Changed.

edemaine · 2017-01-20T21:18:00Z

@gagern Good idea. We could suggest defining a macro '\color': '\textcolor' in release notes to 0.8.0, assuming it gets approved at all. I'll just reiterate that I think it's good to use LaTeX as the reference model. Even if MathJax is the reference model, it offers both options.

gagern · 2017-01-20T22:52:57Z

Code looks good to me. I'd like a go-ahead from @xymostech, though, to make sure the workaround for the incompatibility is feasible for KA.

edemaine · 2017-06-10T20:09:29Z

@xymostech or @kevinbarabash, could we make a decision here? I see two reasonable options:

Leave \color as is (two-argument function) because it's compatible with MathJax without any flags. But provide an option that switches \color to behave as in LaTeX, just like MathJax does.
Change \color to make it compatible with LaTeX (one-argument function changing current group). But provide an option that switches \color to behave as in MathJax (i.e. old behavior) for (somewhat manual) backwards compatibility.

Option 1 has the advantage of backwards compatibility without option changing. Option 2 is preferable to me because KaTeX's mantra is/should be "renders just like LaTeX". But it would probably require a significant version increment.

In both cases, we can provide \textcolor which behaves just like LaTeX, because that should unambiguously be a two-argument function.

kevinbarabash · 2017-06-10T20:55:06Z

@edemaine let's go with option 2. Will want to bump the version number to 0.8.x when we do a publish that includes this change.

edemaine · 2017-06-10T21:00:09Z

Cool, thanks, @kevinbarabash ! That's almost what this PR does. Preference on calling the option oldColor or colorIsTextColor or something else?

kevinbarabash · 2017-06-10T21:12:23Z

colorIsTextColor is more descriptive so my vote would be for that.

edemaine · 2017-06-10T23:28:14Z

4dd21a7 adds a colorIsTextColor, documentation in README, and parsesLike tests that confirm the exact behavior of \color in the three cases (no specification, and colorIsTextColor set to false or true).

kevinbarabash

LGTM

kevinbarabash · 2017-06-13T02:59:58Z

README.md

@@ -49,6 +49,7 @@ You can provide an object of options as the last argument to `katex.render` and
 - `throwOnError`: `boolean`. If `true`, KaTeX will throw a `ParseError` when it encounters an unsupported command. If `false`, KaTeX will render the unsupported command as text in the color given by `errorColor`. (default: `true`)
 - `errorColor`: `string`. A color string given in the format `"#XXX"` or `"#XXXXXX"`. This option determines the color which unsupported commands are rendered in. (default: `#cc0000`)
 - `macros`: `object`. A collection of custom macros. Each macro is a property with a name like `\name` (written `"\\name"` in JavaScript) which maps to a string that describes the expansion of the macro.
+- `colorIsTextColor`: `boolean`. If `true`, `\color` will work like LaTeX's `\textcolor`, and take two arguments (e.g., `\color{blue}{hello}`), which restores the old behavior of KaTeX (pre-0.8.0). If `false` (the default), `\color` will work like LaTeX's `\color`, and take one argument (e.g., `\color{blue}hello`).  In both cases, `\textcolor` works as in LaTeX (e.g., `\textcolor{blue}{hello}`).


kevinbarabash · 2017-06-13T03:04:09Z

src/functions.js

@@ -166,6 +166,14 @@ defineFunction("\\color", {
    };
 });

+// \color is handled in Parser.js's parseImplicitGroup


Thanks for commenting this. It would be nice to have a way to mark functions in this file as to whether or not the parser needs to the parse an implicit group for them.

Yes. @gagern and I had discussed this at some point, and it felt like all the implicit group parsing could be moved to functions.js. Worth considering in the future...

kevinbarabash · 2017-06-13T03:04:56Z

test/errors-spec.js

-                   " \\color{#̲f̲f̲f̲f̲f̲f̲{̲t̲e̲x̲t̲}");
+            expect("\\textcolor{#ffffff{text}").toFailWithParseError(
+                   "Invalid color: '#ffffff{text' at position 12:" +
+                   " \\textcolor{#̲f̲f̲f̲f̲f̲f̲{̲t̲e̲x̲t̲}");


I don't even know what that character is, lol.

kevinbarabash · 2017-06-13T03:08:47Z

test/katex-spec.js

+    it("should use two-argument \\color if requested", function() {
+        expect(oldColorExpression).toParseLike("\\textcolor{#fA6}{x}y", {
+            colorIsTextColor: true,
+        });


Nice tests!

kevinbarabash · 2017-06-13T03:09:51Z

test/screenshotter/ss_data.yaml

    nolatex: different syntax and different scope
-ColorSpacing: \color{red}{\displaystyle \int x} + 1
+ColorSpacing: \textcolor{red}{\displaystyle \int x} + 1
+ColorImplicit: bl{ack\color{red}red\textcolor{green}{green}red\color{blue}blue}black


kevinbarabash · 2017-06-13T03:12:18Z

@edemaine thanks for the PR. Sorry for taking so long to make a decision regarding \color's behavior.

edemaine · 2017-06-13T16:07:53Z

No worries -- big decisions are hard! Thanks for reviewing + merging.

edemaine mentioned this pull request Jan 10, 2017

katex package for LaTeX #272

Closed

gagern reviewed Jan 20, 2017

View reviewed changes

gagern approved these changes Jan 20, 2017

View reviewed changes

edemaine force-pushed the color branch from 62c5556 to f2afa81 Compare June 10, 2017 20:01

edemaine added 4 commits June 10, 2017 16:20

Implicit \color, explicitly grouped \textcolor

f350bdd

\color screenshot tests

c764cb9

\color -> \textcolor in tests

0a61cd0

More interesting \color test thanks to @gagern

70273c7

edemaine force-pushed the color branch from 75ffd2f to 70273c7 Compare June 10, 2017 20:20

Add colorIsTextColor option and corresponding tests

4dd21a7

kevinbarabash self-assigned this Jun 11, 2017

kevinbarabash added the GH Review: review-needed label Jun 11, 2017

kevinbarabash approved these changes Jun 13, 2017

View reviewed changes

kevinbarabash merged commit 25dde7f into KaTeX:master Jun 13, 2017

kevinbarabash added GH review: accepted and removed GH Review: review-needed labels Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implicit \color, explicitly grouped \textcolor #619

Implicit \color, explicitly grouped \textcolor #619

edemaine commented Jan 10, 2017 •

edited

kevinbarabash commented Jan 10, 2017 •

edited by edemaine

kevinbarabash commented Jan 10, 2017

edemaine commented Jan 10, 2017 •

edited

xymostech commented Jan 11, 2017

edemaine commented Jan 11, 2017 •

edited

kevinbarabash commented Jan 11, 2017

edemaine commented Jan 11, 2017

gagern commented Jan 20, 2017

gagern Jan 20, 2017

edemaine Jan 20, 2017

edemaine commented Jan 20, 2017

gagern commented Jan 20, 2017

edemaine commented Jun 10, 2017

kevinbarabash commented Jun 10, 2017

edemaine commented Jun 10, 2017

kevinbarabash commented Jun 10, 2017 •

edited

edemaine commented Jun 10, 2017

kevinbarabash left a comment

kevinbarabash Jun 13, 2017

kevinbarabash Jun 13, 2017

edemaine Jun 13, 2017

kevinbarabash Jun 13, 2017

kevinbarabash Jun 13, 2017

kevinbarabash Jun 13, 2017

kevinbarabash commented Jun 13, 2017

edemaine commented Jun 13, 2017

Implicit \color, explicitly grouped \textcolor #619

Implicit \color, explicitly grouped \textcolor #619

Conversation

edemaine commented Jan 10, 2017 • edited

kevinbarabash commented Jan 10, 2017 • edited by edemaine

kevinbarabash commented Jan 10, 2017

edemaine commented Jan 10, 2017 • edited

xymostech commented Jan 11, 2017

edemaine commented Jan 11, 2017 • edited

kevinbarabash commented Jan 11, 2017

edemaine commented Jan 11, 2017

gagern commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edemaine commented Jan 20, 2017

gagern commented Jan 20, 2017

edemaine commented Jun 10, 2017

kevinbarabash commented Jun 10, 2017

edemaine commented Jun 10, 2017

kevinbarabash commented Jun 10, 2017 • edited

edemaine commented Jun 10, 2017

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash commented Jun 13, 2017

edemaine commented Jun 13, 2017

edemaine commented Jan 10, 2017 •

edited

kevinbarabash commented Jan 10, 2017 •

edited by edemaine

edemaine commented Jan 10, 2017 •

edited

edemaine commented Jan 11, 2017 •

edited

kevinbarabash commented Jun 10, 2017 •

edited