SVG conditional processing is only implemented for <switch>

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: dbaron, Assigned: scootermorris)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

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.
(Assignee)

Comment 2

13 years ago
Created attachment 200765 [details] [diff] [review]
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.
Assignee: general → scootermorris
Status: NEW → ASSIGNED
Attachment #200765 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #200765 - Flags: review? → review?(tor)

Comment 3

13 years ago
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?
(Assignee)

Comment 4

13 years ago
Created attachment 203812 [details] [diff] [review]
Updated to avoid if cascade
Attachment #200765 - Attachment is obsolete: true
Attachment #203812 - Flags: review?(tor)
Attachment #200765 - Flags: review?(tor)
(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?)
(Assignee)

Comment 6

13 years ago
(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

13 years ago
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?
(Assignee)

Comment 8

13 years ago
(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

13 years ago
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.
Attachment #203812 - Flags: review?(tor) → review+
(Assignee)

Comment 10

13 years ago
Created attachment 208763 [details] [diff] [review]
Updated patch with tests combined
Attachment #203812 - Attachment is obsolete: true
Attachment #208763 - Flags: superreview?
(Assignee)

Updated

13 years ago
Attachment #208763 - Flags: superreview? → superreview?(dbaron)
(Assignee)

Comment 11

13 years ago
Created attachment 209168 [details] [diff] [review]
Patch reorganized after sicking's 'atomic' landing
Attachment #208763 - Attachment is obsolete: true
Attachment #209168 - Flags: review?(tor)
Attachment #208763 - Flags: superreview?(dbaron)

Comment 12

13 years ago
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.
Attachment #209168 - Flags: review?(tor)
(Assignee)

Comment 13

13 years ago
Created attachment 209891 [details] [diff] [review]
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.
Attachment #209168 - Attachment is obsolete: true
Attachment #209891 - Flags: review?(tor)

Comment 14

13 years ago
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.
Attachment #209891 - Flags: review?(tor) → review+
(Assignee)

Comment 15

13 years ago
Created attachment 209917 [details] [diff] [review]
Updated to reflect tor's comments
Attachment #209891 - Attachment is obsolete: true
Attachment #209917 - Flags: superreview?
(Assignee)

Updated

13 years ago
Attachment #209917 - Flags: superreview? → superreview?(bugmail)
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)
Attachment #209917 - Flags: superreview?(bugmail) → review+
(Assignee)

Comment 17

13 years ago
Created attachment 209929 [details] [diff] [review]
Updated to reflect sicking's comments
Attachment #209917 - Attachment is obsolete: true
Attachment #209929 - Flags: superreview?
(Assignee)

Updated

13 years ago
Attachment #209929 - Flags: superreview? → superreview?(tor)

Updated

13 years ago
Attachment #209929 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 18

13 years ago
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
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

13 years ago
Reopening -- busted camino
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

13 years ago
Created attachment 210262 [details] [diff] [review]
Patch to fix camino bustage
Attachment #210262 - Flags: review?(tor)
(Assignee)

Comment 21

13 years ago
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

Updated

13 years ago
Attachment #210262 - Flags: review?(tor) → review+
(Assignee)

Updated

13 years ago
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.