[css-highlight-api] maplike vs setlike

[frivoal]

from w3ctag/design-reviews#584 (comment)

@hober and I discussed this today in our VF2F. We noticed that HighlightsRegister is a setlike, but throws when a Highlight object with an existing registered name, which indicates that perhaps maplike would have been more appropriate? It's not ideal developer experience if developers need to iterate the entire data structure before registering their object, to check if an existing one of the same name exists. Maplike would also provide an easy way to get all ranges under a given name.

[tabatkins]

Yeah, setlike should only be used if object identity is sufficient for membership. (Or, if a derived key is instead used for uniqueness, adding something else with the same derived key causes replacement, but that's treading on thin ice.)

If uniqueness depends on a derived key and replacement isn't allowed, then it's not a setlike at all, it's a registration map with write-once semantics, and should be a maplike.

[sanketj]

@LeaVerou @hober @tabatkins

The original explainer did use maplike for this (the type was originally called HighlightMap), and the Highlight object (originally called HighlightRangeGroup) would only need to be named when it was being registered. The reason this was problematic was that the same Highlight object could be registered multiple times under different names. To prevent that, we would have to override put, and either iterate the entire data structure to check if any map entry already has that Highlight as its value (which has similar performance to what we do today), or require authors to create Highlights with a name and also register with that exact same name (which doesn't seem developer friendly).

That's why we went with the current approach. Do you think one of the other options is better, or do you have any suggestions for a different solution?

[LeaVerou]

@sanketj Just so I understand the problem: what was the issue with having the same highlight object registered multiple times under multiple names? Why did this need to be prevented?

[sanketj]

Even if we allowed the same highlight object to be registered under multiple names, only one name can be reliably used to style the highlight. If the highlight were to be styled using multiple names, in the case of conflicting styles, the styles for the last registered name would always be painted on top and there would be no way to 'prioritize' the styles for names that were registered earlier. This could be confusing for authors because the paint order may not reflect the expected cascade order. For example, consider the following style rules where the same highlight object is registered with names 'foo' and 'bar', and 'bar' was the last registered name:

#p1::highlight(foo) {
    color: yellow;
}

#p1::highlight(bar) {
    color: red;
}

#p1::highlight(foo) {
   color: green;
}

Each rule targets the same highlight and has the same specificity, therefore authors may expect the last rule to win and the color of the highlighted content to be green. In reality, #p1::highlight(foo) and #p1::highlight(bar) are independent and the highlight will be painted twice - once with the green color for 'foo' and once with the red color for 'bar'. Because 'bar' was registered later, the red color would be painted on top. Even if we used color: green !important for the third rule, the red color would still paint on top.

Though this is technically not a cascade violation, it is definitely confusing behavior. So I think it is best to enforce that a highlight object can only be registered with a single unique name.

[frivoal]

In short:

• registering the same highlight multiple times under different names -> they share the priority, which is confusing as @sanketj explained about -> switched from map to set
• registering a new highlight under an existing name could either replace the old one, or throw. throwing seems to allow for better detection of accidental collision between highlights. That said, we could change that.

Another alternative could be to switch back to a map, and do something if an already registered highlight gets registered again under a new name (throw? remove the older one?). Maybe you can help me think through something here: can we reliably test object equality in JS?

Or maybe we switch back to map, and allow duplicate entries (with at most, a warning in the console), and just advise authors not to do that…

[LeaVerou]

@sanketj Isn't this the same in terms of author experience as a new highlight being registered that spans the same text as another highlight? (which is allowed)

@frivoal

Maybe you can help me think through something here: can we reliably test object equality in JS?

It depends. What kind of equality would you like to test?

[sanketj]

@LeaVerou It is different from the overlapping highlight case because those are actually separate highlight objects. Authors will expect the styles for the two highlights to be independent, and they have the option to control which styles paint on top via the priority property. In the case where both names target the same highlight, it is confusing that the highlight gets two independent sets of styles. Additionally, authors cannot control the prioritization of conflicting styles - the styles for the name that was registered later will always paint on top.

That said, if all we are debating is maplike vs. setlike, I do agree with @frivoal that we could switch back to maplike and override the 'set' operation to throw if we try to register a previously registered name.

[sanketj]

Summarizing a few approaches based on this thread for our vF2F discussions:

Approach 1 (what is currently spec'ed):
Highlight objects are constructed with a name, which guarantees that each highlight will have exactly one name. HighlightRegistry is a setlike collection of registered Highlight objects. Multiple independent highlights may be constructed with the same name, but only one can be registered at any time. To avoid duplicate registrations, the HighlightRegistry.add API compares the new highlight's name with all previously registered highlight names. If the name was previously registered, the API throws and fails; otherwise, the add operation succeeds.

Approach 2:
Highlight objects are not associated with a name at construction. Instead, they are given a name when they are registered. HighlightRegistry is a maplike that takes a highlight name as key and Highlight object as value. This approach allows the same Highlight to be registered under multiple names, which seems problematic because this allows a highlight to have multiple independent sets of styles and no way to prioritize amongst them (see previous comments in this issue for more details).

Approach 3:
Same as approach 2, but disallow registration of the same highlight under multiple names by overriding the HighlightRegistry.put API. When registering a highlight, checks the values in the HighlightRegistry to see if the new highlight was previously registered. If the highlight was previously registered under a different name, throw and fail. Otherwise, the put operation succeeds.

[LeaVerou]

@LeaVerou It is different from the overlapping highlight case because those are actually separate highlight objects.

I'm not convinced this is a conceptual difference for authors vs an implementation detail.

Authors will expect the styles for the two highlights to be independent, and they have the option to control which styles paint on top via the priority property.

This sounds like it highlights a problem with the current design, rather than a reason to create an error condition. It sounds like the priority property is conceptually an attribute of the relation between Highlight object and name. If that's correct, perhaps it should be specified at registration?

I'm also not sure the inability to specify the priority for this edge case is that much of a problem.
I do agree that the code example presented in #5910 (comment) is indeed confusing, but it doesn't always follow that if the same highlight is registered under two names, both of these will be used in the stylesheet simultaneously.
E.g. authors may wish to register a highlight under many different names to give their users the option of which one to use, without having to keep multiple objects in sync. Off the top of my head, I can see this being useful for syntax highlighters, e.g. Prism does provide aliases for a lot of its tokens.

[css-meeting-bot]

The CSS Working Group just discussed maplike vs setlike, and agreed to the following:

• RESOLVED: Switch Highlights back to a maplike of (name)=> Highlight, add a Note about what happens if you register the same highlight with multiple names

The full IRC log of that discussion

<Rossen_> Topic: maplike vs setlike
<Rossen_> github: https://github.com//issues/5910
<TabAtkins> fantasai: A custom highlight is made of three things: the Highlight object (a bunch of ranges), then a priority to know where it stacks, then a name so it's addressable from teh stylesheet
<TabAtkins> s/fantasai/florian/
<TabAtkins> florian: Question is how to group these
<TabAtkins> florian: Could say priority is an attribute of the highlight object
<TabAtkins> florian: But what about the name?
<TabAtkins> florian: One is a Map where key is name and vlaue is Highlight
<TabAtkins> florian: Another is a Set where name is a property of Highlight.
<Rossen_> s/maplike vs setlike/Highlight API collection, maplike vs setlike/
<TabAtkins> florian: If you use a Map, you coudl potentially register the same highlight multiple times under multiple names.
<TabAtkins> florian: Has some odd ergonomics for user.
<TabAtkins> florian: You style ::custom-spelling and ::custom-grammar, but they have the same priority, it's got some odd stacking.
<TabAtkins> florian: Not *hard* to explain or implement, just unintuitive. We consider it a footgun.
<TabAtkins> florian: So we decided to make it impossible to set up the same highlight multiple times.
<hober> q+
<TabAtkins> florian: So we switched to a Set that throws if you register multiple times. We could also have a Set that just ignores if you set multiple.
<TabAtkins> florian: Or a Map that throws if you register multiple.
<TabAtkins> florian: Or we could just ignore it all and let it happen.
<leaverou_> q+
<TabAtkins> florian: Currently the spec has it as a Set where name is an attribute, and it throws if you register the same thing twice.
<TabAtkins> florian: But that's unusual.
<TabAtkins> florian: Simplest is a Map that doesn't throw, but that might not make sense. But maybe that's fine.
<TabAtkins> hober: This came up in a TAG review
<Rossen_> ack hober
<TabAtkins> hober: been several months so convo is swapped out by now...
<TabAtkins> hober: But i remember being surprised about, it would be common to write code that says "have I registered this yet? No? then add it"
<TabAtkins> hober: Might have a few libraries that use highlights for various purposes, and they don't want to clobber themselves
<TabAtkins> hober: With a Map that's super easy
<TabAtkins> hober: just check the key
<TabAtkins> hober: With a set you have to iterate
<TabAtkins> hober: We thought that's weird, we expected checking to see if it's registered to be a common op
<TabAtkins> hober: I think we saw the ergonomic downside as more of dev inconvenience than the dev registering the same thing multiple times
<TabAtkins> hober: So I think we're inclined to just let the dev hurt themselves if they want to register things twice
<TabAtkins> hober: Not a hill we want to die on, just want common operations to be easy.
<sanketj> q+
<TabAtkins> florian: With time that has passed since original decision, sounds good to me
<Rossen_> ack leaverou_
<TabAtkins> leaverou_: Agree with hober, she said most of what I wanted to
<TabAtkins> leaverou_: I think using try/catch every time you register a highlight isn't ideal
<TabAtkins> leaverou_: I think there might even be use-cases for doing this - providing aliases for the same highlight
<TabAtkins> leaverou_: Sanket pointed out the problem is the priority (can't set it in that case), but that sounds like priority is associated with the name rather than the Highlight... maybe it's misplaced?
<TabAtkins> florian: I thougth about that, but then how would you set it?
<TabAtkins> sanketj: Design I had in mind originally was every Highlight has one name and one priority, and that's reflected in the spec we have today.
<TabAtkins> sanketj: You could say the prio is associated with the name rather than the highlight, but you'd need a different data structure
<TabAtkins> Rossen_: What's your position on the change to use Map?
<iank_> sanketj: likely want an options arg for the ctor, e.g. "new Highlight({name: 'hi', priority: 2})"
<TabAtkins> Yeah, it'd take a sequence or options object instead, i guess
<iank_> can't it be a maplike with an additional convenince function?
<TabAtkins> sanketj: hober and leaverou_'s argumetns make sense. And I think associating prio with highlight is convenient; I'd prefer not to change that if there's not a big reason to
<TabAtkins> sanketj: If we do allow a highlight to have multiple names, what's the messaging around it?
<TabAtkins> sanketj: Lea mentioned a use-case; I always thought about this as making a separate Highlight object, then they can prio against each other properly
<TabAtkins> sanketj: Sounds like you were suggesting having the same highlight be prio'd two different ways, which might be more complicated
<Rossen_> q?
<TabAtkins> florian: I think the proposal is to just do it the naive way - same highligh under two names would share prio
<TabAtkins> florian: So painting order would use the fallback of registration order
<TabAtkins> florian: A bit limiting, but it's not there *because* of the use-case, it's just the natural fallout.
<TabAtkins> florian: And if people find ways to make it nice for them, sure.
<leaverou_> +1 to florian
<TabAtkins> florian: Probably it's a footgun, but possibly there's good things to come out of it
<iank_> e.g. ```interface Highlights { readonly maplike<string, Highlight>; addHighlight(Highlight or HightlightDictionary); }```
<TabAtkins> sanketj: Do we want something in the spec warning against it?
<leaverou_> Yes, this would need to be a note in the spec
<TabAtkins> sanketj: Seems could be unexpected results
<TabAtkins> florian: Don't think we should have a normative thing, but a Note saying this would be odd if you did it woudl be approriate
<TabAtkins> leaverou_: agreed
<TabAtkins> sanketj: We could just document the case from the issue
<TabAtkins> sanketj: Seems fine
<TabAtkins> Rossen_: Sounds like we're approaching a reoslution?
<TabAtkins> iank_: I left some IRC comments
<TabAtkins> iank_: You don't need to strictly make it a setlike - could add convenience functions
<TabAtkins> iank_: Dunno if this changes the discussion particularly
<TabAtkins> florian: Not sure what's being proposed to help here
<TabAtkins> iank_: could make it a readonly maplike, then provide an add() that takes a Highlight, plus a remove()
<TabAtkins> florian: Does that bring us back to what we have today?
<TabAtkins> sanketj: I think the impl is that the name would live on the highlight, and also be exposed as a key on the readonly maplike
<TabAtkins> TabAtkins: I don't think the3re's enough worth innovating in data structures, versus just letting authors do this
<TabAtkins> porposed resolution: Switch Highlights back to a maplike of (name)=> Highlight, add a Note about what happens if you register the same highlight with multiple names
<TabAtkins> leaverou_: Just wnat to make sure that if you register to an existing key, it doesn't throw - that was mentioned in the ear4ly options
<TabAtkins> florian: Right, normal Map behavior
<TabAtkins> RESOLVED: Switch Highlights back to a maplike of (name)=> Highlight, add a Note about what happens if you register the same highlight with multiple names