-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Unmark Firefox as partial for ::highlight CSS selector
#28375
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
base: main
Are you sure you want to change the base?
Conversation
Also splits up the notes, adding bug links.
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
| "edge": "mirror", | ||
| "firefox": { | ||
| "version_added": "140", | ||
| "partial_implementation": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the partial stay.
I think it's hard to make the case that lack of support here does not have developer impact. Most critically, there is an interop proposal with upvotes.
Moreover, I don't think this represents some subtle or implicit behavior. Both the docs and the spec explicitly call out that 1) this pseudo only works with specific properties and 2) these two properties are included.
When this was originally added to BCD, it was noted by the implementer that it was "not feature complete" who suggested that the release notes ought to say "partial support" (note the discussion here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being able to apply a text decoration to a custom highlight is one of the most important use cases for this API (that and background color). So removing the partial flag without having support for text-decoration seems premature to me.
That being said, I was told support for this property is in Firefox Beta. So perhaps we could remove the partial flag but also update the version to match whatever Beta is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1845446, which landed in Firefox 146. But it also needs https://bugzilla.mozilla.org/show_bug.cgi?id=1987618.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that the test linked in the second bug is also failing in Safari. So whilst I agree that text-decoration is important here and was sufficient grounds to mark the implementation as partial, it's unclear to me that the remaining bug is sufficient to block changing the status. Or, if it is we need to also mark the Safari implementation as partial and have much better clarity on what's actually needed to be considered a full implementation, both in this case specifically, and as a matter of process for other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a bit with this demo my team and I made recently. If all you do is set text-decoration without doing anything else special, then it just works. However, if you start moving the line down with text-underline-offset, then at some point the line gets clipped and no longer appears.
On the one hand, it makes sense to consider this partial, because otherwise developers might get bitten by this buggy behavior.
On the other hand, 99% of cases are probably just going to work.
In any case, whether we decide to retain the partial flag or not, I agree that Safari and Firefox should match. There's no reason to block Firefox's support of the feature because of a partial flag when Safari is affected by the same bug.
My personal opinion: remove the partial flag, and make Firefox's version_added be 146.
| "Cannot be used with `text-decoration`. See [bug 1987618](https://bugzil.la/1987618)", | ||
| "Cannot be used with `text-shadow`. See [bug 1845447](https://bugzil.la/1845447)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do really like that this is two separate notes, with bugs for each. I do prefer it when notes are complete sentences, however. Something like:
| "Cannot be used with `text-decoration`. See [bug 1987618](https://bugzil.la/1987618)", | |
| "Cannot be used with `text-shadow`. See [bug 1845447](https://bugzil.la/1845447)" | |
| "The pseudo-element cannot be used with `text-decoration`. See [bug 1987618](https://bugzil.la/1987618)", | |
| "The pseudo-element cannot be used with `text-shadow`. See [bug 1845447](https://bugzil.la/1845447)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, because your suggestion makes the sentence 50% longer without adding any additional information not already included in the context.
Starting the notes with "Cannot be used" or similar makes it easier/faster for users to parse. In your suggestion, both notes share the first 38 characters, which is twice as much as "Cannot be used with".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are now three issues raised about the note here: reading speed, length, and completeness. Unfortunately, they're in competition with each other, but here's another try:
| "Cannot be used with `text-decoration`. See [bug 1987618](https://bugzil.la/1987618)", | |
| "Cannot be used with `text-shadow`. See [bug 1845447](https://bugzil.la/1845447)" | |
| "The `text-decoration` property doesn't work with `::highlight()`. See [bug 1987618](https://bugzil.la/1987618)", | |
| "The `text-shadow` property doesn't work with `::highlight()`. See [bug 1845447](https://bugzil.la/1845447)" |
To improve reading speed and skimmability, we ought to move unique text to the head of the notes, to avoid visual repetition from the left.
That makes it harder to go short. We could trim it down to something like "text-shadow doesn't work" but that starts to make real compromises on meaning. (I ran into this somewhat often in the notes audit, where some notes were so terse I had to read the linked bug anyway.)
My new suggestion still solves the completeness problem. Mostly I suggest complete sentences because, while shorter notes look fine on MDN on desktop they look much worse on caniuse or MDN on mobile, where notes are not necessarily arranged horizontally with the subject of the note (or sometimes even on the same screen, in case of MDN on mobile).
(I think there are other good reasons for complete sentences, such as friendliness to English learners and professional pride, but I will not belabor the point further.)
Summary
Unmarks Firefox as partial for
::highlightCSS Selector, and splits up the existing notes, adding bug links.Test results and supporting details
I challenge the following requirement for partial implementation:
Related issues
Related: #28374