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-highlight-api] Inline styles for Highlight should apply on top of styles provided by ::highlight #4588

Closed
sanketj opened this issue Dec 12, 2019 · 7 comments

Comments

@sanketj
Copy link
Member

sanketj commented Dec 12, 2019

This section (https://drafts.csswg.org/css-highlight-api-1/#c-and-h) of the highlight API spec mentions that "the User Agent must use the styles provided in the style attribute of the corresponding highlight when no applicable property has been specified via ::highlight()". This seems to imply that if a ::highlight has been specified, then the ::highlight styles should "win" over those coming from the .style attribute. What was initially proposed in the highlight API explainer (https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/master/highlight/explainer.md#application-of-css-properties) was that "the Highlight object's 'inline' style (i.e. properties set directly on the .style member) will be applied on top of the cascaded values for the given highlight identifier", which is the reverse of what we currently have spec'ed.

Not sure if this was a deliberate change but I think it is important that styles set via the .style attribute apply on top of ::highlight styles. This is a useful convenience for authors to be able to override the highlight styles from script. I can see it being especially useful when combined with highlight events (https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/master/highlight/events-explainer.md). For example, if the author wants to change the background color of a highlight when it is clicked, it is convenient to attach a click event listener to the highlight and then set the .style attribute in the callback.

cc: @frivoal @BoCupp-Microsoft @gked

@frivoal
Copy link
Collaborator

frivoal commented Dec 13, 2019

This difference was deliberate when I drafted the document, but it's not something I'm completely sure about either, and did want to discuss. Here's the logic I had when I wrote this:

  1. There was already a "default styling" behavior for the built-in pseudos, used for ::selection and ::active selection, defining how styles using the pseudo-element and styles coming from elsewhere would interact, so I thought that it made sense to try and use the same mechanics there rather than define something similar but different.

  2. If it overrides the styles coming from the stylesheet, then it feels a lot like the style attribute on DOM elements. But in that case, this feels like something that we should extend to all pseudo elements, no just the ::highlight() ones, in part because that would be generally useful, and in part because otherwise, how exactly does it work? does it override all uses of ::highlight(), or only :root:highlight() or…? What about !important? Does it replace the ::highlight() value at specified, or computed, or used value time? How does it interact with animations? How do the APIs to inspect the CSSOM account for it? How does it interact with var() or attr()? Etc...These questions are easily answered if it's a generic style attribute for pseudo-elements that fit the normal cascading model the same way the style attribute for DOM elements does, but they're all open questions if we come up with something new.

So my general idea was that if it is specific to overlays, it probably should work like the default for ::selection (and we still need to answer some of these open questions), but probably it should be replaced by something that works on all pseudo-elements, including, but not limited to ::highlight().

@sanketj sanketj changed the title [css-highlight-api-4] Inline styles for HighlightRangeGroups should apply on top of styles provided by ::highlight [css-highlight-api] Inline styles for HighlightRangeGroups should apply on top of styles provided by ::highlight Dec 14, 2019
@frivoal
Copy link
Collaborator

frivoal commented Dec 16, 2019

More specifically, I think the way we should solve this is by:

  1. Droping the style attribute on Highlight
  2. Extending the CSSPseudoElement interface
    [Exposed=Window]
    partial interface CSSPseudoElement {
        attribute CSSStyleDeclaration style;
    };
  3. Defining this style attribute on the pseudo element to be a style attribute in the sense of css-style-attr and as handled in css-cascade
  4. Extending the type defined in css-pseudo-4 to also accept ::highlight(<name>) in addition to the ::before, ::after, and ::marker so that you can use someElement.pseudo("::highlight(foo)"

This would give us:

  • the ability to style a custom highlight entirely from Javascript (same as original MS explainer and current spec)
  • the ability to do so in a way that takes precedence over styles defined in a stylesheet (same as original MS explainer, different from current spec)
  • The ability to chose whether this style applies to the entire highlight (document.documentElement.pseudo("::highlight(foo)").style.cssText="color: pink") or to the section of it that is originating from a particular element (document.getElementById("id").pseudo("::highlight(foo)").style.cssText="color: pink") (new ability)
  • a fully defined model for how this works, consistent with the rest of the platform, and answering questions such as inheritance as raised in [css-highlight-api] How should inline highlight styles with 'inherit' values be treated? #4602, or var() or attr() or currentColor, or interaction with animations, etc... (not previously defined in original MS explainer or current spec)

@frivoal frivoal added the css-pseudo-4 Current Work label Dec 16, 2019
frivoal added a commit to frivoal/csswg-drafts that referenced this issue Dec 16, 2019
Use it in css-highlight-api as a replacement for the style attribute on
HighlightRangeGroup.

Closes w3c#4588
Closes w3c#4602
@frivoal
Copy link
Collaborator

frivoal commented Dec 16, 2019

Drafted the above proposal in Pull Request form: #4607

@sanketj
Copy link
Member Author

sanketj commented Dec 19, 2019

Yes, I definitely see inline styles for custom highlights working similar to how they do for DOM elements. In that sense, I agree we should try to explore making this work generically for all pseudo elements. Currently though, the CSSPseudoElement interface only supports ::before, ::after and ::marker, which are all pseudos that do not inherit styles from other pseudos and also do not apply across multiple elements. To make CSSPseudoElement work with the highlight pseudos (and possibly ::first-line), I think we have to define a new event path. And if we're doing this, we should make sure we define it in a way that works with highlight events as well. (I left a comment about this in the PR.)

frivoal added a commit to frivoal/csswg-drafts that referenced this issue Dec 20, 2019
Use it in css-highlight-api as a replacement for the style attribute on
HighlightRangeGroup.

Closes w3c#4588
Closes w3c#4602
@frivoal frivoal changed the title [css-highlight-api] Inline styles for HighlightRangeGroups should apply on top of styles provided by ::highlight [css-highlight-api] Inline styles for Highlight should apply on top of styles provided by ::highlight Dec 20, 2019
frivoal added a commit to frivoal/csswg-drafts that referenced this issue Dec 20, 2019
Use it in css-highlight-api as a replacement for the style attribute on
Highlight.

Closes w3c#4588
Closes w3c#4602
@sanketj
Copy link
Member Author

sanketj commented Dec 20, 2019

@frivoal After thinking some more about this (particularly in regard the scenarios we discussed today), I'm starting to wonder if we really need to support inline styling of highlights as part of css-highlight-api-1? The .style property provides a nice convenience, but styling with ::highlight seems generally good enough. Additionally, in event scenarios, authors likely want to change the style for one range, in which case setting .style on an entire group isn't helpful. Instead, the author will probably add the range to a different highlight and style that.

Pulling .style out would allow us to simplify the cascade interactions for this spec and we wouldn't need to depend on the outcomes of CSSPseudoElement. Additionally, once CSSPseudoElement does support .style and its scope expands to non-tree abiding pseudos, we would be able to inline style highlight pseudos in the same way as other pseudos.

@frivoal
Copy link
Collaborator

frivoal commented Dec 20, 2019

That makes sense to me. The #4607 Pull Request already greatly reduces the coupling, as after it, basically all that's left regarding style in the css-highlight-api spec is:

  • a mention that CSSPseudoElement.style exists
  • a requirement that Element.pseudo() accepts "::highlights()"
  • an example

But indeed, the style attribute is a mere convenience, and it's not critical to the custom highlight api, so we could go further and drop it entirely from the spec, and just wait on css-pseudo defining the OM-based style attribute.

What would you rather do?

@sanketj
Copy link
Member Author

sanketj commented Dec 20, 2019

I'd prefer to remove .style entirely for now because I think there's more work needed before CSSPseudoElement can be used with non-tree abiding pseudo elements, even just for styling. To be able to apply an 'inline' style to a highlight with CSSPseudoElement.style, we'll need to figure out a way to reference the pseudo or its owning element from the highlight. (Perhaps we'll need to define something like Highlight.pseudo()?) This is going to require some design that's probably better handled separately, either as part of css-pseudo, or as part of a future revision of this spec after CSSPseudoElement has made more progress.

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

2 participants