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

Support dfn types short syntax, "for", (no)export #134

Merged

Conversation

sideshowbarker
Copy link
Contributor

@sideshowbarker sideshowbarker commented Aug 29, 2020

This change adds support for doing the following with attributes of dfn elements:

  • recognize attributes with names that are known dfn types (currently: element, element-attr, event, interface, extended-attribute, method, attribute, enum-value, and http-header), and replace them with corresponding data-dfn-type=element, etc., attributes in output

  • for any dfn element that has an attribute named for, replace it in output with an attribute named data-dfn-for

  • for any dfn element with an export or noexport attribute , replace it with one named data-export or data-noexport

Relates to whatwg/html#5694


This patch does what I’ve inferred the desired behavior is, based on what I understand from the whatwg/html#5694. But it’s possible I’m misunderstanding. If so, I’m quite happy to iterate on this further (or completely re-do it…).

This change adds support for doing the following with attributes
of dfn elements:

* recognize attributes with names that are known dfn types (currently,
  'element', 'element-attr', 'event', 'interface', 'extended-attribute',
  'method', 'attribute', 'enum-value', and 'http-header'), and replace
  them with data-dfn-type=element, etc., attributes in output

* for any dfn element that has an attribute named "for", replace it in
  output with an attribute named "data-dfn-for"

* for any dfn element with an attribute named "export" or "noexport",
  replace it with one named "data-export" or "data-noexport"

Relates to whatwg/html#5694
@domenic
Copy link
Member

domenic commented Aug 29, 2020

Will give this a closer look on Monday, but at first glance your inference seems exactly right. I'm very excited about this!

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM with suggested additions. Have you tested this on whatwg/html#5694? In particular it'd be good to make sure this code runs at the right "phase" so that things end up as expected; I feel like our MDN panels work ran into phase problems so it's good to check.

Hmm, also, can we strip these attributes out of the developer edition and review drafts?

src/wattsi.pas Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/whatwg/html/pull/5694/files contains "noexport" but this doesn't translate those.

@sideshowbarker
Copy link
Contributor Author

https://github.com/whatwg/html/pull/5694/files contains "noexport" but this doesn't translate those.

oops, thanks for catching that — will fix

...because this got overlooked in the previous commit.
This change causes Wattsi to fail with an error message if a dfn that
has a type name is found to also have an export attribute.
@sideshowbarker
Copy link
Contributor Author

https://github.com/whatwg/html/pull/5694/files contains "noexport" but this doesn't translate those.

oops, thanks for catching that — will fix

Fixed now

This change makes Wattsi recognize the full set of 40 dfn types from
https://github.com/tabatkins/bikeshed/blob/master/bikeshed/config/dfnTypes.py#L7 —
rather than just the subset of 9 types the initial code recognized.

Two dfn types that are used in HTML but that weren’t part of that
initial subset (and that therefore weren’t getting converted as
expected) are “attr-value” and “constructor”.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all so awesome. Thanks very much. I'm happy to merge, but we could also do the lt="" here (I'll go comment on that now). So up to you.

@sideshowbarker
Copy link
Contributor Author

This is all so awesome. Thanks very much. I'm happy to merge, but we could also do the lt="" here (I'll go comment on that now). So up to you.

Let’s go ahead and merge this and then I can follow up with a separate patch for the lt="" change.

@sideshowbarker sideshowbarker merged commit ff89d46 into master Sep 15, 2020
@sideshowbarker sideshowbarker deleted the sideshowbarker/bikeshed-short-syntax-for-dfn-types branch September 15, 2020 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants