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.

Sign up for GitHub

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

Jump to bottom

Link legacy APIs within the advisements #416

Merged
merged 22 commits into from
Aug 24, 2017
Merged

Link legacy APIs within the advisements #416

merged 22 commits into from
Aug 24, 2017

Conversation

tobie
Copy link
Collaborator

@tobie tobie commented Aug 17, 2017 •
edited by pr-preview bot
Loading

Make advisements more prominent.

  • [LenientSetter]
  • [LenientThis]
  • [NamedConstructor]
  • [OverrideBuiltins]
  • [TreatNonObjectAsNull]
  • [TreatNullAs=EmptyString] (didn't gather all examples of legacy usage).
  • [LegacyUnenumerableNamedProperties]
  • [LegacyArrayClass]
  • [NoInterfaceObject] (didn't gather all examples of legacy usage).

Preview | Diff

@tobie
Link legacy APIs within the advisements
a9ed4dd 
Make advisements more prominent.
@tobie tobie added the editorial Changes that do not affect how the standard is understood label Aug 17, 2017
@tobie tobie requested review from domenic and annevk August 17, 2017 10:27
annevk
annevk reviewed Aug 17, 2017
View reviewed changes
index.bs Outdated
text: HTMLImageElement; url: htmlimageelement
text: onmouseenter; for: Document; url: handler-onmouseenter
text: onmouseleave; for: Document; url: handler-onmouseleave
text: onreadystatechange; for: Document; url: handler-onreadystatechange
Copy link
Member

Choose a reason for hiding this comment

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

These should be all be exported if they're not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in whatwg/html#2936 and whatwg/html#2937.

index.bs Outdated
type: interface;
text: fullscreenEnabled; for: Document; url: dom-document-fullscreenenabled
text: fullscreen; for: Document; url: dom-document-fullscreen
text: fullscreenElement; for: DocumentOrShadowRoot; url: dom-document-fullscreenelement
Copy link
Member

Choose a reason for hiding this comment

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

These too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think these are exported by default but that fullscreen isn't picked up by Shepherd? Maybe @plinss or @tabatkins can confirm and fix if my assumption is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think that's correct.

index.bs Outdated
@@ -8644,8 +8664,24 @@ in strict mode to be ignored rather than causing an exception to be thrown.
<a href="mailto:[email protected]">[email protected]</a>
mailing list before proceeding.

<small class="non-normative">
Copy link
Member

Choose a reason for hiding this comment

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

Why no "advisement"?

Copy link
Collaborator Author

@tobie tobie Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

"advisement" is normative, and thus the specs where the linked APIs are defined end-up in the normative parts of the bibliography.

index.bs Outdated
unless required for compatibility reasons. Specification authors who
wish to use this feature are strongly advised to discuss this on the
<a href="mailto:[email protected]">[email protected]</a>
mailing list before proceeding.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe open an issue on GitHub instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I just moved that text around. I'll fix it while I'm at it.

domenic
domenic reviewed Aug 17, 2017
View reviewed changes
index.bs Outdated
unless required to specify the behavior of legacy APIs.

<small class="non-normative">
The [{{LegacyWindowAlias}}] [=extended attribute=] appears on the following [=interfaces=]:
Copy link
Member

Choose a reason for hiding this comment

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

I am not really in favor of this because maintaining a reverse-registry kind of defeats the point of Web IDL (in creating a toolset anyone can use).

Listing maybe one example is fine. But saying "here are all the places it appears" exhaustively is not good and will either be out of date or interpreted as a requirement to add your use to here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see that as a feature given we explicitely request people to discuss each new use case on the list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy to meet you in the middle, however, and have an exhaustive list which isn't presented as such.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess that's fair... it just feels a bit backward... I dunno, I guess I'll let you decide, or maybe others can weigh in.

Copy link
Collaborator Author

@tobie tobie Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It just feels a bit backward...

I agree. I'm fighting my own internal purist here to just do the pragmatic thing (which is to list these things and be done with it).

@tobie
Copy link
Collaborator Author

tobie commented Aug 17, 2017

As a sidenote, this should make tackling #350 a lot easier.

TimothyGu
TimothyGu reviewed Aug 17, 2017
View reviewed changes
index.bs Outdated
@@ -8546,6 +8559,26 @@ entails.

<h4 id="LegacyWindowAlias" extended-attribute lt="LegacyWindowAlias">[LegacyWindowAlias]</h4>

<div class="advisement">

[{{LegacyWindowAlias}}] [=extended attributes=] are an undesirable feature.
Copy link
Member

Choose a reason for hiding this comment

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

Singular?

@tobie tobie force-pushed the link-legacy-apis branch from 8d80686 to 0225e14 Compare August 22, 2017 13:08
@tobie tobie force-pushed the link-legacy-apis branch from 7a14655 to 1782219 Compare August 22, 2017 14:14
TimothyGu
TimothyGu reviewed Aug 22, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

I'm in favor of this change as well, as it can help limit any new usages of these extended attributes.

index.bs Outdated
{{DOMRectList}},
{{MediaList}},
{{StyleSheetList}}, and
{{CSSRuleList}}. [[GEOMETRY]] [CSSOM]
Copy link
Member

Choose a reason for hiding this comment

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

Missing brackets around CSSOM?

@tobie
Copy link
Collaborator Author

tobie commented Aug 22, 2017

OK, this is ready for final review.

@tobie tobie changed the title [WIP] Link legacy APIs within the advisements Link legacy APIs within the advisements Aug 22, 2017
domenic
domenic requested changes Aug 24, 2017
View reviewed changes
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.

All "file an issue" links go to the wrong repository. Several of them have the wrong text too, I believe.

Otherwise looks good.

Maybe worth sneaking in "Default Biblio Status": "current" to the header while here since several of the new references go to bad places.

@tobie
Copy link
Collaborator Author

tobie commented Aug 24, 2017

All "file an issue" links go to the wrong repository. Several of them have the wrong text too, I believe.

That is pretty bad. I have no idea how that happened, but thanks for catching it!

@tobie tobie merged commit f869be8 into whatwg:master Aug 24, 2017
@tobie tobie deleted the link-legacy-apis branch August 24, 2017 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

@TimothyGu TimothyGu TimothyGu left review comments

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
editorial Changes that do not affect how the standard is understood
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobie @domenic @TimothyGu @annevk