[go: nahoru, domu]

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] highlights and color:currentColor #6818

Closed
delan opened this issue Nov 16, 2021 · 8 comments
Closed

[css-pseudo] highlights and color:currentColor #6818

delan opened this issue Nov 16, 2021 · 8 comments

Comments

@delan
Copy link
Contributor
delan commented Nov 16, 2021

https://drafts.csswg.org/css-pseudo-4/#highlight-text

For this purpose, ‘currentColor’ on a highlight pseudo-element’s ‘color’ property represents the ‘color’ of the next highlight pseudo-element layer below, falling back finally to the colors that would otherwise have been used (those applied by the originating element or an intervening pseudo-element such as ‘::first-line’ or ‘::first-letter’).

  1. When we choose the “next highlight pseudo-element layer below”, do we consider only the layers that are actively highlighting the content in question? I think it makes sense to do so, in order for color:currentColor to effectively mean “highlighting with this pseudo doesn’t change the foreground color”. Otherwise, color:currentColor on ::selection always yields ::target-text’s default foreground color, even when that’s irrelevant to the content.

  2. If so, what is the result of getComputedStyle(..., "::selection").color? The problem is that we need the resolved value, which would be indeterminate for a highlight with color:currentColor. Even if the active highlights were the same throughout the whole originating element, giving us a single unambiguous answer, we can only expose that answer if it didn’t come from ::spelling-error or ::grammar-error (#highlight-security).

(cc @mrego, @frivoal)

@delan delan added the css-pseudo-4 Current Work label Nov 16, 2021
@delan delan added the Agenda+ label Nov 24, 2021
@astearns astearns added this to Temp2 in December 8 meeting Dec 7, 2021
@astearns astearns moved this from Temp2 to pseudo in December 8 meeting Dec 7, 2021
@delan
Copy link
Contributor Author
delan commented Dec 8, 2021

(summary for meeting)

color:currentColor is always a special case. Normally it means the same thing as color:inherit, but for highlights, it means “the next highlight pseudo-element layer below”. The two questions here are: [see description]

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed highlights and color:currentcolor.

The full IRC log of that discussion <emilio> topic: highlights and color:currentcolor
<emilio> github: https://github.com//issues/6818
<emilio> delan: setting color to currentcolor is always a special-case, it normally means the same thing as `inherit` but for highlights we have a new meaning where according to the spec it means "the next highlight layered below"
<emilio> ... there's two questions about it, first one is, when we say the next below, are we considering just the highlights that highlight that piece of text?
<emilio> ... so for example if you have `::target-text { color: red } ::selection { color: currentcolor }`, should it always be red? Or only if the selected text is also `::target-text`
<emilio> fantasai: only reasonable behavior is the later
<emilio> delan: agreed, it's mostly ok impl-wise except for e.g. getComputedStyle
<emilio> ... if you ask for the `getComputedStyle(element, "::selection").color`, then what is the color? It could be many different things depending on what you're looking at
<emilio> astearns: maybe return just the keyword in this particular case?
<emilio> delan: gCS is explicitly using the resolved value
<emilio> ... which turns it into an actual color
<emilio> florian: do we have precedent for this kind of problem?
<fantasai> s/things/colors/
<emilio> delan: not aware of it
<emilio> florian: we might have a similar problem with fragmentation if you allow different fragments to be styled differently
<emilio> ... I believe we have answered that except exceptions we'd answer the first one
<emilio> ... I think that means that if the entire selection crosses the entire element then that makes sense but then you look at the first chunk of the element and answer based on that one
<emilio> q+
<emilio> delan: so answering based on the first highlight for the first element or so?
<emilio> florian: that seems similar to other questions on the past
<fantasai> emilio: Right now, getComptuedStyle() with selection does return the actual selection colors, regardless of whether anything is selected
<fantasai> emilio: would be bad if it involved resolving style of all the layers below
<sanketj> q+
<fantasai> emilio: also, color: initial and color: currentColor do different things here, is weird
<astearns> ack emilio
<fantasai> emilio: I'm not sure I agree with behavior of currentColor here, would be nice if not so weirdly
<fantasai> emilio: Do we know reason for currentColor being different here?
<fantasai> delan: my understanding, the intuitive intent of 'color: currentColor' is same for highlights and not-highlights
<fantasai> delan: it means don't change the color
<fantasai> delan: But in case of highlight, have a different inheritance tree, inheriting from parent's highlight styles
<fantasai> delan: so it means "don't change the color compared to if you didn't highlight with this highlight"
<fantasai> delan: if selection says currentColor, says use color I would use if text was not selected
<fantasai> delan: but that's not inherit, can't be
<fantasai> emilio: Maybe selection styles shouldn't ...
<fantasai> emilio: idk
<fantasai> emilio: seems very weird to specialcase currentcolor and bunch of other stuff that could be specialcased
<fantasai> delan: highlights are a lot of special cases. Maybe too many
<fantasai> emilio: adding all this complexity, we have a lot of use cases for this for this particular way
<fantasai> emilio: currentcolor behaves the same everywhere else
<fantasai> emilio: could argue the same for ::first-line and that kind of thing
<sanketj> q-
<fantasai> emilio: in fact, don't we solve this for ::first-line in some other interesting way?
<fantasai> emilio: If you have a span with 'color :currentcolor'
<emilio> `div { color: green } div::first-line { color: red } span { color: currentColor }` then `<div><span>First line</span></div>`
<fantasai> delan: is it not well-defined for these in the same way, they add boxes to the box tree
<fantasai> delan: there's a well-defined inheritance in the one tree?
<fantasai> delan: whereas there's a separate inheritance tree for each pseudo-element
<fantasai> florian: it is well-behaved in browsers, i.e. does the thing delan expects
<fantasai> emilio: making the span inherit from first line, can we make selection inherit from whatever special style we're taking the color from instead, and there's *an* answer for this?
<fantasai> astearns: we are out of time, so take this back to the issue
<fantasai> astearns: can come back to it next week along with the other pseudo issues... though we are going ot start with lenght units and video MQ
<fantasai> astearns: so thanks for putting all these issues up
<fantasai> astearns: can you join next week?
<fantasai> delan: yes
<oriol> ::first-line inheritance was discussed in https://github.com//issues/1097
<fantasai> astearns: out of time
<fantasai> astearns: so done for this week
<florian> "pick the first fragment" was discussed in https://github.com//issues/6513

@emilio
Copy link
Collaborator
emilio commented Dec 8, 2021

So at least for gCS, probably the simple answer is not having other highlights into account, so we always return consistent styles, I believe. I still don't think currentColor behaving oddly is great though.

@delan
Copy link
Contributor Author
delan commented Dec 8, 2021
  • @emilio is unhappy with color:currentColor having a special case for highlight, and wonders if we can instead reconcile things by making ::selection inherit from ::target-text and so on (such that currentColor=inherit again)
  • @frivoal suggests returning the first value if there are multiple colors throughout the ::selection (see also [cssom][css-break] getComputedStyle and fragmentation #6513)
    • (@delan but what if there is nothing selected?)
  • @emilio suggests making gCS(::selection) pretend that the whole element is selected but not highlighted by anything else, regardless of actual highlight ranges, i think?
    • (@delan this would also solve the potential privacy leak where ::selection exposes colors from ::spelling-error or ::grammar-error)

@delan
Copy link
Contributor Author
delan commented Dec 8, 2021

Thanks @emilio, so if I understand you correctly, color:currentColor (as currently specified) would be the originating element’s ‘color’ in gCS, because we pretend that no other highlights are active? I can see this working better than @frivoal’s suggestion of using the color of the first highlighted character/replaced, because it avoids the problems of nothing being highlighted or ::selection/::target-text leaking ::{spelling,grammar}-error.

My only question then would be whether it’s ok to make gCS(highlights) be so “hypothetical” and different from reality? I don’t really mind, but I also have never really used gCS that often, so I don’t know.

As for making color:currentColor equivalent to color:inherit for highlights (like it is for non-highlight styles), I guess there are two ways we can do that. One is to drop the functionality that color:currentColor provides, and the other is what I think you said on IRC, change highlight inheritance so we inherit (all?) from the next active highlight layer below.

If you mean the first, maybe @fantasai or @frivoal can comment on the intent behind color:currentColor, and whether we really need it (or can provide it some other way). If the second, can you elaborate on how we would do that while also being able to inherit styles from the same highlight in the parent?

@svgeesus
Copy link
Contributor

The language about currentColor in CSS Pseudo is odd indeed and makes currentColor even more complex than it already is.

(As an aside, why is CSS Pseudo 4 linking to CSS Color 3 for the color definitions?)

@atanassov atanassov added this to Can wait until 2022 in December 15 meeting Dec 15, 2021
@atanassov atanassov moved this from Can wait until 2022 to Top priority in December 15 meeting Dec 15, 2021
@delan
Copy link
Contributor Author
delan commented Dec 15, 2021

If you mean the first, maybe @fantasai or @frivoal can comment on the intent behind color:currentColor, and whether we really need it (or can provide it some other way).

I remembered the use case this was important for btw! With ::spelling-error and ::grammar-error, typical styles are to add a red or green squiggly line without changing the background or foreground color. The former is trivial (initial/transparent), but the latter needs some way of grabbing the ‘color’ that would have been used prior to the highlight, otherwise every misspelled word would become initial/black (#6779).

The language about currentColor in CSS Pseudo is odd indeed and makes currentColor even more complex than it already is.

I agree, though I fear this might end up being the least complicated way to solve that problem.

(As an aside, why is CSS Pseudo 4 linking to CSS Color 3 for the color definitions?)

No idea personally… I’m not yet familiar with our conventions around spec levels. Should we link to level 4 or 5?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Highlights and currentcolor, and agreed to the following:

  • RESOLVED: 'color: currentColor' on a highlight takes the next *active* highlight color, so that text is drawn as if highlight weren't taking effect
  • RESOLVED: getComptuedStyle() with a highlight pseudo takes color as if that highlight applied and no other highlight applied
The full IRC log of that discussion <fantasai> Topic: Highlights and currentcolor
<fantasai> delan: Started discussing this issue last week, but only got partway through
<fantasai> delan: about special case of setting 'color: currentColor'
<fantasai> delan: outside of highlights, it means 'color: inherit'
<fantasai> delan: for highlights, actually means something else
<fantasai> delan: there were 2 questions originally, now 3
<fantasai> delan: 1st question, double-checking, spec says when setting to currentColor, do we want to grab color from next highlight layer below regardless, or only active highlight layers
<fantasai> delan: fantasai said it only makes sense to use active highlight layer, and I think everyone agrees on that
<fantasai> florian: I'm a little unsure, agree with that assessment, but discussion last time suggested maybe there was a whole different way to think about this
<fantasai> delan: that's the 3rd question, whether this is the right way to solve the problem that it solves
<fantasai> delan: 2nd question is, if it is the next active highlight specifically, then what happens when you getComputedStyle?
<fantasai> delan: getComputedStyle needs to return resolved colors, so has to return an actual color
<TabAtkins> q+
<fantasai> delan: if you have an element with multiple underlying colors, what do you do
<fantasai> delan: currntly a few ideas on how to address that
<fantasai> delan: 3rd question, which emilio brought up, this exceptional behavior of currentColor for highlights is odd and complicated
<fantasai> delan: wondering if this was the right way to solve the problem, or if better way to solve the problem
<fantasai> delan: To clarify, best example of a use case for this, is the spelling and grammar error pseudo-elements
<fantasai> delan: a typical styling for that would be to add a red or green squiggly line to the text
<fantasai> delan: but you're just adding the decoration, you don't want to change the color of the text
<fantasai> delan: if you spell a word, still want to have the same text color
<cpn> s/cpn/chcunningham/
<fantasai> delan: but given way highlight painting works, if we didn't have this rule for currentColor, then there would be no way to highlight something without changing the color to 'initial' (black)
<fantasai> delan: it's complicated and an odd exception, but we need some solution to this problem
<fantasai> florian: one extra bit, suggestion last time is that this might be similar to ::first-line
<fantasai> florian: we don't redefine how currentColor works, we redefine what inherits from what
<fantasai> florian: if you set currentColor on first line, it'll get its color from ::first line
<emilio> q+
<Rossen_> ack TabAtkins
<fantasai> TabAtkins: I'm not certain about best way to handle property itself, but for purpose of getComputedStyle I think it's reasonable to say "pretend it's not selected at all", so just get underlying style
<delan> q+
<fantasai> TabAtkins: that seems the most straightforward, and doesn't leak info about spelling/grammar errors
<TabAtkins> scribe+ TabAtkins
<florian> q+ to ask clarification from Tab
<Rossen_> ack fantasai
<TabAtkins> fantasai: WE also have to consider that when you set the - we have paired cascading, and it sets to "the color it would have already been" it sets the background, so this behavior needs to represent that as well
<Rossen_> ack emilio
<fantasai> emilio: does this really only apply to currentCOlor? seems applies to inherited properties applied to highlights
<fantasai> emilio: maybe color is the only one atm
<fantasai> emilio: seems you'd have the same problem with text-fill and other things
<fantasai> emilio: so, in general, I think the right fix would be inheritance, but that might be annoying to implement
<Rossen_> q?
<Rossen_> ack delan
<fantasai> delan: Tab, is it true that you mean same thing as emilio, wrt Q2, my understanding is that if you ask for getComputedStyle for ::selection, we pretend everything is selected and no other highlights apply
<fantasai> TabAtkins: no opinion on pretending selected or pretending not selected, defer to impl on that question
<fantasai> TabAtkins: but ignoring any other highlights is the important bit
<fantasai> delan: sounds like a straightforward solution to that, I agree
<fantasai> delan: wrt Q3, inheritance and possible parallels with ::first-line
<fantasai> delan: I'm not that fresh on ::first-line and ::first-letter
<fantasai> delan: can someone explain how it would work for us to sometimes inherit from another highlight (grab color from ancestor highlight) while still inheriting within this inheritance tree for ???
<Rossen_> q?
<florian> q-
<fantasai> emilio: I think that's the hard part
<astearns> github: https://github.com//issues/6818
<fantasai> emilio: My selection styles inherit from my parent selection styles
<Rossen_> q?
<fantasai> emilio: E.g. selection style inherits from other highlight which inherits from regular text which inherits from parent selection style
<fantasai> delan: ...
<fantasai> delan: ::target-text inherits from ::selection style??
<fantasai> emilio: whichever order we decide on...
<fantasai> emilio: for simplicity, say we have ::selection and ::spelling-error
<fantasai> emilio: say ::selection inherits from ::spelling-error, and ::spelling-error inherits from parent ::spelling-error
<fantasai> emilio: I think that would get you the right behavior
<Rossen_> q?
<TabAtkins> fantasai: It would not, because if ::selection inherits from ::spelling-error, and that inherits from parent ::spelling-error, it'll never inherit from the parent selection
<fantasai> emilio: ::spelling-error inherits from ::selection
<TabAtkins> fantasai: Why would spelling-error inerit from selection?
<fantasai> emilio: ::spelling-error inherits from ::selection
<fantasai> fantasai: that doesn't work
<TabAtkins> fantasai: can you give me an example of the zigzag?
<TabAtkins> fantasai: there's two pseudos - selection paints over spelling-error - what's the inheritance chain?
<fantasai> delan: So you go in a zigzag, our ::selection inherits from our ::spelling error, and our ::spelling-error inherits from our parent's ::selection, which inherits from parent's ::spelling-error, etc.
<TabAtkins> child selection -> child spelling error -> parent selection -> parent spelling-error
<fantasai> fantasai: Why would it make sense for ::spelling-error to inherit from parent's ::selection?
<TabAtkins> fantasai: Why would it make sense for spelling error to inherit from the parent's selection?
<fantasai> emilio: I guess it doesn't quite...
<fantasai> delan: when you jump back in the zigzag, you have a lower layer inheriting from a higher layer
<fantasai> delan: it's hard for me to imagine this
<TabAtkins> fantasai: imagine spelling was red, seleciton was blue
<TabAtkins> fantasai: you highlight some things
<Rossen_> q?
<TabAtkins> fantasai: the zigzag means you'll get red text when you highlight, if the parent has an spelling error
<tantek> This (special inheritance across pseudo-elements) seems confusing enough to the WG that I can't imagine authors actually coming up with a predictive mental model for what is going on.
<TabAtkins> fantasai: seems weird
<fantasai> Rossen_: Seems work to do here, need to decide if we are going to rethink through inheritance or to tweak existing model
<delan> q+
<fantasai> delan: This is a valid conversation, about solving via other mechanism
<fantasai> delan: but wondering if we can resolve Q1 and Q2, based on assumption that things work the way specified now using currentColor
<fantasai> delan: and later if we want to solve a different way, we can do that?
<fantasai> Rossen_: Makes sense to me
<florian> q+
<dbaron> I *think* one of he goals here is that which of grammar-error/spelling-error/target-text/selection's styles wins does *not* depend on which elements are associated with the selectors (and how they nest), but rather on a spec-defined order of the pseudos.
<Rossen_> ack florian
<delan> q+
<fantasai> florian: I support doing this. Earlier Tab suggested that you put either everything selected or nothing is, suggest assuming everything because otherwise no point in querying ::selection
<Rossen_> ack delan
<fantasai> delan: I thought question was between nothing or pretending that just the pseudo you asked for
<fantasai> florian: that's the one
<fantasai> florian: answering that just the pseudo you asked for is everywhere and nothing else
<fantasai> florian: It's not useful to assume no highlights at all
<fantasai> emilio: I think that's the current behavior
<fantasai> emilio: I'm moderately sure that querying ::selection will get you the ::selection styles
<fantasai> delan: pretendign everything else not selected also solves, what happens if ::selection leaks a color from ::spelling-error or ::grammar-error, which we made it forbidden for privacy reasons so this solves that problem
<fantasai> emilio: it's also simpler, fixes privacy leak
<fantasai> florian: alternative we mentioned last time was to fragment things, and answer question about first one, but overkill for no good reason
<fantasai> delan: don't think anyone needs that answer
<fantasai> florian: if we really needed that answer, we'd need an API to reply on each individual fragment
<fantasai_> delan: For Q1, proposing that we say that when you say 'color: currentColor' on a highlight pseudo, you grab the next active highlight's color
<fantasai_> delan: so that color is as if this highlight wasn't applying
<fantasai_> emilio: ...
<fantasai_> fantasai_: currentColor computes to itself
<fantasai_> emilio: except in 'color' property
<fantasai_> emilio: I'll double-check
<fantasai_> emilio: anyway, seems fine
<fantasai_> RESOLVED: 'color: currentColor' on a highlight takes the next *active* highlight color, so that text is drawn as if highlight weren't taking effect
<fantasai_> scribe+
<fantasai_> delan: For Q2, when you call getComputedStyle() with a highlight pseudo, the color of the result should behave as if the pseudo you passed in is highlighted, but all other highlight pseudos are not highlighting
<fantasai_> delan: we pretend
<florian> +1
<fantasai_> RESOLVED: getComptuedStyle() with a highlight pseudo takes color as if that highlight applied and no other highlight applied
<fantasai_> Rossen: we'll take Q3 back to the issue

delan added a commit to delan/csswg-drafts that referenced this issue Apr 15, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 6, 2022
Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
aarongable pushed a commit to chromium/chromium that referenced this issue Jun 8, 2022
Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3690272
Reviewed-by: Delan Azabani <dazabani@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1012045}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 8, 2022
Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3690272
Reviewed-by: Delan Azabani <dazabani@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1012045}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 8, 2022
Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3690272
Reviewed-by: Delan Azabani <dazabani@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1012045}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 13, 2022
…the correct originating element, a=testonly

Automatic update from web-platform-tests
currentColor for highlight pseudos uses the correct originating element

Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3690272
Reviewed-by: Delan Azabani <dazabani@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1012045}

--

wpt-commits: c801c26f4182f6e2fbf72ac3093a3746d8d50400
wpt-pr: 34315
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 14, 2022
…the correct originating element, a=testonly

Automatic update from web-platform-tests
currentColor for highlight pseudos uses the correct originating element

Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3690272
Reviewed-by: Delan Azabani <dazabani@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1012045}

--

wpt-commits: c801c26f4182f6e2fbf72ac3093a3746d8d50400
wpt-pr: 34315
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Per the resolutions in [1], for a highlight pseudo with currentColor
as its color, getComputedStyle(element, "::highlight(...)").color
should return the color of the originating element (since this is the
color that would be painted if that highlight were the only one
applied).

This is currently achieved by having Color::ApplyValue grab the color
from the originating element for highlights using currentColor.

This hits a problem when we do
getComputedStyle(element, "::highlight(...)") on an element that
doesn't directly match a highlight pseudo, but is a child of an
element matching a highlight pseudo. In that case we end up reusing
the computed style for the highlight that was built for the ancestor
element. That highlight style still uses the color from the ancestor
element, which is the wrong result.

Here we fix this by having StyleAdjuster::AdjustComputedStyle adjust
the style to use the color from the correct originating element.

[1] w3c/csswg-drafts#6818 (comment)

Bug: 1321540
Change-Id: Ibbc9126811fe52ecadb358713857df05c1143708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3690272
Reviewed-by: Delan Azabani <dazabani@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1012045}
NOKEYCHECK=True
GitOrigin-RevId: ff9902331c011197e27922ba0f88eac7192da4b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

5 participants