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

Reference even more baseline concepts in CSS Inline Layout #5826

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

annevk
Copy link
Member

@annevk annevk commented Aug 14, 2020

See w3c/csswg-drafts#5312. And also #5429 for where all of this started.

Ideally though w3c/csswg-drafts#5431 would be fixed too and we'd figure out how to update images/baselines.png (I've asked Ian if there's an original somewhere).


/canvas.html ( diff )
/infrastructure.html ( diff )


/canvas.html ( diff )
/infrastructure.html ( diff )

@annevk
Copy link
Member Author

annevk commented Aug 14, 2020

Per @kojiishi's comment at w3c/csswg-drafts#5380 (comment) there might be normative changes warranted here depending on which browser we want to follow.

cc @jfkthame

@annevk
Copy link
Member Author

annevk commented Aug 24, 2020

It seems the baseline image was introduced in 96b4346 and as far as I can tell it's not clear how it was generated. One option here would be to drop it in favor of relying even more on https://drafts.csswg.org/css-inline/#css-metrics which does not seem all bad. @fantasai @dauwhe @domenic thoughts?

@domenic
Copy link
Member

domenic commented Aug 24, 2020

In general I'd prefer if the image were maintained in CSS, but to my non-expert's eye, there's not a lot of overlap in the terminology and examples used in https://github.com/whatwg/html/blob/master/images/baselines.png vs. https://drafts.csswg.org/css-inline/#css-metrics. So, I'd appreciate it if someone was able to reassure me that the CSS spec was comprehensive and that we wouldn't be losing useful information by deleting the HTML spec's image.

@annevk
Copy link
Member Author

annevk commented Jun 2, 2021

@fantasai @dauwhe @jfkthame @whatwg/css ping. Would be nice to sort this as apparently there are compatibility issues as well, see #6731.

@annevk annevk added topic: canvas needs tests Moving the issue forward requires someone to write tests labels Jun 2, 2021
@annevk annevk force-pushed the annevk/more-baselines branch from cd94f0d to ea1745c Compare June 2, 2021 11:25
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented Oct 12, 2021

@fantasai @dauwhe @jfkthame @whatwg/css another ping. It'd be great to get some (additional) review.

@mfreed7
Copy link
Contributor

mfreed7 commented Jun 15, 2023

@fserb

@past past removed the agenda+ To be discussed at a triage meeting label Jun 15, 2023
@fserb
Copy link
Contributor

fserb commented Jul 7, 2023

Sorry for the delay.
@yiyix

@macnmm
Copy link

macnmm commented Aug 29, 2023

@himorin has informed me that I should provide a better illustration of various baselines to show the issue, so I will do so shortly.

@annevk
Copy link
Member Author

annevk commented Sep 18, 2023

That would be great!

@annevk
Copy link
Member Author

annevk commented Dec 2, 2024

@schenney-chromium is this something you could help review as part of your TextMetrics improvements?

@schenney-chromium
Copy link
Contributor

Yes, I'll review once I make sure I understand all of the context for the change.

@schenney-chromium
Copy link
Contributor

Making this change to unify the definition of top/em-over and bottom/em-under in the canvas text styles section to explicitly align with CSS is a good change to land. With the previously suggested edit and the fix for the image to correct the labels I would LGTM this.

Regarding whether the metrics explaining image we have is better/worse than the CSS image, one advantage of the existing canvas metrics image is the clear indication that glyphs can escape the em box. That's important because it motivates the various bounds methods on TextMetrics.

As a meta point, the definitions in CSS for em-over and em-under are explicitly not relevant to CSS, and are even derived values not present in fonts. We could move them from the CSS spec to the canvas spec, along with the method to derive them. My desire to do that does not block this PR.

See w3c/csswg-drafts#5312. And also #5429 for where all of this started.

Ideally though w3c/csswg-drafts#5431 would be fixed too and we'd figure out how to update images/baselines.png (I've asked Ian if there's an original somewhere).
@annevk annevk force-pushed the annevk/more-baselines branch from ea1745c to 3dd6ae1 Compare December 4, 2024 18:05
@annevk annevk marked this pull request as ready for review December 4, 2024 18:05
@annevk
Copy link
Member Author

annevk commented Dec 4, 2024

I edited the image locally by essentially replacing the text. Seems to look good enough.

I'm no longer sure why I marked this "needs tests" though.

Copy link
Contributor

@schenney-chromium schenney-chromium left a comment

Choose a reason for hiding this comment

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

LGTM. Nice image editing.

Of course, I need to do more to get write access.

@annevk annevk merged commit a97372b into main Dec 5, 2024
2 checks passed
@annevk annevk deleted the annevk/more-baselines branch December 5, 2024 06:43
@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants