Closed Bug 984778 Opened 10 years ago Closed 8 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: annevk, Assigned: ayg)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 1 obsolete file)

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.
(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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Reopening as the usage count is actually really low in Chromium: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25428#c1
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Status: REOPENED → NEW
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)
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
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #8785710 - Flags: review?(bzbarsky)
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
Summary: Castrate hasFeature() → Make hasFeature() always return true (even for SVG)
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.
Attachment #8785710 - Flags: review?(bzbarsky)
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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?
Flags: needinfo?(dholbert)
Robert Longson is likely more in-the-know about this than I am; punting needinfo to him.
Flags: needinfo?(dholbert) → needinfo?(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.
Flags: needinfo?(longsonr)
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?
Flags: needinfo?(longsonr)
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...
Aha, I didn't realize it was including something.  Thanks!
Flags: needinfo?(longsonr)
Requesting review from longsonr for the SVG bits and bz for the rest.
Attachment #8785710 - Attachment is obsolete: true
Attachment #8786846 - Flags: review?(longsonr)
Attachment #8786846 - Flags: review?(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.
Attachment #8786846 - Flags: review?(bzbarsky) → review+
Attachment #8786846 - Flags: review?(longsonr) → review+
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3385e09f0c0
Make hasFeature() and SVG requiredFeatures always return true; r=bz,longsonr
https://hg.mozilla.org/mozilla-central/rev/a3385e09f0c0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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]
Flags: needinfo?(ehsan)
This bug is about DOMImplementation.hasFeature, not Navigator.hasFeature.  So totally unrelated to bug 1299493.
Flags: needinfo?(ehsan)
Oh thanks, I missed the different interface. Thanks
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.