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-pseudo-4] need for clarification on how ::selection text-shadows work #3932

Closed
Gankra opened this issue May 14, 2019 · 13 comments
Closed

Comments

@Gankra
Copy link

Gankra commented May 14, 2019

The spec doesn't seem completely clear on what should happen when there are text-shadows defined by a ::selection pseudo-selector.

I have written a live demo that demonstrates/discusses how different browsers work here. But here's a better description:

  • Chrome: clips ::selection shadows to the highlight, but doesn't disable non-::selection shadows (as if the ::selection was a distinct element, and not a restyling?). More specifically, if a character is selected, its ::selection shadows will be drawn clipped to the highlight, but its non-::selection shadows will be drawn clipped-out from the highlight. Notably, shadows of characters that aren't selected but still from the same element aren't clipped by the highlight.

  • Safari: seems to be implementing the same logic as chrome, but has extremely buggy clipping/invalidation of the shadows and sometimes ends up drawing shadows outside of the highlight or even when the text isn't selected.

  • Firefox: clips ::selection shadows to the bounds of the entire text frame. This lets shadows escape the highlight as long as there's text there, but will clip them otherwise. However, the bounds used by firefox for this clipping include the area covered by non-::selection shadows, so non-::selection shadows can expand the area ::selection shadows can escape to. Unlike chrome/safari, firefox completely disables all the non-::selection shadows for the characters that are selected (as if they were being normally overridden by cascading).

    • Webrender Backend: Inherits firefox's shadow cascading logic, but doesn't yet clip ::selection shadows any differently from normal ones.

So the questions we have are:

  • Should ::selection text-shadows be clipped differently from normal text-shadows? And if so, should they specifically be clipped to the highlight?

  • Should ::selection text-shadows cascade and overwrite normal text-shadows, or are selections supposed to act like a new element with a separate text-shadow property? If the latter, should normal shadows be clipped/overwritten by the highlight?

@Gankra
Copy link
Author

Gankra commented May 14, 2019

FWIW the folks I have discussed this with at Mozilla seem to like Webrender's behaviour (cascade the ::selection text-shadow as a completely normal, non-specially-clipped, text-shadow). They regard Firefox's special clipping behavior to be an implementation bug, and not intentional (which makes sense if you dig into the implementation details).

@fantasai
Copy link
Collaborator

fantasai commented May 14, 2019

Wrt first question, I don't think there's any need for ::selection text-shadows to be clipped differently from normal text-shadows.

Wrt the second question, there was some discussion about it in #3605 ; the conclusion was that the ::selection’s text-shadow wins, and UAs are expected to set it to 'none' by default. I'm now thinking about how the mechanics of that are going to work for ::spelling-error and ::grammar-error, and I'm not sure how to describe it.... because for those pseudo-elements, we'd want text-shadows to not be suppressed by default, but it's not clear how the property values interact!

@fantasai fantasai added the css-pseudo-4 Current Work label May 14, 2019
@fantasai
Copy link
Collaborator

To summarize, the things we seem to want are:

  • Author can set text-shadow on a highlight pseudo and have it take effect.
  • The default case of selection with a background suppresses document text-shadows, for readability.
  • ::spelling-error and ::grammar-error, by default, do not suppress text shadows.

Possible models to make things happy:

  • Non-transparent background on any layer disables shadows on layers below
  • Background on any layer paints over shadows on layers below (but they may show through if semi-transparent or larger than the background area).
  • ???

@fantasai
Copy link
Collaborator

Related question: if multiple layers specify a text-shadow and there's no backgrounds involved...

  • Draw only the topmost non-none shadows?
  • Draw all of them?
  • [Drawing only the topmost value, with none suppressing shadows below it, is unworkable due to the desire for ::spelling-error and ::grammar-error to not suppress shadows by default.]

@TalbotG
Copy link
Collaborator

TalbotG commented Jul 17, 2019

6 tests on text-shadow and ::selection

5 of them are manual tests.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed need for clarification on how ::selection text-shadows work, and agreed to the following:

  • RESOLVED: If multiple layers with text-shadows we draw them all
  • RESOLVED: Background on a highlight layer paints over shadows on layers below
The full IRC log of that discussion <dael> Topic: need for clarification on how ::selection text-shadows work
<dael> github: https://github.com//issues/3932
<dael> fantasai: I was going over what we want for text-shadows. Came up with a bunch of questions for ::selection text-shadows
<dael> fantasai: Last time we discussed only thought about ::selection but have multiple highlight pseudos that can layer. Models discussed don't make too much sense.
<fantasai> https://github.com//issues/3932#issuecomment-510220043
<dael> fantasai: Summary ^
<dael> fantasai: Default case selection with a background suppresses text-shadows because otherwise might not get enough contrast. Spelling and grammer do not suppress text-shadows. Need to make it that having a highlight there shouldn't suppress text-shadow below
<dael> fantasai: 2 models. Any non-transparent background disables shadows on layers below. Background on any paints over layers below.
<dael> fantasai: Highlight pseudos might want to change order and paint background inbetween
<dael> fantasai: There are questions on multi-layers with text-shadows and they all spec text shadow do we draw top most non-none? All?
<dael> fantasai: I don't have a clear answer to these questions so I want feedback from WG
<dael> AmeliaBR: I don't think have any other way in CSS where you can composit together text-shadow from different declarations
<dael> fantasai: Right, because shadows inherit. Parent inherits to child so shadow takes effect on child unles syou do something. Here is a bunch of layers without parent/child so no clear inheritence. But we don't want something like spelling-error to prevent a shadow
<fantasai> s/shadow/shadow just because it now exists/
<dael> AmeliaBR: As with everything on selection highlight classes I wish we could make sense of this in normal cascade way. I think trying to draw 2 text shadows on same text is strange.
<dael> AmeliaBR: For background layering multiple backgrounds seems to make sense. Question of if it's performant to draw shadows that will be obsurced.
<dael> fantasai: I don't care if we draw background over or don't draw because the background will obscure. Either gets reasonable behavior
<dael> astearns: Main thing is pick easily impl and can be consistent. Seems edge case b/c selection happens on editable text.
<dael> fantasai: COuld select to copy out. THat's frequent
<dael> smfr: Some add funcky text shadow to change anti-aliasing
<dael> astearns: ooh, fun.
<dael> smfr: I'm confused about how thigns are supposed to work. Understand text-shadow style gets cascade into text-shadow on elemnt and paint result. fantasai sounds like saying selection has own text-shadow style hence the layers
<dael> fantasai: Didn't quite follow. In this case you have a word and it is a spelling error and a grammar error and it's selected.
<dael> fantasai: Properties on each pseudo element cascades in and inherited through...trying to remember b/c changed...[reads spec]
<dael> fantasai: You aren't going to have the text-shadow value inherit from base document element. Properties are inherited from parent. Each element has own ::selection and the ::selection of span inherits from ::selection of p which inherits from ::selection of body. Text-shadow around that word text-shadow prop won't have that value on it.
<dael> fantasai: If going to draw text we would see text-shadow:none and that's not okay for spelling error. You don't expect a spelling error to suppress shadows. Even though spelling errors don't have text-shadow you want to draw the shadow
<fantasai> https://drafts.csswg.org/css-pseudo-4/#highlight-cascade
<dael> smfr: It's about the decoration style clobbering the text-shadow of unselected version
<dael> fantasai: Could do it for selection, but not for spelling and grammar. Making it disappear with only difference is underline doesn't make sense and could make text unreadable
<dael> smfr: Impl no problem paining multi shadow. Prefer to avoid complex logic if thigns are transparent
<dael> fantasai: 2 related questions. 1: How do we deal with suppressing shadows when drawing background b/c selection has background. Can do that by saying if bg in non-transparent we don't paint shadows below. Or paint shadows first and then paint background
<dael> smfr: bg may nto be opaque
<dael> fantasai: Right. Or smaller then text-shadow
<dael> fantasai: You could dsitinguish between two, but difference isn't that important question of what's easier to implement
<dael> smfr: Want to look at native platform to see if those make sense and should be matched
<dael> fantasai: Second question is if we have multi-layers spec text-shadow. So author decides spelling error creates a blurred red ext-shadow and grammar is green shadow and selection is orange shadow, do we draw all or only top most non-none?
<dael> smfr: I think draw all. Draw what author asked for
<dael> smfr: Mac native does paint text shadow under selection, I think. You do get combination
<dael> fantasai: Prop: If multiple layers with text-shadows we draw them all
<dael> fantasai: Resolve on that?
<dael> astearns: Objections?
<dael> RESOLVED: If multiple layers with text-shadows we draw them all
<dael> fantasai: Background either suppress or paint over lower level, I think smfr suggests we paint over?
<dael> smfr: Yes
<dael> fantasai: Background on a highlight layer paints over shadows on layers below
<dael> astearns: Obj?
<dael> RESOLVED: Background on a highlight layer paints over shadows on layers below
<dbaron> I'm not crazy about the background painting over thing.
<dael> astearns: Is that enough to spec interop?
<dbaron> seems like the shadows may well peek out at the edges.
<dael> fantasai: THat's enought o spec. Interop depends on impl.
<dael> astearns: There are test cases I expect will need to be ammended to cover
<dael> fantasai: Yeah
<dbaron> (I'm listening, but I'd need to reconnect in order to unmute)
<dael> astearns: dbaron you mentioned you're not crazy about peaking out? I believe that is the case and shadows will show if larger then background
<dael> fantasai: I don't care which way it goes between the 2
<fantasai> as long as we have an answer
<dael> astearns: Let's go with that option. dbaron if you have a change or obj please update the issue
<dbaron> yeah, I don't feel that strongly, but the behavior seems a bit ugly
<fantasai> Also OK if we want to make it a UA choice between the two
<dael> astearns: Anything else on this?
<fantasai> just so long as it's only those two options and not "do anything" :)

@fantasai
Copy link
Collaborator

@Gankra @frivoal OK, I've updated the spec with the CSSWG resolutions; text-shadows are now defined to be stacked up under/over the highlight backgrounds. Please let me know if anything seems off. (But note that nobody implements this kind of layering atm.)

@frivoal
Copy link
Collaborator

frivoal commented Sep 11, 2020

The edits linked above seem clear to me, and to correctly reflect the way the WG decided to solve this issue.

@delan
Copy link
Contributor

delan commented Jan 18, 2021

G’day @fantasai @Gankra, I found this issue long after updating some ::selection text-shadow tests to reflect the new spec behaviour. I didn’t realise specs could actually reference WPT cases until I stumbled upon the bikeshed button:

screenshot

Sorry for breaking that. I’ve replaced those two with highlight-painting-order.html which:

  • checks the relative painting order of text, original shadows, ::selection background, and ::selection shadows
  • asserts that the ::selection shadows are unclipped and independent of the original shadows

@TalbotG
Copy link
Collaborator

TalbotG commented Jan 18, 2021

Delan,

I added
css/css-pseudo/selection-text-shadow-006-manual.html
css/css-pseudo/selection-text-shadow-016.html
css/css-pseudo/selection-text-shadow-016-ref.html
in PR20802
and then I bikeshed-added these in PR5006

I’ve replaced those two

Why did you have to remove the 2 tests?

Why didn't you just add your test?

Gérard

@delan
Copy link
Contributor

delan commented Jan 18, 2021

@TalbotG I removed them because the changes they needed, both for the new spec and new browser quirks that broke them, would have neared a rewrite anyway, and I had already designed a reliable reftest that subsumes their functionality. I wasn’t able to find any etiquette guidelines around removing tests, so I thought it would be ok as an improvement done in good faith.

@TalbotG
Copy link
Collaborator

TalbotG commented Jan 27, 2021

Delan,

I have bikeshed-added a link to your highlight-painting-order.html test
(along with links to 31 other tests) and I have removed the links to
selection-text-shadow-006-manual.html
selection-text-shadow-016.html
in PR5899 . So the 2 Bikeshed Errors in bikeshed/css-pseudo-4/
have been fixed now.

@delan
Copy link
Contributor

delan commented Jan 27, 2021

Wonderful! Thanks @TalbotG.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 14, 2021
There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 14, 2021
There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 14, 2021
There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222139
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#931761}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 15, 2021
There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222139
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#931761}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 28, 2021
…or custom ::highlight pseudos, a=testonly

Automatic update from web-platform-tests
Highlight API: Don't paint text-shadow for custom ::highlight pseudos

There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222139
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#931761}

--

wpt-commits: e6042b2aa44d5ee0a2ebcfffd0612ee933675f89
wpt-pr: 31248
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 28, 2021
…or custom ::highlight pseudos, a=testonly

Automatic update from web-platform-tests
Highlight API: Don't paint text-shadow for custom ::highlight pseudos

There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222139
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#931761}

--

wpt-commits: e6042b2aa44d5ee0a2ebcfffd0612ee933675f89
wpt-pr: 31248
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this issue Nov 18, 2021
There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222139
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#931761}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
There are some uncertanties [1] [2] [3] about how text-shadow should
work for highlight pseudos. Until these are resolved, disable
text-shadow for custom ::highlight so that when custom Highlight API
ships, developers don't take dependencies on text-shadow behavior that
may change or be removed entirely.

[1] w3c/csswg-drafts#3932
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2658617/
[3] https://hackmd.io/@dazabani/cr-highlight-pseudos-2021-06#text-shadow-property

Bug: 1164461
Change-Id: I8253ad2f3054320c5aba452296be0c72f0015a6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3222139
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#931761}
NOKEYCHECK=True
GitOrigin-RevId: cbd89f4a54c93f7cf36bf8799a357f289d23312d
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

6 participants