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

Incorrect test added for text-transform #46490

Open
frivoal opened this issue May 27, 2024 · 11 comments
Open

Incorrect test added for text-transform #46490

frivoal opened this issue May 27, 2024 · 11 comments

Comments

@frivoal
Copy link
Contributor

frivoal commented May 27, 2024

cc: @fred-wang, @mrego , @xiaochengh (as authors / reviewers)

As far as I can tell, https://github.com/web-platform-tests/wpt/blob/master/css/css-text/text-transform/text-transform-upperlower-107.html is in direct contradiction with the spec text.

https://drafts.csswg.org/css-text/#text-transform-property says:

This property transforms text for styling purposes. It has no effect on the underlying content, and must not affect the content of a plain text copy & paste operation.

But the test checks the opposite: that the copied text is affected by the transform.

Please be careful when writing / reviewing tests to look at the specification and confirm that it matches the behavior you expect, otherwise this can be very misleading / confusing for other people later on.

In this case, should we simply revert the test (and the corresponding implementation change), or do you want to open a bug against the spec to argue that it should be changed?

cc: @fantasai (as co-editor)

@fred-wang
Copy link
Contributor

@frivoal Thanks. I believe this was discussed elsewhere (probably w3c/csswg-drafts#3775), but chromium's implementation purposely disagrees with the spec and AFAIK there is no plan to change the implementation. So indeed this shouldn't have landed into the official repo but we need to keep this and probably similar tests for Chromium. So probably the plan forward is to move these tests into wpt_internal (and similar internal repositories for other browsers).

@fred-wang
Copy link
Contributor

cc @bkardell

@bkardell
Copy link
Contributor

bkardell commented May 28, 2024

Note: It passes in its current form in WebKit as well, so it seems that by far the vast, vast majority of users currently experience the opposite of what the spec says? I'm curious if there are bugs from users suggesting that it's wrong?

@frivoal
Copy link
Contributor Author

frivoal commented May 28, 2024

Firefox does what the spec say, and as far as I know it doesn't have bugs reported against it.

In any case, I know where I stand in this debate, but if disagreement persists, we should argue about it and about what the spec should say, not introduce test that encourage others to ignore the spec and do something else.

@fred-wang
Copy link
Contributor

not introduce test that encourage others to ignore the spec and do something else.

I assume you didn't mean to suggest that this was our intention when we introduced these tests, but just repeating myself: this is a mistake and such a test should really be kept in the internal ones (and it is important for chromium to ensure copying text-transformed text is not broken).

@adampage
Copy link
Member

Coincidentally, I added a similar WPT test a couple days ago to investigate the effect of text-transform in the computing of an element’s accessible name. I also filed an accompanying issue in w3c/accname, where I think a clarification will be beneficial either way.

The bleeding of text-transform into the browser’s accessibility tree feels to me like a straightforward violation of its specified purpose and more likely to harm a user’s experience than help it. But I also now understand that accessibility folks have previously debated this and some have advocated for text-transform to convey semantics. In any case, all 3 major engines do currently expose the transformed text.

All the while, I was unaware of this similar WPT issue and all the discourse around the expectations for this feature. 😅

I suppose I don’t have anything new to add here, but will be watching this issue as I consider whether I need to remove my own WPT test or invert their expectations.

@frivoal
Copy link
Contributor Author

frivoal commented May 30, 2024

I assume you didn't mean to suggest that this was our intention […]

I did not, and I apologize for the phrasing.

@yisibl
Copy link
Contributor

yisibl commented Jun 5, 2024

Please note: Chrome changed the behavior of copying to the clipboard a while ago, and now text-transform does not affect the actual text copied to the clipboard.

@fred-wang
Copy link
Contributor

@adampage @yisibl Thanks for the context information, it seems we should keep an eye on this. It seems the copy-and-paste thing is still experimented under a flag and the a11y part is to be discussed again.

There are a few text-transform tests using selection/copy. As I see ./text-transform-upperlower-107.html and text-transform-math-auto-003.html assume the transformed text is copied. Will change the expectation to copying the original text and keep their current versions as internal tests. text-transform-upperlower-105.html and text-transform-upperlower-106.html just checks that selecting the DOM text is properly reflected in the background of the rendered text so they look fine (the failure on 106 seems a bug in safari).

https://wpt.fyi/results/css/css-text/text-transform?label=master&label=experimental&aligned&q=text-transform-upperlower-105.html%7Ctext-transform-upperlower-106.html%7Ctext-transform-upperlower-107.html%7Ctext-transform-math-auto-003.html

$ css/css-text/text-transform$ find -type f | xargs grep 'getSelection()'
./math/text-transform-math-auto-003.html:            .getSelection()
./math/text-transform-math-auto-003.html:            window.getSelection().toString(),
./text-transform-upperlower-107.html:    window.getSelection().setBaseAndExtent(target, 0, target, 1);
./text-transform-upperlower-107.html:    assert_equals(window.getSelection().toString(), "SS");
./text-transform-upperlower-106.html:  window.getSelection().setBaseAndExtent(target.firstChild, 0, target.firstChild, 3);
./text-transform-upperlower-105.html:  window.getSelection().setBaseAndExtent(target, 0, target, 1);
./reference/text-transform-upperlower-106-ref.html:  window.getSelection().setBaseAndExtent(target.firstChild, 0, target.firstChild, 2);
./reference/text-transform-upperlower-105-ref.html:  window.getSelection().setBaseAndExtent(target, 0, target, 1);
$ css/css-text/text-transform$ find -type f | xargs grep 'toString()'
./math/text-transform-math-auto-003.html:    <meta name="assert" content="Verify Selection.toString() on a character with 'text-transform: math-auto' returns the transformed unicode character.">
./math/text-transform-math-auto-003.html:            window.getSelection().toString(),
./math/text-transform-math-auto-003.html:        }, `Selection.toString() for math-auto '${String.fromCodePoint(code_point)}' returns the transformed character.`);
./text-transform-upperlower-107.html:    assert_equals(window.getSelection().toString(), "SS");
./text-transform-upperlower-107.html:  }, "Selection.toString() for 'ß' with text-transform: uppercase");

@fred-wang
Copy link
Contributor

Please note: Chrome changed the behavior of copying to the clipboard a while ago, and now text-transform does not affect the actual text copied to the clipboard.

@yisibl I noticed you commented there are more cases where the transformed text is used instead of the original text: https://issues.chromium.org/issues/40343523#comment32 ; Selection.toString() which is used by the tests I mentioned, don't seem to be affected by this behavior change.

@fred-wang
Copy link
Contributor

fred-wang commented Jul 3, 2024

As far as I can tell, https://github.com/web-platform-tests/wpt/blob/master/css/css-text/text-transform/text-transform-upperlower-107.html is in direct contradiction with the spec text.

https://drafts.csswg.org/css-text/#text-transform-property says:

This property transforms text for styling purposes. It has no effect on the underlying content, and must not affect the content of a plain text copy & paste operation.

But the test checks the opposite: that the copied text is affected by the transform.

So checking again ./text-transform-upperlower-107.html and text-transform-math-auto-003.html rely on Selection.toString() which is actually not performing a copy and paste operation, and so not covered by this quotation from the css text spec and not affected by Chromium's new behavior (flag: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=2148)

Instead, it returns a string with the result of https://w3c.github.io/selection-api/#dom-selection-stringifier :

The stringification must return the string, which is the concatenation of the rendered text if there is a range associated with this.

I'm not sure what is referred to by "rendered text" but it looks like it should have the transform applied and so Chromium's behavior aligns with the spec.

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

8 participants