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

Phase out *-baseline in table as IE doesn't follow this part of the SVG standard #2076

Merged
merged 3 commits into from
Oct 10, 2017

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Oct 9, 2017

Local, recent Firefox doesn't adhere to alignment-baseline here, this tests effects on CI.

@monfera monfera self-assigned this Oct 9, 2017
@monfera monfera added status: in progress bug something broken labels Oct 9, 2017
@monfera monfera force-pushed the table-firefox-baseline branch from 1fd18c3 to 7df8c2b Compare October 9, 2017 21:54
@monfera
Copy link
Contributor Author

monfera commented Oct 9, 2017

It fixed the local FF misalignment of unwrapped lines:

image

... and it didn't break the same thing which evidently worked fine on CI (test cases intact).

Also, adding a similar fix to parcoords, where the effect of the problem (locally) was much less noticeable, and similarly, it worked fine on CI.

So I'm proposing this for merging.

@monfera
Copy link
Contributor Author

monfera commented Oct 9, 2017

Thanks @alexcjohnson for bringing up an issue seen in CI (separate one, not dealt with by this PR) that led to to these FF problems.

@alexcjohnson
Copy link
Collaborator

I do like those mocks a bit better, but now I'm rather confused... parcoords plots look fine in IE11 but according to a quick check on browserstack neither alignment-baseline nor dominant-baseline works there? How is that possible? Suffice it to say it doesn't seem to me like these attributes (or styles?) are really ready for prime time. How hard would it be to 🔪 them entirely?

dominant baseline chrome - http://bl.ocks.org/eweitnauer/7325338 (assumed to be the authority)
dominant baseline chrome

dominant baseline ff - seems to work but somewhat differently (baseline-shift fails, dominant-baseline has small differences)
dominant baseline ff

dominant baseline ie11 - fail
dominant baseline ie11

alignment baseline chrome - https://bl.ocks.org/emmasaunders/0016ee0a2cab25a643ee9bd4855d3464
alignment baseline chrome

alignment baseline ff - fail
alignment baseline ff

alignment baseline ie11 - fail
alignment baseline ie11

@monfera
Copy link
Contributor Author

monfera commented Oct 9, 2017

Indeed! This says it's not supported by IE: https://msdn.microsoft.com/en-us/library/gg558060(v=vs.85).aspx

Looks like we need to do the dy: 0.35em trick for central/middle and something analogous for hanging.

@monfera monfera force-pushed the table-firefox-baseline branch from d7e2d9d to ea94d21 Compare October 10, 2017 08:30
@monfera
Copy link
Contributor Author

monfera commented Oct 10, 2017

There are still some vertical misalignments specifically in CI, adding a separate item. The purpose of this ticket is, after @alexcjohnson 's IE testing, merely the replacement of *-baseline attributes with something even IE adheres to. This is a big improvement because, without this PR, Firefox and especially IE rendering have serious alignment problems so its separate merge is suggested.

@monfera monfera changed the title Switch to dominant-baseline in table Phase out *-baseline in table as IE doesn't follow this part of the SVG standard Oct 10, 2017
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Excellent, thanks. 💃 And thanks for calling out the remaining alignment CI issues in #2056

@monfera monfera merged commit c3f0a18 into master Oct 10, 2017
@monfera monfera deleted the table-firefox-baseline branch October 10, 2017 14:58
@AlexVvx AlexVvx mentioned this pull request Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants