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

Remove <applet> #1399

Merged
merged 5 commits into from
Aug 21, 2017
Merged

Remove <applet> #1399

merged 5 commits into from
Aug 21, 2017

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jun 7, 2016

  • Removed applet from document.all[name] collection.
  • Removed applet from document[name] collection.
  • Removed applet from window[name] collection.
  • The document.applets collection now returns an empty collection.
  • Removed handling for applet in <iframe sandbox>.
  • Removed rendering rules for applet.
  • Removed the element itself and HTMLAppletElement.
  • <applet> now uses HTMLUnknownElement.

The parser is intentionally not changed.

Fixes #454.


Bugs:
Chromium: https://crbug.com/470301 (removed support in Sep 2015)
WebKit: https://bugs.webkit.org/show_bug.cgi?id=157926
Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1279218
Edge: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11946645/

@zcorpan zcorpan added the removal/deprecation Removing or deprecating a feature label Jun 7, 2016
@annevk
Copy link
Member

annevk commented Jun 9, 2016

LGTM.

@domenic
Copy link
Member

domenic commented Jun 9, 2016

Missed the obsolete attributes section:

datasrc on a, applet, button, div, frame, iframe, img, input, label, legend, marquee, object, option, select, span, table, and textarea elements
datafld on a, applet, button, div, fieldset, frame, iframe, img, input, label, legend, marquee, object, param, select, span, and textarea elements

I am a little concerned that this removal is premature given that applet is currently implemented in 3/4 rendering engines. We have signs Edge is interested in removal, but we have heard negative signs from Gecko and no signs from WebKit.

Our precedents for removing features of this magnitude includes:

In contrast, we have

My thoughts would be to tag this with "do not merge yet" until we get a clearer sign that this is on track for removal across the board.

@zcorpan zcorpan added the do not merge yet Pull request must not be merged per rationale in comment label Jun 9, 2016
@zcorpan
Copy link
Member Author

zcorpan commented Jun 9, 2016

Yeah, I agree.

I left datasrc and datafld alone because they are the elements on which they worked in IE. Those attributes were never part of any HTML standard. I could be convinced that those attributes should not be listed there in the first place...

@domenic
Copy link
Member

domenic commented Jun 9, 2016

I left datasrc and datafld alone because they are the elements on which they worked in IE.

Well, everything else in that section refers to an element that is defined in the spec, per the note at the top:

The following attributes are obsolete (though the elements are still part of the language), and must not be used by authors:

In other words, the rest of the section says "on element X, which exists, the attribute Y no longer exists." Whereas if we leave applet in there, it'll be saying "on element X, which no longer exists, the attribute Y no longer exists." Kind of redundant.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 10, 2016

Yep, but that's also true for frameset, frame and marquee which are listed. But we can remove those too.

@domenic domenic removed their assignment Jun 20, 2016
@domenic
Copy link
Member

domenic commented Jun 20, 2016

Yep, but that's also true for frameset, frame and marquee which are listed.

Those are all defined in the spec... authors aren't suppose to use them, but they're defined. After this change, applet will not be.

@zcorpan zcorpan added the needs tests Moving the issue forward requires someone to write tests label Nov 3, 2016
@domenic domenic mentioned this pull request Nov 22, 2016
@qdot
Copy link

qdot commented May 3, 2017

We've got a bug and an assignee (me) for removal of the applet tag in Gecko. https://bugzilla.mozilla.org/show_bug.cgi?id=1279218

@annevk
Copy link
Member

annevk commented May 3, 2017

@domenic given that Firefox is close to landing this per @qdot should we go ahead (after rebase) or wait until Firefox has shipped?

@annevk
Copy link
Member

annevk commented May 3, 2017

(Close to landing is inaccurate, my bad. It's more that we've now got @qdot looking into it.)

@zcorpan
Copy link
Member Author

zcorpan commented May 3, 2017

I think we might as well wait until Firefox has shipped.

@rniwa
Copy link
Collaborator

rniwa commented May 9, 2017

I think we should wait until at least one major browser ships this change in its stable release. It's unlikely that WebKit removes this element in near future regardless, however, due to compatibility concerns.

@zcorpan
Copy link
Member Author

zcorpan commented May 9, 2017

@rniwa Chromium removed support in Sep 2015, so has been shipping in Chrome and Opera for a while now.

@zcorpan
Copy link
Member Author

zcorpan commented May 9, 2017

I filed an issue for Edge now as it seems there wasn't one. https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11946645/

We also still need tests for this. Do Chromium or Gecko have tests or plan to write/change tests that can be upstreamed to web-platform-tests?

@annevk
Copy link
Member

annevk commented May 9, 2017

I suspect Gecko would do that, though help appreciated. Running grep applet -r * against web-platform-tests I get quite a few hits, including I think at least one browser test for it being obsolete, but also lots that assume it still works.

@qdot
Copy link

qdot commented May 24, 2017

I'll handle the web platform test changes as part of the landing criteria for applet removal in Gecko.

Just curious, do we also want to deprecate code/codebase/codetype/archive/classid applet parameters that are reflected in object?

@domenic
Copy link
Member

domenic commented May 25, 2017

Great question. Interestingly all but classid are in the obsolete section of the spec; classid affects the object processing model. See "whenever one of the following conditions occur" in the object element. (Although I believe there are open issues about the object processing model not being super accurate, so I am not sure if that's true...)

Does anyone know if classid has an effect on, e.g., Flash?

It may just be easiest to leave them alone. But if implementers are up for it, it might be nice to remove this cruft too. It does seem unlikely that anyone is relying on the reflection of those Java-specific attributes into the object element given lack of Java support... @foolip, maybe you can give Chromium thoughts?

@qdot
Copy link

qdot commented May 25, 2017

In gecko, classid was only used for java (See comments near http://searchfox.org/mozilla-central/source/dom/base/nsObjectLoadingContent.cpp#1625), which is why I included it as a possible candidate for deprecation. We're removing a ton of java support code along with applet, so all that code will be excised anyways.

@annevk
Copy link
Member

annevk commented May 25, 2017

@qdot according to https://lists.w3.org/Archives/Public/public-whatwg-archive/2006Aug/0134.html by @bzbarsky, back in the day Fx used to ignore the type attribute if classid was present and then go to fallback if the classid was not supported. So if we'd ignore classid altogether you'd not end up in fallback but rather end up dispatching on type. I don't know if that changed meanwhile though or if changing that code path is problematic in practice.

@zcorpan
Copy link
Member Author

zcorpan commented May 27, 2017

I prefer to leave the reflecting attributes alone. We have lots of obsolete reflecting attributes, they are cheap to support and there's non-zero risk in removing them.

@foolip
Copy link
Member

foolip commented May 29, 2017

@domenic, in Chromium, the classid attribute still appears in HTMLObjectElement, where at least part of it points back to the HTML spec:
https://chromium.googlesource.com/chromium/src/+/d5bf6c6f5651782d2fe6db6194d7e9776e2aff35/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp#248

That's all what the element "represents", i.e. layout. AFAICT, the classid attribute isn't used to actually load anything, I can't find anything corresponding to this: "If the classid attribute is present, and has a value that isn't the empty string, then: if the user agent can find a plugin suitable according to the value of the classid attribute, and either plugins aren't being sandboxed or that plugin can be secured, then that plugin should be used, and the value of the data attribute, if any, should be passed to the plugin. If no suitable plugin can be found, or if the plugin reports an error, jump to the step below labeled fallback."

Not sure what to make of that.

If the reflected attributes are implemented everywhere, then keeping them even if the attributes do nothing would make sense to me.

@domenic
Copy link
Member

domenic commented May 30, 2017

Sounds like we'll leave all the reflecting attributes in. I'll file a separate issue to investigate the object processing model.

@domenic
Copy link
Member

domenic commented May 30, 2017

Actually, I guess it is fine. That step about using classid is pretty vague and it could be argued Chrome is never able to find a plugin using classid.

I think there's a larger cleanup to be done around the <object> processing model as implementations start removing support for everything-but-Flash (or even removing Flash, as some mobile implementations have done). Related: #387, where it's made clear that we're not even capturing the current heuristics very correctly. But we can leave that for later...

@rniwa
Copy link
Collaborator

rniwa commented May 30, 2017

Note that Safari does support non-Flash plugins.

@domenic
Copy link
Member

domenic commented May 30, 2017

Hmm. I think Safari started giving back empty arrays for navigator.plugins and navigator.mimeTypes, and that threw me off? Upon further reflection, that is definitely separate from what it actually supports in <object>.

@domenic
Copy link
Member

domenic commented Jul 17, 2017

It looks like Mozilla is implementing this; we should get ready to land, and also we should put some tests together. I can try to find time myself.

zcorpan and others added 3 commits July 31, 2017 11:35
* Removed applet from document.all[name] collection.
* Removed applet from document[name] collection.
* Removed applet from window[name] collection.
* The document.applets collection now returns an empty collection.
* Removed handling for applet in <iframe sandbox>.
* Removed rendering rules for applet.
* Removed the element itself and HTMLAppletElement.
* <applet> now uses HTMLUnknownElement.

The parser is intentionally not changed.

Fixes #454.
@annevk
Copy link
Member

annevk commented Jul 31, 2017

Rebased this PR. It does seem like WPT still needs updates since with https://hg.mozilla.org/integration/autoland/rev/e6f1f895449d @qdot simply rebased the results rather than fixed the tests...

@qdot
Copy link

qdot commented Jul 31, 2017

@annevk Sorry, was kind of on a deadline for getting this in, and I wasn't sure what order the WPTs needed to be updated in relative to the spec. I'll file a followup in bugzilla and get that taken care of?

@qdot
Copy link

qdot commented Jul 31, 2017

Ok, WPT updates happening at https://bugzilla.mozilla.org/show_bug.cgi?id=1386008

@annevk
Copy link
Member

annevk commented Aug 2, 2017

Rereading this thread I'm left unsure as to whether there's still outstanding feedback on the PR. In any case it needs another round of review.

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.

LGTM with my latest commit which removes a few extra things.

Copy link
Member Author

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

LGTM.

@annevk annevk removed needs tests Moving the issue forward requires someone to write tests do not merge yet Pull request must not be merged per rationale in comment labels Aug 21, 2017
@annevk annevk merged commit b9b9d60 into master Aug 21, 2017
@annevk annevk deleted the remove-applet branch August 21, 2017 13:19
domenic pushed a commit to jsdom/jsdom that referenced this pull request Nov 19, 2017
<applet> was removed from the spec in whatwg/html#1399.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal/deprecation Removing or deprecating a feature
Development

Successfully merging this pull request may close these issues.

None yet

6 participants