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

New ambiguous dfns #5958

Closed
npm1 opened this issue Sep 29, 2020 · 22 comments
Closed

New ambiguous dfns #5958

npm1 opened this issue Sep 29, 2020 · 22 comments
Labels
meta Changes to the ecosystem around the standard, not its contents.

Comments

@npm1
Copy link
Contributor

npm1 commented Sep 29, 2020

I tried regenerating longtasks on bikeshed and found that there are a couple of conflicting dfns:

@domenic
Copy link
Member

domenic commented Sep 29, 2020

We can't afford to never use terms twice in HTML; that's not a constraint on HTML editing we can accept. This "ambiguity" is, IMO, a problem with Bikeshed's data model, where unqualified links (with no for) get counted as ambiguous, instead of defaulting to for=/. You can opt in to the better data model with "Use Explicit For". Then edits to other specs which introduce a second instance of a term will not break your build.

For the browsing context case, the issue is that for some reason Bikeshed is counting the navigation param one as exported. However, it is not exported. So there's not much we can do on the HTML side.

/cc @tabatkins

@annevk
Copy link
Member

annevk commented Sep 29, 2020

I think we can add data-noexport. I believe that for HTML Shepherd defaults to export for dfn.

@annevk annevk added the meta Changes to the ecosystem around the standard, not its contents. label Sep 29, 2020
@domenic
Copy link
Member

domenic commented Sep 29, 2020

Could we change Shepherd to not do that?

@tabatkins
Copy link
Contributor

I presume Shepherd does that specially for HTML because, since HTML doesn't generally annotate its definitions properly and still largely relies on Shepherd heuristics, doing so would mean a ton of HTML definitions are treated as unexported.

Unless that's changed, and every dfn-type definition you want exported has been marked accordingly?

@domenic
Copy link
Member

domenic commented Sep 29, 2020

I don't know if we've gotten there yet, but I'd rather move toward that world, especially now that we have some harm being caused by the current setup.

(I also think it's pretty unlikely that Shepherd defaults to export for dfn; otherwise large anchor blocks like https://github.com/WICG/portals/blob/3c57d0d78dda6d7a5e551ed701957583ae91d124/index.bs#L25-L46 would not be necessary. Maybe there's some heuristic involved instead.)

@annevk
Copy link
Member

annevk commented Sep 30, 2020

@plinss would know. Is there a dataset we could inspect somewhere?

@plinss
Copy link

plinss commented Sep 30, 2020

Shepherd default exports all anchors that have a known type other than a bare dfn, not just those in the HTML spec.

Ideally it determines the anchor type from the data-dfn-type attribute, but it has all sorts of heuristics to guess the anchor type from context when it's not explicitly provided. A lot of those are specific to the HTML spec and if we removed them we would indeed drop a whole lot of anchor types.

I would also much prefer that the HTML spec grows the explicit attributes to specify the anchor types, just like Bikeshed and Respec do, and then we can remove the heuristics.

A data-noexport attribute will always cause an anchor to not be exported.

If you want to look at Shepherd's output, you can dump the entire anchor database via: https://api.csswg.org/sheperd/spec/?anchors=1&drafts=1 (you can restrict that to specific specs by adding &spec=) or you can query for individual anchors at https://respec.org/xref/ or via Bikeshed's command line.

@dbaron
Copy link
Member

dbaron commented Oct 11, 2020

I'd note that the "browsing context" issue here is helped by neither Assume Explicit For: true, nor a link-default of spec:html; type:dfn; for:/; text:browsing context, nor both. (The TAG's design principles document used to hit this error (and it had that link-default in question) but a recent modification to the document no longer hits it.)

In particular, the following test bikeshed document hits it:

<pre class="metadata">
Title: Web Platform Design Principles
Group: tag
Shortname: design-principles
Repository: w3ctag/design-principles
Status: ED
Level: none
ED: https://w3ctag.github.io/design-principles/
Editor: Sangwhan Moon, Invited Expert, https://sangwhan.com
Abstract: This document contains a set of design principles to be used when designing Web Platform technologies. These principles have been collected during the Technical Architecture Group's discussions in <a href="https://github.com/w3ctag/design-reviews/">reviewing</a> developing specifications. We encourage specification designers to read this document and use it as a resource when making design decisions.
Assume Explicit For: true
</pre>

<pre class="link-defaults">
spec:html; type:dfn; for:/; text:browsing context
</pre>

<p>Link to <a>browsing context</a>.</p>

and produces the bikeshed output:

LINK ERROR: Multiple possible 'browsing context' dfn refs for '/'.
Arbitrarily chose https://html.spec.whatwg.org/multipage/browsers.html#browsing-context
The following refs show up multiple times in their spec, in a way that Bikeshed can't distinguish between. Either create a manual link, or ask the spec maintainer to add disambiguating attributes (usually a for='' attribute to all of them).
spec:html; type:dfn; for:/; text:browsing context
  https://html.spec.whatwg.org/multipage/browsers.html#browsing-context
  https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigation-params-browsing-context
<a data-link-type="dfn" data-lt="browsing context">browsing context</a>
 ✔  Successfully generated, with 1 linking errors

dbaron added a commit to dbaron/design-principles that referenced this issue Oct 11, 2020
This removes the "browsing context" hack that was added in 470b2a7 (part of that commit) in w3ctag#242, which is no longer needed because the referencing text was removed in 4f5f94c; the underlying issue whatwg/html#5958 still exists.

It removes the `line-height` hack (added in 8ec8a21 in w3ctag#243) that was needed for unknown reasons and is no longer needed.
dbaron added a commit to w3ctag/design-principles that referenced this issue Oct 12, 2020
This removes the "browsing context" hack that was added in 470b2a7 (part of that commit) in #242, which is no longer needed because the referencing text was removed in 4f5f94c; the underlying issue whatwg/html#5958 still exists.

It removes the `line-height` hack (added in 8ec8a21 in #243) that was needed for unknown reasons and is no longer needed.
@npm1
Copy link
Contributor Author

npm1 commented Nov 27, 2020

I see another set of ambiguous refs according to bikeshed:

The following refs show up multiple times in their spec, in a way that Bikeshed can't distinguish between. Either create a manual link, or ask the spec maintainer to add disambiguating attributes (usually a for='' attribute to all of them).
spec:html; type:element-attr; for:object; text:name
  https://html.spec.whatwg.org/multipage/iframe-embed-object.html#attr-object-name
  https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#attr-fe-name

@tabatkins
Copy link
Contributor

Yup, this one's just straight ambiguous, since the "form control" name dfn claims its also for object, clashing with the explicitly-for-object name definition.

domenic added a commit that referenced this issue Dec 1, 2020
@domenic
Copy link
Member

domenic commented Dec 1, 2020

I'll fix that one.

Did we ever figure out the browsing context issue? See #6054 (comment) where it appears Bikeshed is treating it as exported even though it contains export: False

@annevk
Copy link
Member

annevk commented Mar 11, 2021

No, I still see the double browsing context thing show up, currently in Mixed Content. Should we just add a for attribute even though it's not exported?

@domenic
Copy link
Member

domenic commented Mar 11, 2021

I'd really like to get to the bottom of why the Bikeshed database is using marked-as-not-exported terms. That is, Bikeshed says export: False but somehow still generates conflicts.

@annevk
Copy link
Member

annevk commented Mar 11, 2021

Right, okay, I guess that comes down to asking @plinss to disable Shepherd heuristics now that we have marked things for exporting explicitly. Or maybe @tabatkins has access to that as well?

@domenic
Copy link
Member

domenic commented Mar 11, 2021

That might work, but it seems like there's a separate issue where Bikeshed is picking up things even if the Shepherd heuristic says they should not be exported: see #6054 (comment)

@annevk
Copy link
Member

annevk commented Mar 11, 2021

Thanks, filed speced/bikeshed#1982 in the hope of moving that forward. "Fortunately" W3C doesn't use fatal Bikeshed mode yet so nobody is blocked on us, but it's not pretty either.

@tabatkins
Copy link
Contributor

Hm, I'm no longer seeing this issue locally at all; [=/browsing context=] automatically picks up the sole correct definition, and [=browsing context=] is ambiguous, but only between the three exported definitions.

Let me go see what Mixed Content is doing...

@tabatkins
Copy link
Contributor

Ah, I see what the problem is. Mixed Content contains a link-defaults entry specifying:

spec:html; type:dfn; for:/; text:browsing context

And one of the side-effects of link-defaults is that it treats the matching definitions as exported, because you explicitly said that you wanted to use them.

So since HTML does still have two distinct definitions that are identical under the linking model, and both of them get turned on by this default, you suddenly have ambiguity and an extra warning message about indistinguishability.

I could possibly address this by checking if multiple entries get selected by the link-defaults fixer, and then if exactly one of them is already exported leaving them as-is.

But I think the right fix is in the source, to give the second browsing-context dfn a for="navigation params" or something.

@annevk
Copy link
Member

annevk commented Mar 11, 2021

Thanks for debugging, that will help if we run into this again. I created w3c/webappsec-mixed-content#47.

Let's close this.

@annevk annevk closed this as completed Mar 11, 2021
@domenic
Copy link
Member

domenic commented Mar 11, 2021

But I think the right fix is in the source, to give the second browsing-context dfn a for="navigation params" or something.

This doesn't seem right to me. We shouldn't need to add Bikeshed dfn contract things to explicitly non-exported terms... we should be free to use dfn internally without worrying about that.

@tabatkins
Copy link
Contributor

Okay, so what you want is something stronger than "not exported", which is intended to be overrideable when desired, but rather "this definition must never be referenced by anyone outside of this spec", and you want this to be triggerable while still using <dfn> for the element in your spec markup. Right?

@domenic
Copy link
Member

domenic commented Mar 11, 2021

Well, I guess, but what I really want is for that to be the default, so that we don't have to use anything Bikeshed-specific in a non-Bikeshed-doc's non-exported <dfn> elements.

From that perspective, I think the fix you described

I could possibly address this by checking if multiple entries get selected by the link-defaults fixer, and then if exactly one of them is already exported leaving them as-is.

sounds pretty good. The way I would phrase it is, first consider exported definitions, and see if that satisfies the request to set a default. If you then need to fall back to non-exported definitions, OK, but now you're in dangerous territory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Changes to the ecosystem around the standard, not its contents.
Development

Successfully merging a pull request may close this issue.

6 participants