984778 - Make hasFeature() always return true (even for SVG)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anne (:annevk)]

Follow up to bug 801425; based on research by the Google/Opera guys we can always return true for hasFeature(), even for SVG. To make the implementation even simpler we can remove the arguments from IDL too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25428 suggests that the Google research was wrong.

Comment 2

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #1)
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=25428 suggests that the
> Google research was wrong.

I've started to be very cautious when drawing conclusions from the Chrome use counters, and it seems like the Blink community is converging around the same idea.

Comment 3

[Anne (:annevk)]

Reopening as the usage count is actually really low in Chromium: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25428#c1

Comment 4

[Arkadiusz Michalski (Spirit)]

Starting from Chrome 44 this method always return true and it seems that there are no any negative reports (per https://www.w3.org/Bugs/Public/show_bug.cgi?id=25428#c5)

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 0001-Bug-984778-Make-hasFeature-always-return-true-r-bz.patch

Looks like Chrome has done this since 2014.  Any reason for us not to?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d415e9b45cf9&exclusion_profile=false

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8785710 [details] [diff] [review]
0001-Bug-984778-Make-hasFeature-always-return-true-r-bz.patch

> nsIAtom** SVGTests::sStringListNames[3] =
> {
>-  &nsGkAtoms::requiredFeatures,
>   &nsGkAtoms::requiredExtensions,
>   &nsGkAtoms::systemLanguage,
> };

So the "3" here needs to be deleted, or else it crashes (sigh, C++).  New try run with that one character deleted: https://treeherder.mozilla.org/#/jobs?repo=try&revision=707306be74d3

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8785710 [details] [diff] [review]
0001-Bug-984778-Make-hasFeature-always-return-true-r-bz.patch

Canceling review request until I sort out try failures.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So I just found out about SVG's "requiredFeatures" attribute.  I don't think it makes sense to return true from hasFeature() for everything if we still support requiredFeatures, because I don't see any other way for script to detect what requiredFeatures will return.  It seems Chrome dropped support for requiredFeatures too:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4422

Do we actually want to do this?  Is SVG expected to ever add new features such that requiredFeatures would be useful in the future?  Offhand, it's not obvious to me how to implement graceful fallback otherwise without script, and SVG without script seems like an important use-case (no script is allowed in <img>, right?).  Do we care what the SVG WG thinks about dropping requiredFeatures, and if so, what in fact do they think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Probably a good idea to ask the SVG module owner.  He's not accepting needinfo requests until October apparently, so maybe try the module peers (but not roc) or asking on .platform?  Especially for the SVGWG interactions.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Currently our implementation of document.implementation.hasFeature() returns true for everything that doesn't look like SVG.  The DOM spec says to return true for everything without exception, which Chrome has done now for some time.  This is on the theory that hasFeature is a bad approach to feature-testing, and authors should directly test (using JavaScript etc.) whether the feature they want is present.

If we were to match the spec, it seems that we would want to get rid of requiredFeatures too, because it doesn't make sense to keep the declarative feature-testing for SVG while giving no way for script to test it.  Chrome has done this, and treats all features as supported <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4422>.  So do we want to drop requiredFeatures?  Or should we ask DOM to re-add hasFeature() for SVG?

Some questions to ask about dropping requiredFeatures:

1) Is requiredFeatures useful in SVG?

1a) Are there UAs that support only a subset of features such that declarative testing is useful for graceful fallback?

1b) If not, is it likely that new features would be added to SVG in the future so that requiredFeatures would become useful?

1c) If so, is this true even given that UAs may incorrectly advertise support for a feature that they really don't support correctly, such that requiredFeatures isn't reliable?

2) Do we care what the SVG WG thinks about this?  If so, is it likely they'd agree to this change?

Comment 11

[Daniel Holbert [:dholbert]]

Robert Longson is likely more in-the-know about this than I am; punting needinfo to him.

Comment 12

[Robert Longson [:longsonr]]

1) probably not

1a) I don't know

1b) definitely not

1c) requiredFeatures is not very granular. We've been conservative and not advertised anything we don't completely support. Not sure about other UAs.

2) The SVG WG would like requiredFeatures to disappear. It's been removed from the SVG 2 specification. See bug 1295404 and bug 1295404.

Returning true always as this bug proposes seems a good first step especially if Chrome does it. I'd be happy to review such a patch.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I have a patch that has both hasFeature and requiredFeatures always return true, but I get these two reftest failures that don't make any sense to me:

https://treeherder.mozilla.org/logviewer.html#?job_id=26670239&repo=try#L22846

Do you know what's going wrong?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Those tests load an SVG file via <svg:image> or via a fill url() value.  In those contexts, script is not supported in SVG.

The file being loaded does:

  <rect width="100%" height="100%" fill="lime"/>
  <rect requiredFeatures="http://www.w3.org/TR/SVG11/feature#Script" width="100%" height="100%" fill="blue"/>

and expects the lime rect to be visible and the blue one not to be visible.  See the nsSVGFeatures::HasFeature code that handles that feature string which your patch removes...

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Aha, I didn't realize it was including something.  Thanks!

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 0001-Bug-984778-Make-hasFeature-and-SVG-requiredFeatures-.patch

Requesting review from longsonr for the SVG bits and bz for the rest.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786846 [details] [diff] [review]
0001-Bug-984778-Make-hasFeature-and-SVG-requiredFeatures-.patch

r=me on the IDL bit; I trust Robert to review everything else here, and haven't looked at any of the rest of the patch.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=630b24bdb318

Failures should all be fixed, but I did a new reftest-only try run to be sure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=696a56d69153

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3385e09f0c0
Make hasFeature() and SVG requiredFeatures always return true; r=bz,longsonr

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3385e09f0c0

Comment 21

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/hasfeature-will-always-return-true-even-for-svg/

Comment 22

[Jean-Yves Perrier [:teoli]]

Ehsan, I am correct in understanding that this feature has been completely removed in bug 1299493 (same release, Firefox 51)? [And therefore this bug doesn't need docs]

Comment 23

[Boris Zbarsky [:bzbarsky]]

This bug is about DOMImplementation.hasFeature, not Navigator.hasFeature.  So totally unrelated to bug 1299493.

Comment 24

[Jean-Yves Perrier [:teoli]]

Oh thanks, I missed the different interface. Thanks

Comment 25

[Jean-Yves Perrier [:teoli]]

Documentation updated:
https://developer.mozilla.org/en-US/Firefox/Releases/51#DOM_HTML_DOM
and
https://developer.mozilla.org/en-US/docs/Web/API/DOMImplementation/hasFeature