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

Change frac-line from border to full span #976

Merged
merged 8 commits into from
Nov 24, 2017
Merged

Change frac-line from border to full span #976

merged 8 commits into from
Nov 24, 2017

Conversation

ronkok
Copy link
Collaborator

@ronkok ronkok commented Nov 20, 2017

Change frac-line and vertical-separator. Instead of using span borders to render the lines, fill an entire span using a box-shadow SVG.

This change will enable more dependable engagement of the min-height CSS.

Change `frac-line`  and `vertical-separator`.  Instead of using span borders, fill entire span using a `box-shadow`.

This change will enable more dependable engagement of the `min-height` CSS.
@marcianx
Copy link
Collaborator

FYI: I'd be wary of using box-shadow for anything since in Windows high contrast mode, browsers remove all box shadows (and also background colors/images). I've totally been bitten by this when working on making accessible web UIs.

@ronkok
Copy link
Collaborator Author

ronkok commented Nov 20, 2017

Pity. I already knew that background colors were a bad idea when printing. I suppose I could use an SVG image. Well, this will take a little longer.

@edemaine
Copy link
Member

Is this true even if the settings are marked !important? (relevant to both backgrounds and shadow boxes) It seems a shame to have to use such advanced features for something as simple as a fraction... though I'm not coming up with an alternative?

@marcianx
Copy link
Collaborator

!¡mportant is specifically designed to override CSS precedence. I'm not aware of it offering any other use.

@edemaine
Copy link
Member

@marcianx It's at least sometimes relevant, but maybe only in ms-specific rules... See #716 (comment)

@ronkok
Copy link
Collaborator Author

ronkok commented Nov 20, 2017

Interesting.

I want to avoid background-color, even if there is a way to avoid the high contrast problem. When printing, the default setting is to omit background colors.

But regarding box-shadow, as @edemaine points out, the KaTeX CSS already includes

.katex * {
  -ms-high-contrast-adjust: none !important;
}

I just finished writing the code for a frac-line via inline SVG image. The code works fine, but I have not made a commit. First, I'll check if box-shadow works when high-contrast is set and also works when printing.

@ronkok
Copy link
Collaborator Author

ronkok commented Nov 20, 2017

High contrast results are in. The image on the left is the box-shadow method. On the right is the inline SVG image.

box shadow
SVG is the clear winner. I'll turn in a commit.

Thank you to @marcianx for the warning about high contrast.

@ronkok
Copy link
Collaborator Author

ronkok commented Nov 20, 2017

Well, now there’s a working PR and, thanks to @marcianx, we have overcome one hurdle. At this point, I am the wrong person to do any further detailed testing on this work. I have not been able to reproduce the problem on my own screen and I have never mastered Docker for the Screenshotter tests. Any help would be appreciated.

@trimbonz
Copy link

Both the box-shadow and SVG methods in this PR fix the issue for my old Android webView clients, that would previously not render any fraction lines up to and including API 19 (KitKat). Previously I had to tweak the line thicknesses manually to make fraction lines appear, but they all show up without further intervention after I started using the fraclinespan branch.

@ronkok
Copy link
Collaborator Author

ronkok commented Nov 22, 2017

@trimbonz Cool! Thank you for the test and for the report.

@kevinbarabash
Copy link
Member

@ronkok I can regenerate the screenshots.

["preserveAspectRatio", "xMinYMin slice"],
];
const svg = new domTree.svgNode([pathNode], attributes);
return buildCommon.makeSpan([className, "hide-tail"], [svg], options);
Copy link
Member

Choose a reason for hiding this comment

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

How is the height (or width) set for the ruleSpan?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The hide-tail class contains the CSS overflow: hidden;. So the height that gets rendered is the line.style.height set by buildHTML.makeLineSpan().

Class frac-line has always contained the CSS width: 100%;, so the width is being set the way it always has been set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarification: My response above pertained to \frac, \overline, and \underline. I did not change \rule in this PR, so it is still using a top-border to render a line.

@media screen and (-webkit-min-device-pixel-ratio: 2.0),
screen and (min-resolution: 192dpi) {
min-width: 0.5px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should .frac-line have its min-height set in a similar fashion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

frac-line already has that min-height set. That was the purpose of PR #931.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for refreshing my memory. 😅

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinbarabash
Copy link
Member

@ronkok thanks for making these changes.

@kevinbarabash kevinbarabash merged commit b2698d3 into KaTeX:master Nov 24, 2017
@ronkok ronkok deleted the fraclinespan branch November 24, 2017 16:45
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

5 participants