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-text] text-align: match-parent on the root element with direction: rtl doesn't match browsers #6542

Closed
zcorpan opened this issue Aug 25, 2021 · 17 comments
Labels
Closed Accepted by CSSWG Resolution css-text-3 Current Work Tested Memory aid - issue has WPT tests

Comments

@zcorpan
Copy link
Member

zcorpan commented Aug 25, 2021

See this test: web-platform-tests/wpt#30175

If I understand correctly, a used value of 'direction: rtl' on the root element is propagated to the ICB per https://drafts.csswg.org/css-writing-modes-3/#icb

Then, https://drafts.csswg.org/css-text/#valdef-text-align-match-parent says

match-parent
This value behaves the same as inherit (computes to its parent’s computed value) except that an inherited value of start or end is interpreted against the parent’s (or the initial containing block’s, if there is no parent) direction value and results in a computed value of either left or right. When specified on the text-align shorthand, sets both text-align-all and text-align-last to match-parent.

The initial value of text-align is 'start', so I guess the inherited value for the root element has to be 'start'. Therefore, it looks at the 'direction' of the ICB, which is propagated from the root element, and is thus 'rtl'. Therefore text-align should compute to 'right'.

Here are the results of the test:

Firefox Nightly Safari TP Chrome Canary
expected "right" but got "start" expected "right" but got "left" expected "right" but got "left"

Is my reading of the spec correct?

Should the spec be changed to side with either Firefox or Safari/Chrome?

Also, the spec only says that the computed value is "either left or right", which doesn't say which value to use. I assume a mapping where direction: ltr -> left and direction: rtl -> right, but the spec should say so explicitly.

@zcorpan zcorpan added the css-text-3 Current Work label Aug 25, 2021
@frivoal
Copy link
Collaborator

frivoal commented Aug 26, 2021

Is my reading of the spec correct?

I think so.

Should the spec be changed to side with either Firefox or Safari/Chrome?

Is there any particular reason it would be difficult to fix the browsers? From a theoretical / author point of view, the specified behavior makes more sense, so I'd rather not give up on it too easily.

Also, the spec only says that the computed value is "either left or right", which doesn't say which value to use. I assume a mapping where direction: ltr -> left and direction: rtl -> right, but the spec should say so explicitly.

Isn't this clear enough:

…start or end is interpreted against the parent’s (or the initial containing block’s, if there is no parent) direction value and results in a computed value of either left or right.

Especially given that clicking through the definitions of start and end to css-writing-modes will tell you this about start:

The side from which text of the inline base direction would start. For boxes with a used direction value of ltr, this means the line-left side. For boxes with a used direction value of rtl, this means the line-right side.

and this about end:

The side opposite start.

I don't mind clarifying if it's not, but I'd rather avoid redundant normative text, and it does seem clear to me already.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 26, 2021

I don't expect it to be particularly hard to change implementations. What worries me more is getting 3 different behaviors and then it's never fixed in the other 2 browsers because it's too low prio. match-parent on the root doesn't make a whole lot of sense to begin with, so I think the shortest path to interop (Firefox changing to match WebKit/Chromium) seems like a good option.

A literal reading allows any behavior so long as the computed value is either left or right.

@frivoal
Copy link
Collaborator

frivoal commented Dec 23, 2021

A literal reading allows any behavior so long as the computed value is either left or right.

If it only said

results in a computed value of either left or right.

I'd agree with you, but right before that there's

start or end is interpreted against the parent’s […] direction value

which doesn't seem to allow "any behavior so long as the computer value is either left or right", but calls for a specific way to find whether it's left or right. At least as far as I can tell. Does it not?

@frivoal
Copy link
Collaborator

frivoal commented Dec 23, 2021

match-parent on the root doesn't make a whole lot of sense to begin with, so I think the shortest path to interop (Firefox changing to match WebKit/Chromium) seems like a good option.

Do we have any usage numbers on this case? My expectation would be that using it on the root would be pretty rare, but that when it is used there, the specified behavior does make more sense. However, if it's hardly even used, maybe it doesn't matter too much and we could indeed take the quickest path to interop?

@fantasai
Copy link
Collaborator

fantasai commented Apr 20, 2022

If it doesn't matter too much, then having interop or not doesn't matter either. I'd rather the spec defined reasonable, expected behavior so that when the relevant bits of code get rewritten they get rewritten to sensible behavior not some random "shortest path to interop on an insignificant problem back in 2022" behavior.

@fantasai
Copy link
Collaborator

Proposal: no change to spec, land the testcase as-is. @zcorpan do you have any objection to this?

@zcorpan
Copy link
Member Author

zcorpan commented Apr 27, 2022

If it doesn't matter too much, then having interop or not doesn't matter either.

I disagree, getting interop on the details even if the details are by themselves not important makes for a robust and predictable platform where developers can test what happens in one UA and expect it to work the same in others. Specifying something that no UA implements now means the first to conform will make interop worse and risk breaking some web content.

I object to this. Ways to resolve my objection:

  • get sign-off from relevant folks from at least two of Gecko, WebKit and Chromium that they want to change to conform to the spec. In this case, they should all have bugs filed to track the issue.
  • specify what WebKit and Chromium do. In this case, we should file a bug for Gecko.
  • edit: specify what Gecko does and get sign-off from at least one of WebKit and Chromium that they want to match Gecko. File bugs for WebKit and Chromium.

@emilio
Copy link
Collaborator

emilio commented May 25, 2022

For the record gecko behavior is intentional, see:

https://searchfox.org/mozilla-central/rev/9902932742fcdce2c956eeb81fd38350f5394ab2/servo/components/style/values/specified/text.rs#558-566

                // on the root <html> element we should still respect the dir
                // but the parent dir of that element is LTR even if it's <html dir=rtl>
                // and will only be RTL if certain prefs have been set.
                // In that case, the default behavior here will set it to left,
                // but we want to set it to right -- instead set it to the default (`start`),
                // which will do the right thing in this case (but not the general case)
                if _context.builder.is_root_element {
                    return TextAlignKeyword::Start;
                }

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed text-align: match-parent.

The full IRC log of that discussion <TabAtkins> Topic: text-align: match-parent
<TabAtkins> florian: match-parent on text-align does as the name implies, matches the parent's alignment
<TabAtkins> florian: Different form just inheriting, as the meanign of start and end might have switched if the direction switches. With match-parent it doesn't flip, it def matches the parent
<TabAtkins> florian: So the question is what to do on the root. The spec has an answer that I think makes sense, but doesn't have interop.
<TabAtkins> florian: But not sure I'm aware of significant real problems triggered by this, so if we have a sensible behavior in the spec that doesn't cause real issues I think it's okay to leave it as-is and consider non-compliance a bug, but zcorpan didn't see it that way
<TabAtkins> fantasai: zcorpan wanted us to either change the spec to match impls, or get a commitment from browser engines that they want to fix this bug
<TabAtkins> fantasai: My personal take is that this is a very low-prio fix, but it should be considered a bug so when someone is fixing that area of the code they could fix it too.
<TabAtkins> fantasai: Don't think we should require anyone to prioritize fixing it. Super low prio, possibly lowest prio I've ever seen. But don't think we should change the spec, I think the spec is more correct.
<TabAtkins> astearns: Assume there's not bugs filed on this?
<TabAtkins> florian: Not sure.
<TabAtkins> astearns: So part of zcorpan's request is to get bugs filed. I agree with him.
<TabAtkins> fantasai: I can take an action to make sure bugs are filed.
<TabAtkins> astearns: And once bugs are filed, we'll see if we can get evals from impls on the bugs themselves.
<TabAtkins> astearns: But like fantasai said, don't think a timely fix is strictly required for this.
<TabAtkins> fantasai: Well is it okay to close the bug, with the understanding that we have bugs filed and impls plan to fix it at some point?
<TabAtkins> astearns: Right, let's file the bugs and get to the next part
<TabAtkins> emilio: Gecko behavior here is intentional. I'd need to investigate why it's a special case, I didn't write the code.
<TabAtkins> florian: That's relevant, if browsers have specific reasons for differeing from the spec it would be good to report so we can see if the spec should reflect that. But if it's just accidental we should know that too, so we can be confident of keeping the current spec.
<TabAtkins> astearns: So next step is writing bugs.

@frivoal
Copy link
Collaborator

frivoal commented May 25, 2022

but the parent dir of that element is LTR even if it's <html dir=rtl>

Is that spec compliant? Unless I'm confused, that seems to go against https://www.w3.org/TR/css-writing-modes-3/#icb

Either way, the keyword that gecko resolves to is start rather than right, but that does put it on the right side, so it's more spec compliant than Chrome & Safari, isn't it?

@emilio
Copy link
Collaborator

emilio commented May 25, 2022

@frivoal propagation to the icb is done after style resolution, though, so it can't really be used to deal with this. We could use the root element's writing-mode itself, but that wouldn't match the spec either because the principal writing mode is propagated from the body (and we don't have the body style when computing the root style for obvious reasons, you need the root style for the <body> to inherit from).

I tracked that bit of code down through blame. Reasoning for our behavior is here:

Maybe the spec didn't say anything about the ICB back then? I think making it compute to start is the most reasonable thing FWIW.

@emilio
Copy link
Collaborator

emilio commented May 25, 2022

https://bugzilla.mozilla.org/show_bug.cgi?id=1341714#c25 is additional conversation here.

@zcorpan
Copy link
Member Author

zcorpan commented May 26, 2022

A third way to resolve my objection is to specify what Gecko does and get sign-off from at least one of WebKit and Chromium that they want to match Gecko.

@fantasai
Copy link
Collaborator

I think making it compute to start is the most reasonable thing FWIW.

I think the problem with that is that match-parent everywhere else computes to an absolute side, and doesn't change sides as it inherits through descendants. I think it's better for it to compute to one of 'left' or 'right', therefore. If we can't make it compute against the ICB when specified on the root, I think making it compute against the root itself is fine. (It'll be a bit weird if someone sets a different writing mode on BODY, but they shouldn't be doing that anyway, they should be setting dir on the root. And who sets match-parent on the root anyway.)

If that's unacceptable then I'd rather go with the Blink/WebKit behavior than Firefox, because at least it inherits as an absolute alignment, and having it resolve against the default directionality (LTR) at least has some kind of logic to it.

@nt1m
Copy link
Member

nt1m commented Jun 1, 2022

Chromium seems to match Firefox's behavior of using start now. I'm working on fixing this in WebKit, and I'm not sure what the intended resolution is, so I'm planning to follow Firefox/Chrome here for interop. If this changes, I'm happy to change accordingly.

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jun 2, 2022
https://bugs.webkit.org/show_bug.cgi?id=241164

Reviewed by Myles C. Maxfield.

The spec says the initial containing block's direction should be used when the element has no parent.

Regarding the computed value, Chrome & Firefox computes to `start` for this case.
This may change in w3c/csswg-drafts#6542 to `left`/`right`.

Test: imported/w3c/web-platform-tests/css/css-text/text-align/text-align-match-parent-root-rtl.html

* LayoutTests/TestExpectations:
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertTextAlign):

Canonical link: https://commits.webkit.org/251211@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295120 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@nt1m
Copy link
Member

nt1m commented Jun 2, 2022

I've implemented this in WebKit to return start: WebKit/WebKit@faf2d5c

@css-meeting-bot
Copy link
Member

css-meeting-bot commented Jun 8, 2022

The CSS Working Group just discussed [css-text] text-align: match-parent on the root element with direction: rtl doesn't match browsers, and agreed to the following:

  • RESOLVED: text-align: match-parent computes to 'start' on the root element
The full IRC log of that discussion <fantasai> topic: [css-text] text-align: match-parent on the root element with direction: rtl doesn't match browsers
<fantasai> github: https://github.com//issues/6542
<emilio> LOL
<emilio> fantasai: seems we currently have all browsers matching
<emilio> ... I don't think this is the ideal behavior but it's not terrible
<emilio> ... and it's a situation that is almost never going to happen in a real page
<emilio> ... unless somebody did something super weird by accident
<emilio> ... given we currently seem all engines compute to start
<emilio> ... which is Firefox's behavior
<emilio> ... I'm happy to resolve to that
<emilio> ... I think the right answer is to compute to right
<emilio> ... because it otherwise resolves to an absolute side
<emilio> florian: so proposal is computing to start
<emilio> ... behavior is the same as the spec asked for
<fantasai> on single-direction pages
<fantasai> emilio: I would rather to this, mostly because of how it interacts with writing-mode propagation
<fantasai> emilio: just compute to start value
<fantasai> Rossen_: any objections?
<fantasai> RESOLVED: text-align: match-parent computes to 'start' on the root element

See official minutes

fantasai added a commit that referenced this issue Dec 28, 2022
@frivoal frivoal closed this as completed Dec 28, 2022
frivoal added a commit to web-platform-tests/wpt that referenced this issue Dec 29, 2022
@frivoal frivoal added Tested Memory aid - issue has WPT tests and removed Needs Testcase (WPT) labels Dec 29, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 5, 2023
…-align:match parent, a=testonly

Automatic update from web-platform-tests
Add tests for boundary condition of text-align:match parent

See w3c/csswg-drafts#6542

--

wpt-commits: 60a2b189b81354c7efdc70e6da0d142e81a23b08
wpt-pr: 37693
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jan 13, 2023
…-align:match parent, a=testonly

Automatic update from web-platform-tests
Add tests for boundary condition of text-align:match parent

See w3c/csswg-drafts#6542

--

wpt-commits: 60a2b189b81354c7efdc70e6da0d142e81a23b08
wpt-pr: 37693
jakearchibald pushed a commit to jakearchibald/csswg-drafts that referenced this issue Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution css-text-3 Current Work Tested Memory aid - issue has WPT tests
Projects
None yet
Development

No branches or pull requests

6 participants