-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add general partial_implementation guideline
#26780
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
Conversation
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
|
I suppose one thing that is potentially missing here—or at least not directly mentioned—is some notion of severity: how do we know to use a note alone versus note+partial? A good example of this being a concern is on something like Safari's top-level await bug (#26510) or My inclination is to adopt a guideline like this, which doesn't require a judgment call on severity, and wait to see if we find a situation where the guideline requires us to do something overtly silly. |
caugner
left a comment
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.
LGTM so far, just one nit.
And one possible extension: What about BCD features with subfeatures? In general, we don't set partial_implementation: true on the parent feature, e.g. a CSS property, if the information can be captured on a subfeature, e.g. a CSS property value with partial_implementation: true or version_added: false.
Co-authored-by: Claas Augner <[email protected]>
If you wish to make this explicit you can, though we don't really document (for example) promoting
|
Yes, please, I like that. |
caugner
left a comment
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.
Some mostly structural suggestions to ease skimming, reading, and referencing:
|
As mentioned in #26838 (comment), I think that point 1...
... should be sufficient to mark support as partial if the missing behavior pertains to accessibility, especially if the lack of support isn't obvious to the layman as the feature seems to work. Alternatively, it would be nice to be able to prominently warn of accessibility pitfalls, even if it isn't through the |
|
@pygy For context, this PR is meant to regularize a process we already do here, so I don't think introducing new criteria is in scope. But for completeness, I'll say that I don't believe that deviation from a specification alone is a workable trigger for There is a concurrent proposal, #26781, to regularize "behavioral subfeatures" which typically capture the kind of relatively anonymous specification evolution that has gone on for #26838. That said, nothing in this PR (or the behavioral subfeature PR) should be understood to forbid (non-partial) notes about bugs and other limitations, accessibility or otherwise. |
| You must set `"partial_implementation": true` when all of the following conditions are met: | ||
|
|
||
| - The browser's support does not implement mandatory specified behavior. | ||
| - The browser's support is inconsistent with at least one other browser. |
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.
What if it's the other browser that's wrong?
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 what's meant here is:
At least one other browser implements this behavior.
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.
Ok. In that case, I'd suggest rephrasing.
Right now, this reads as "set partial implementation to true if this browser doesn't work like that other browser". And to me, I have no way of know which browser is right.
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.
@caugner (as you know and have commented) Given an interface, it is not uncommon for new properties or methods to be added later. At that point the parent interface becomes partial even if it was complete previously. My understanding though is that you wouldn't "go back" and capture that in the interface level. The interface would remain complete?
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.
My understanding of "complete" would be "complete at the time the data was written", so it would still hold true if another feature was added later. Does that need clarifying? It would be a real mess if people started going around retroactively marking data as partial because new features were added later.
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.
To answer some questions here:
Right now, this reads as "set partial implementation to true if this browser doesn't work like that other browser". And to me, I have no way of know which browser is right.
All three conditions must be met. The spec is almost always right—unless all implementers ignore the spec same exact way. It's possible to have partials for all implementers, if they all differ from the spec and they're not all mutually compatible.
At that point the parent interface becomes partial even if it was complete previously. My understanding though is that you wouldn't "go back" and capture that in the interface level. The interface would remain complete?
Partial data doesn't cascade to descendants and it doesn't bubble up to parents. Intl is probably the idealized case: Intl is fully supported ever since it was exposed because being exposed is all it does. The fact that Intl.Segmenter was added much later doesn't matter for Intl—we don't retroactively mark it as partial.
I'm not trying to propose any radical changes here. Instead, I'm trying to describe what we usually do most of the time anyway.
ddbeck
left a comment
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.
Some responses to @caugner's writing suggestions.
|
Here is an interesting case from WebDriver BiDi: The
Before Firefox 133, a size of 0x0 was accepted, and did not result in an I don't think it makes sense to set partial implementation here, even though strictly speaking this behavior
Am I missing something? |
|
@caugner that is an interesting example! I have a few thoughts on this: First, in an earlier draft (before I submitted the PR), I had a caveat about how the partial implementation must correspond to "reasonable developer behavior." I didn't want to allow situations where a contrived scenario would lead to a partial implementation when it would have no real-world meaning. I dropped it from the draft because I didn't quite know how to define "reasonable" but this is the kind of scenario I was thinking of. It doesn't much matter that Firefox and Chrome are incompatible because there's no compelling reason for a developer to expect producing a non-conforming dimensionless PDF to work. Second, looking into the both the bug and specification text seems to be in response to a challenge when it comes to testing the API (read w3c/webdriver-bidi#473). It seems like the sort of thing that we would have never known about except that:
At no point in this story was an ordinary web developer involved. This suggests to me that there's the possibility for a fourth condition to be added to the list:
I would take such a condition to mean that there has to be a bug reported (or replicated) by a developer in a situation that impacts real-world users (including preventing the feature from being used with real-world users). Or, in the case of a feature that has not yet shipped (e.g., in a beta), the authors and reviewers of the PR have good reason to believe that it will have that effect when it does ship. |
|
We discussed this on a special double-length BCD call today. Main takeaways:
|
|
This came up again in the BCD call today. I've applied a few changes:
@caugner and @Elchi3, I'd welcome re-review here. Thank you! |
Elchi3
left a comment
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.
Thanks Daniel, this looks good to me 👍
|
From today's BCD call: this still needs a link to this PR, for symmetry with other guidelines. I'll try to get to this shortly. |
Summary
This PR provides a general guideline for setting
partial_implementation.Related issues
This continues a discussion started in #26605 (comment).