303018 - SVG conditional processing is only implemented for <switch>

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For some reason, the SVG conditional processing attributes have only been
implemented for the <svg:switch> element.  (I remember this when I reviewed that
patch, but I hadn't read very much of the spec at that time.)  These attributes,
such as requiredFeatures and requiredExtensions, should be implemented equally
for all elements:  switch then only behaves differently in that it uses the
first of its relevant (probably the wrong term) children rather than all of them.

Steps to reproduce:
  load http://dbaron.org/tests/svg/SVG11/t050801-conditional-01-b

Should be green; is red.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(See also other tests in that directory.)

Comment 2

[Scooter Morris]

Attachment: Patch to allow conditional on non-switch elements

This patch supports conditionals on elements outside of a <switch>.  It still doesn't fix the problem of referencing an element from a <use> when that element has a failed conditional.  According to the spec the element should still be available to a use, even though the conditional fails and it is not rendered.

Comment 3

[tor]

Comment on attachment 200765 [details] [diff] [review]
Patch to allow conditional on non-switch elements

>+  // See if this element supports conditionals & if it does,
>+  // handle it
>+  if (NS_SVG_LangSupported(aTag) || NS_SVG_TestsSupported(aTag)) {

How much does going through two massive "if" chains for SVG frame creation hurt us?  Would it be better to hashtable to store the supports bits?

Comment 4

[Scooter Morris]

Attachment: Updated to avoid if cascade

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> It still
> doesn't fix the problem of referencing an element from a <use> when that
> element has a failed conditional.  According to the spec the element should
> still be available to a use, even though the conditional fails and it is not
> rendered.

There have been recent clarifications to SVG12Mobile on this topic, but I think the case you're talking about being buggy is where an *ancestor* has a failed conditional -- we should then display it.  (Would the solution to that in the current architecture be to check conditionals at a time other than frame construction, e.g., paint?)

Comment 6

[Scooter Morris]

(In reply to comment #5)

> There have been recent clarifications to SVG12Mobile on this topic, but I think
> the case you're talking about being buggy is where an *ancestor* has a failed
> conditional -- we should then display it.  (Would the solution to that in the
> current architecture be to check conditionals at a time other than frame
> construction, e.g., paint?)
> 

Perhaps, but I believe that there are plans afoot to rethink SVG frames completely and move much (some? all?) of the SVG rendering logic to content.  When this happens, I suspect that handling cases such as this would become much easier (at least if we think about it in advance).  So, my inclination would be to open a separate bug for handling of <use> targets when they are not displayed for some reason (either due to a failed conditional or a display="none").

Comment 7

[tor]

Comment on attachment 203812 [details] [diff] [review]
Updated to avoid if cascade

>+    if (!hasSystemLanguage)
>+      return NS_OK;
>+
>+    if (!hasRequiredExtentions || !hasRequiredFeatures)
>+      return NS_OK;

Any reason in particular that these two tests are split?

Comment 8

[Scooter Morris]

(In reply to comment #7)
> (From update of attachment 203812 [details] [diff] [review] [edit])
> >+    if (!hasSystemLanguage)
> >+      return NS_OK;
> >+
> >+    if (!hasRequiredExtentions || !hasRequiredFeatures)
> >+      return NS_OK;
> 
> Any reason in particular that these two tests are split?
> 

Nope, probably just implementation history.

Comment 9

[tor]

Comment on attachment 203812 [details] [diff] [review]
Updated to avoid if cascade

r=tor with tests combined.  dbaron or bz should probably be the sr.

Comment 10

[Scooter Morris]

Attachment: Updated patch with tests combined

Comment 11

[Scooter Morris]

Attachment: Patch reorganized after sicking's 'atomic' landing

Comment 12

[tor]

Comment on attachment 209168 [details] [diff] [review]
Patch reorganized after sicking's 'atomic' landing

Is the nsSVGElementList.h supposed to be listing all SVG elements?  It's missing some stuff we implement (mask) but also contains stuff we don't do yet (full filter set).  If intended to be a complete list regardless of implementation status, the ifdef for foreignobject could be removed.

Comment 13

[Scooter Morris]

Attachment: Patch now includes *all* elements

OK, the patch now covers all SVG elements.  It should be noted that there is some strangeness in the spec.  The conditional elements are specifically called out for in <mask> and gradients, but the text says that they have no impact.

Comment 14

[tor]

Comment on attachment 209891 [details] [diff] [review]
Patch now includes *all* elements

definition-src atom name should be definition_src.

Remove unused string arg to SVG_ELEMENT.

Update copyright date on  nsSVGElementList.h.

With those changes, r=tor.

Comment 15

[Scooter Morris]

Attachment: Updated to reflect tor's comments

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 209917 [details] [diff] [review]
Updated to reflect tor's comments

>Index: content/svg/content/src/nsSVGFeatures.cpp
..
>+// Test to see if this element supports a specific conditional
>+static PRBool
>+NS_SVG_Conditional(const nsIAtom *atom, PRUint16 cond) {
>+
>+#define SVG_ELEMENT(_atom, _supports) if (atom == nsSVGAtoms::_atom && _supports&cond) return PR_TRUE;
>+#include "nsSVGElementList.h"
>+#undef SVG_ELEMENT
>+  return PR_FALSE;
>+}


What you probably want here is
#define SVG_ELEMENT(_atom, _supports) if (atom == nsSVGAtoms::_atom) return (_supports & cond) != 0;

You could hash this stuff too if it's perf critical. But that might be premature without any data to support it.

>+PRBool
>+NS_SVG_ExternalSupported(const nsIAtom *atom) {
>+  return NS_SVG_Conditional(atom, SUPPORTS_EXTERNAL);
>+}

This function isn't used.

r=me with that addressed (explained or fixed)

Comment 17

[Scooter Morris]

Attachment: Updated to reflect sicking's comments

Comment 18

[Scooter Morris]

Checked in on trunk:
Checking in content/base/src/nsGkAtomList.h;
/cvsroot/mozilla/content/base/src/nsGkAtomList.h,v  <--  nsGkAtomList.h
new revision: 3.8; previous revision: 3.7
done
RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGElementList.h,v
done
Checking in content/svg/content/src/nsSVGElementList.h;
/cvsroot/mozilla/content/svg/content/src/nsSVGElementList.h,v  <--  nsSVGElementList.h
initial revision: 1.1
done
Checking in content/svg/content/src/nsSVGFeatures.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGFeatures.cpp,v  <--  nsSVGFeatures.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1190; previous revision: 1.1189
done

Comment 19

[Scooter Morris]

Reopening -- busted camino

Comment 20

[Scooter Morris]

Attachment: Patch to fix camino bustage

Comment 21

[Scooter Morris]

Fix checked in:
Checking in content/svg/content/src/nsSVGElementList.h;
/cvsroot/mozilla/content/svg/content/src/nsSVGElementList.h,v  <--  nsSVGElementList.h
new revision: 1.2; previous revision: 1.1
done
Checking in content/base/src/nsGkAtomList.h;
/cvsroot/mozilla/content/base/src/nsGkAtomList.h,v  <--  nsGkAtomList.h
new revision: 3.9; previous revision: 3.8
done

Comment 22

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 326495.  I'm not quite sure what made people think just returning NS_OK is fine when what you want it to prevent frame construction.