Bug 169337 - Drop support for non-standard document.all.tags()
Summary: Drop support for non-standard document.all.tags()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2017-03-07 20:21 PST by Chris Dumez
Modified: 2017-03-08 18:37 PST (History)
11 users (show)

See Also:


Attachments
Patch (7.01 KB, patch)
2017-03-07 20:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (954.44 KB, application/zip)
2017-03-07 21:10 PST, Build Bot
no flags Details
Patch (8.15 KB, patch)
2017-03-07 21:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-03-07 20:21:42 PST
Drop support for non-standard document.all.tags(). It is not part of the specification:
- https://html.spec.whatwg.org/multipage/infrastructure.html#the-htmlallcollection-interface

It is not supported by Firefox and its support was dropped from Chrome back in early 2014:
- https://src.chromium.org/viewvc/blink?view=revision&revision=166870
Comment 1 Chris Dumez 2017-03-07 20:24:13 PST
Created attachment 303767 [details]
Patch
Comment 2 Build Bot 2017-03-07 21:10:08 PST
Comment on attachment 303767 [details]
Patch

Attachment 303767 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3263329

New failing tests:
fast/dom/HTMLDocument/document-all.html
Comment 3 Build Bot 2017-03-07 21:10:12 PST
Created attachment 303774 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 4 Chris Dumez 2017-03-07 21:24:16 PST
Created attachment 303778 [details]
Patch
Comment 5 Daniel Bates 2017-03-08 09:15:17 PST
document.all.tags() is a proprietary Microsoft extension and is supported in IE 11.
Comment 6 Daniel Bates 2017-03-08 09:17:27 PST
Is the Google UseCounter data referenced in the commit message for <https://src.chromium.org/viewvc/blink?view=revision&revision=166870> an accurate representation of enterprise customers?
Comment 7 Chris Dumez 2017-03-08 09:58:16 PST
(In reply to comment #5)
> document.all.tags() is a proprietary Microsoft extension and is supported in
> IE 11.

https://msdn.microsoft.com/library/bg182625(v=vs.85).aspx#legacyAPIs says that document.all was dropped in IE11? I do not have a virtual machine handy to verify.
Either way, I think Edge matters a lot more than IE11 at this point.

I was able to remove document.all.tags from Blink 3 years ago. There were no complaints about breaking after removing it. So even if you believe UseCounter data is not necessarily indicative of enterprise use, I believe the fact that there were no complaints after removing it is good indication that the risk should be low and that it is at least worth trying.

Note that when showModalDialog was dropped in Blink for example, there *were* a lot of complaints from enterprise users.
Comment 8 Daniel Bates 2017-03-08 10:21:08 PST
(In reply to comment #7)
> (In reply to comment #5)
> > document.all.tags() is a proprietary Microsoft extension and is supported in
> > IE 11.
> 
> https://msdn.microsoft.com/library/bg182625(v=vs.85).aspx#legacyAPIs says
> that document.all was dropped in IE11? I do not have a virtual machine handy
> to verify.

It was not removed at least in IE 11.0.9600.18500. It would be good to know what version of IE11 removes it?

> Either way, I think Edge matters a lot more than IE11 at this point.
> 
> I was able to remove document.all.tags from Blink 3 years ago. There were no
> complaints about breaking after removing it. 

That is good to know though it assumes that such breakage would be reported. I suspect that usage of document.all.tags() would be on unmaintained legacy web pages, including cached pages (say, archive.org).

> So even if you believe
> UseCounter data is not necessarily indicative of enterprise use, I believe
> the fact that there were no complaints after removing it is good indication
> that the risk should be low and that it is at least worth trying.
> 
> Note that when showModalDialog was dropped in Blink for example, there
> *were* a lot of complaints from enterprise users.

And ...

What is the benefit of removing document.all.tags()? Do we know of any modern web pages (in the last 3 years) whose behavior changes by removing document.all.tags()
Comment 9 Daniel Bates 2017-03-08 10:26:05 PST
(In reply to comment #8)
> What is the benefit of removing document.all.tags()?

Obviously it benefits us (the OpenSource WebKit project) as it removes 11 lines of code (includes whitespace). Does it allow us to remove more code?
Comment 10 Chris Dumez 2017-03-08 10:27:04 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > document.all.tags() is a proprietary Microsoft extension and is supported in
> > > IE 11.
> > 
> > https://msdn.microsoft.com/library/bg182625(v=vs.85).aspx#legacyAPIs says
> > that document.all was dropped in IE11? I do not have a virtual machine handy
> > to verify.
> 
> It was not removed at least in IE 11.0.9600.18500. It would be good to know
> what version of IE11 removes it?
> 
> > Either way, I think Edge matters a lot more than IE11 at this point.
> > 
> > I was able to remove document.all.tags from Blink 3 years ago. There were no
> > complaints about breaking after removing it. 
> 
> That is good to know though it assumes that such breakage would be reported.
> I suspect that usage of document.all.tags() would be on unmaintained legacy
> web pages, including cached pages (say, archive.org).
> 
> > So even if you believe
> > UseCounter data is not necessarily indicative of enterprise use, I believe
> > the fact that there were no complaints after removing it is good indication
> > that the risk should be low and that it is at least worth trying.
> > 
> > Note that when showModalDialog was dropped in Blink for example, there
> > *were* a lot of complaints from enterprise users.
> 
> And ...
> 
> What is the benefit of removing document.all.tags()? Do we know of any
> modern web pages (in the last 3 years) whose behavior changes by removing
> document.all.tags()

The benefit is to align with other browsers and the specification (and drop some code in the process, although not a lot). Other browsers are aligning with standards and we should do the same. We do not want new content developed in WebKit to not work in other browsers because it relies on document.all.tags.

Note that document.all is already deprecated and we already have mitigations in place to make it less likely do be used (e.g. document.all == undefined) in the hope of removing it completely some day. This is a step in this direction.
Comment 11 Daniel Bates 2017-03-08 10:39:57 PST
I know this patch was r+'ed. I am typing some remarks and questions that I would prefer to be answered before this patch is landed.
Comment 12 Daniel Bates 2017-03-08 11:11:22 PST
(In reply to comment #10)

If this extension is removed from the latest version of IE11 then it seems reasonable to remove it from WebKit. Otherwise, is there harm of keeping this extension? I mean, it does not seem very intrusive to the codebase as it is effectively an alias for document.getElementsByTagName() (though maybe there is a cost to keep some form of document.all working?)

As far as I am aware document.all.tags() is a proprietary Microsoft extension and hence never standardized. I take it there is little value in concerning ourself with supporting unmaintained legacy web pages or content archived in archive.org? 

> [...]
> The benefit is to align with other browsers and the specification (and drop
> some code in the process, although not a lot). Other browsers are aligning
> with standards and we should do the same. 
> We do not want new content
> developed in WebKit to not work in other browsers because it relies on
> document.all.tags.

Are we actually worried about this? I mean, document.all.tags() in not listed in the standard.
Comment 13 Chris Dumez 2017-03-08 12:57:54 PST
(In reply to comment #12)
> (In reply to comment #10)
> 
> If this extension is removed from the latest version of IE11 then it seems
> reasonable to remove it from WebKit. Otherwise, is there harm of keeping
> this extension? I mean, it does not seem very intrusive to the codebase as
> it is effectively an alias for document.getElementsByTagName() (though maybe
> there is a cost to keep some form of document.all working?)
> 
> As far as I am aware document.all.tags() is a proprietary Microsoft
> extension and hence never standardized. I take it there is little value in
> concerning ourself with supporting unmaintained legacy web pages or content
> archived in archive.org? 

This proprietary Microsoft extension is definitely gone in Edge, I have just verified this in a VM. I do not see much point in keeping a Microsoft extension that even Microsoft has dropped.

> 
> > [...]
> > The benefit is to align with other browsers and the specification (and drop
> > some code in the process, although not a lot). Other browsers are aligning
> > with standards and we should do the same. 
> > We do not want new content
> > developed in WebKit to not work in other browsers because it relies on
> > document.all.tags.
> 
> Are we actually worried about this? I mean, document.all.tags() in not
> listed in the standard.

As long as an API is exposed in our engine, there are chances people will use them.
And we do care about aligning with other browsers and the standards. I care about this a lot more than trying to maintain an old IE extension that even Microsoft dropped in their latest product.
Comment 14 Chris Dumez 2017-03-08 14:27:26 PST
Given the results on Edge, can I now land this patch?
Comment 15 Sam Weinig 2017-03-08 16:27:40 PST
(In reply to comment #14)
> Given the results on Edge, can I now land this patch?

I double r+.
Comment 16 Daniel Bates 2017-03-08 18:16:46 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > 
> > If this extension is removed from the latest version of IE11 then it seems
> > reasonable to remove it from WebKit. Otherwise, is there harm of keeping
> > this extension? I mean, it does not seem very intrusive to the codebase as
> > it is effectively an alias for document.getElementsByTagName() (though maybe
> > there is a cost to keep some form of document.all working?)
> > 
> > As far as I am aware document.all.tags() is a proprietary Microsoft
> > extension and hence never standardized. I take it there is little value in
> > concerning ourself with supporting unmaintained legacy web pages or content
> > archived in archive.org? 
> 
> This proprietary Microsoft extension is definitely gone in Edge, I have just
> verified this in a VM. I do not see much point in keeping a Microsoft
> extension that even Microsoft has dropped.
> 

I would expect this to be removed from Edge. From my understanding IE11 is still supported and shipped by Microsoft for backwards compatibility and Edge is their new browser that intentionally breaks with backwards compatibility to be more conformant with other browsers and standards.

> > 
> > > [...]
> > > The benefit is to align with other browsers and the specification (and drop
> > > some code in the process, although not a lot). Other browsers are aligning
> > > with standards and we should do the same. 
> > > We do not want new content
> > > developed in WebKit to not work in other browsers because it relies on
> > > document.all.tags.
> > 
> > Are we actually worried about this? I mean, document.all.tags() in not
> > listed in the standard.
> 
> As long as an API is exposed in our engine, there are chances people will
> use them.

We could log a deprecation warning or restrict usage of it to quirks mode.

> And we do care about aligning with other browsers and the standards. I care
> about this a lot more than trying to maintain an old IE extension that even
> Microsoft dropped in their latest product.

I think the focus should be on improving the user experience and avoiding breakage of saved web archives of old pages or unmaintained legacy web pages. If an improvement to the user experience can be accomplished by aligning with other browsers and standards that is a great. If not, we should either work with the standards body or weigh the pros/cons of a change with the impact to the user experience. From this discussion it seems we are more concerned with matching the standard. Maybe this is acceptable?
Comment 17 Daniel Bates 2017-03-08 18:17:04 PST
(In reply to comment #14)
> Given the results on Edge, can I now land this patch?

OK
Comment 18 Chris Dumez 2017-03-08 18:37:47 PST
Comment on attachment 303778 [details]
Patch

Clearing flags on attachment: 303778

Committed r213619: <http://trac.webkit.org/changeset/213619>
Comment 19 Chris Dumez 2017-03-08 18:37:56 PST
All reviewed patches have been landed.  Closing bug.