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

[css-inline-3] Shift top | bottom | center values from alignment-baseline to baseline-shift #5180

Closed
fantasai opened this issue Jun 7, 2020 · 10 comments

Comments

@fantasai
Copy link
Collaborator

fantasai commented Jun 7, 2020

In trying to harmonize CSS and SVG, we made the SVG alignment-baseline and baseline-shift longhands of CSS’s vertical-align property. Overall this seems to work pretty well, but I put the top and bottom (and the corresponding new center value) under alignment-baseline, but I'm wondering if maybe they're a better fit under baseline-shift? They don't exactly specify a baseline, and at least in CSS2 they were mutually exclusive with a shift value.

@fantasai fantasai added the css-inline-3 Current Work label Jun 7, 2020
@AmeliaBR
Copy link
Contributor

AmeliaBR commented Jun 7, 2020

They're definitely not baseline-shift values, either, they're not shifting relative to another alignment.

But syntactically, the important question is how they combine. Does it make sense to align and then shift?

E.g., vertical-align: text-bottom -2px has a meaning (align based on the text-bottom position, then shift by down by 2px). I think that vertical-align: top -2px makes sense, although I'm not sure if it would create layout loops. But what would vertical-align: text-bottom top mean?

I agree that these aren't a perfect fit for alignment-baseline, since they don't use baselines at all, they use line boxes. If it doesn't make sense to add a baseline shift in addition to one of these alignments, maybe they could be a separate set of values for vertical-align that replace the baseline-based alignments altogether? E.g., the syntax for vertical-align would be [<'baseline-source'> || <'baseline-shift'> || <'alignment-baseline'>] | <line-box-alignment>, where a line-box alignment keyword makes the others irrelevant.

@fantasai
Copy link
Collaborator Author

fantasai commented Jun 8, 2020

E.g., the syntax for vertical-align would be [<'baseline-source'> || <'baseline-shift'> || <'alignment-baseline'>] | , where a line-box alignment keyword makes the others irrelevant.

Happy to make that change, too. :) But regardless, it has to be stored in a longhand. I'd rather just add it to baseline-shift than create another longhand.

But what would vertical-align: text-bottom top mean?

It asks for top alignment within the container (line box in the case of an inline-level element, table row in the case of table cells) and sets the alignment baseline to the text-bottom baseline. If the row has no 'baseline'-aligned cells, then this is the alignment baseline that will get exported as the table row’s baseline. It's a weird thing to ask, but it has a meaning.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-inline-3] Shift top | bottom | center values from alignment-baseline to baseline-shift, and agreed to the following:

  • RESOLVED: accept change as proposed by fantasai unless strong impl argument
The full IRC log of that discussion <dael> Topic: [css-inline-3] Shift top | bottom | center values from alignment-baseline to baseline-shift
<dael> github: https://github.com//issues/5180
<dael> fantasai: top bottom and center values. center is new, top and bottom have been since css 1.
<dael> fantasai: When we pulled in alignment-baseline and basline-shift we made them longhands for vertical-align
<dael> fantasai: Weren't sure what to to with top and bottom b/c don't spec baseline or a shift exactly. put under alignment-baseline.
<dael> fantasai: Seemed to make more sense to go under baseline-shift property. Alignment baseline is a concept that exists for a lot of other boxes but top and bottom don't have a meaning.
<Rossen_> q?
<dael> fantasai: I was thinking would make more sense on baseline-shift which is how much to shift afte ryou align. In which case we ignore baselines b/c top and bottom don't care. THey use top and bottom of align subtree
<dael> florian: PoV from insie element top and bottom values don't combine with either, they take over. From PoV of element reltationship with parents it does make sense to continue applying alignment-baseline even if top and bottom shifted. Am I getting that right?
<dael> fantasai: Yeah. Prob only case for both is table cells where top and bottom values trigger behavior on align-content. In that case if you have a table with a single table cell and if you veritical=align top the baseline still has a meaning to allowt able to align to. In which case we have to export a baseline so there's a meaning to having both values
<dael> fantasai: Within inline layout top and bottom has no ability to combine with aligned baseline or baseline shift.
<dael> florian: Another approach is from cascade and setting. Most of the time you set in shorthand. If set separate having some feeling that align-baseline is based on broad context and you could be doing this for whole doc but locally switch to top and bottom
<dael> florian: It's a good fit for neither, but I weak lean toward your proposal
<dael> AmeliaBR: What's our impl status? Are we asking impl to change shipped things or is all still new stuff adding extra vertical-align keywords
<dael> florian: I don't think impl have switched to long hands except in SVG which doesn't have these keywords.
<dael> fantasai: I don't think alignment-baseline is in FF
<dael> AmeliaBR: THat' smy only concern. If we're changing after something has shipped not worth changing. If nothing has shipped I agree with fantasai this makes more sense
<dael> florian: MDN doesn't seem to know these have shipped, but it has a ?
<dael> fremy: We can check
<dael> fantasai: I can't get FF to do anything with alignment-baseline
<dael> Rossen_: Doesn't sound like strong impl constraints
<dael> fremy: Not able to get FF to do something and I'm in FF dev.
<dael> florian: Alternative would be spawn another long hand but I'm not sure I'm excited about it
<dael> AmeliaBR: I suggested if these override both longhands but it gets confusing from cascade PoV
<dael> florian: It can't do something in the shorthand without doing something in long hand
<AmeliaBR> Confirmed that Chromium doesn't support these keywords in alignment-baseline
<dholbert> according to this code, alignment-baseline is indeed not currently supported in Firefox: https://searchfox.org/mozilla-central/rev/eef39502e08bcd3c40573c65a6827828dce4a032/dom/svg/SVGElement.cpp#974-975
<dael> fantasai: Values are mutually exclusive with baseline-shift. You can't do shift and do top and bottom. B/C mutually exclusive might as well be same property
<dael> fantasai: Does anyone have other comments or should we accept and move on? There's arguments in favor of change and none against
<dael> dbaron: Seems only sort of exclusive. You can do top and down but not top and up
<dael> faceless2_: Spec says you can't combine them
<dael> florian: Browsers haven't impl syntax. But even if they did does it mean anything?
<dael> fantasai: Can't really combine. If you want to shift there's a lot of other ways.
<dael> dbaron: Okay with it. Makes sense to forbid it
<dael> Rossen_: Seems like leaning toward forbidding it.
<dael> Rossen_: astearns pointed out....
<dael> florian: Have clarity, but not enthusiasm. Everyone leaning in that direction
<faceless2_> Test: https://jsbin.com/hodevav/edit?html,output
<dael> AmeliaBR: Resolve to accept change as proposed by fantasai unless strong impl argument?
<dael> Rossen_: That's the proposal, yes
<dael> Rossen_: Objections?
<dael> RESOLVED: accept change as proposed by fantasai unless strong impl argument

@AmeliaBR
Copy link
Contributor

For the spec prose, I'd suggest describing these as “shifting to” values as opposed to “shifting by” values. Whatever the initial alignment would be, you then shift until tops/bottoms/centers match.

@fantasai
Copy link
Collaborator Author

@AmeliaBR rewrote intro paragraph as

This property specifies the box’s post-alignment shift. The baseline-relative values , sub, super shift the box relative to its baseline-aligned position, whereas the line-relative values top, center, and bottom shift the inline box and its contents relative to the bounds of its line box.

I'm not convinced the section overall reads that great, there's a ton of visual noise in that paragraph as a result of marking up terms/code/etc. Let me know if you have any suggestions for making it better. :)

@frivoal
Copy link
Collaborator

frivoal commented Jun 18, 2020

I'm thinking that it might be better for the note that's at a7b0a94#diff-81193009bbd77efc16db63e0098a69baR665 to be moved to 4.2.3, between the first paragraph and the advisement.

@fantasai
Copy link
Collaborator Author

@frivoal middle is not a baseline-shift value, but an alignment-baseline value.

@frivoal
Copy link
Collaborator

frivoal commented Jun 18, 2020

Ah, right. Then probably the note should read:

[…]
and ''vertical-align/center''''vertical-align/middle'' (''alignment-baseline: middle'') to ''align-content/center''.
[...]

If not, which part of the spec (other than that note) says that vertical-align: center maps to alignment-baseline: middle?

@fantasai
Copy link
Collaborator Author

@frivoal :/ Fixed. Thanks. :)

@fantasai
Copy link
Collaborator Author

There's some follow-up nonsense wrt table cells in #5234, but calling this fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants