288438 - Turn off <foreignObject> for 1.8

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Comment 1

[Anne (:annevk)]

Would it be possible to provide some more detailed information in this bug?
Also, wasn't the freeze for 1.8 planned today?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Currently we just crash hard when encountering any use of <foreignobject>. See
for example: http://www.croczilla.com/svg/samples/foreign1/foreign1.xml

I think we ought to resolve this bug :-)

Comment 3

[tor]

The crash cause is fairly simple - bug 291339.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I still strongly believe we need to do this, or advanced effects in
<foreignobject> will be significanly harder for authors to use when 1.9 becomes
available. Not to mention all the bugs that will be filed on <foreignobject> by
1.8 users. Given that it will be unreliable in unpredictable ways, it's just
going to give us a bad name.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's the status here?

Comment 6

[Jonathan Watt [:jwatt]]

I'm still waiting for you and Tim (and possibly other drivers?) to agree whether
were doing this or not. Sorry, I thought that was clear from our earlier emails.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We got feedback from Alex that foreignObject is pretty useless as is. Didn't
that clinch it?

Comment 8

[tor]

No, his feedback was that the current foreignObject wasn't appropriate for his
application.  I think that properly release-noted, that authors might find uses
for even this limited implimentation that we haven't thought of.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So what about the scenario where someone tries to do something nontrivial in 1.9
and it just borks 1.8 users? And what about the bugs in our 1.8 implementation
--- are we going to let users find them on their own, are they going to flood
bugzilla, or are we going to write detailed release notes describing what they
should and shouldn't try?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do you actually have release-note text in mind?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And keep in mind that most people don't read the release notes.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That was rather cranky. Sorry. But I'm very concerned about this.

Comment 13

[tor]

Considering that Mozilla supports a nonstandard subset of SVG, to write
nontrivial content authors will need to consult the release notes or other
documentation of the current limitations.  The release notes will probably be a
modified/extended version of:  http://www.mozilla.org/projects/svg/status.html.

Text for foreignObject might read something like this:  "The current
<foreignObject> implementation is a restrictive subset.  General tranformation
of the content is not supported, only translations.  Modification of the content
after document creation may not be visible."

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't forget "some elements will not respect z-ordering or clipping".

What about my first comment in comment #9? (See
http://weblogs.mozillazine.org/roc/archives/2005/05/xtech.html.)

Comment 15

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

I was actually thinking of filing this bug independently before roc pointed this
one out to me.

(In reply to comment #13)
> Considering that Mozilla supports a nonstandard subset of SVG, to write
> nontrivial content authors will need to consult the release notes or other
> documentation of the current limitations.

The idea of a Web browser is really that authors shouldn't be writing for a
particular browser; they should be writing to the Web.  Authors two years in the
future probably won't want to care much about Mozilla 1.8's limitations, but
they might be forced to care if things don't degrade gracefully; we should
ensure that they do.

Comment 16

[Hixie (not reading bugmail)]

The main issue isn't about today's content authors targetting Firefox 1.1. It's
about tomorrow's content authors targetting Firefox 1.5 when many users are
still using Firefox 1.1 and the authors being worried about those users having a
negative play experience.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: patch to disable <foreignObject>

Comment 18

[tor]

Attachment: add configure option

Comment 19

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

We should NOT have a configure option for this.  We should be deciding which way
is right, not leaving it up to each distributor.

Comment 20

[tor]

This is intended for developer's convenience, not distributors.

Comment 21

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

That doesn't matter; if it's there, distributors will think it's for them to use.

Comment 22

[Benjamin Smedberg]

Comment on attachment 187151 [details] [diff] [review]
add configure option

Did you ever sort out your oddity with --disable-svg conflicting with
--disable-svg-foreignobject in some way?

Comment 23

[tor]

I think the mozconfig merge problem is coming from the ac_add_options check to
see if the option already exists:

    # Avoid adding duplicates
    case "$ac_options" in
      *"$_opt"* ) ;;
      * ) mozconfig_ac_options="$mozconfig_ac_options $_opt" ;;
    esac

Maybe *"$opt"^[[:alnum:][:punct:]] would work?

Comment 24

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

In addition to my previous objection, this patch also messes up binary
compatibility of nsDOMClassInfo for when foreignObject is turned back on. 
(While fixing that, it would be good to fix the comments, now that SVG is on by
default, to indicate that new classes should be added at the end.)

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 187151 [details] [diff] [review]
add configure option

minusing based on dbaron's comments. I suggest you just comment out the
configure.in option so that it can only be turned on by modifying configure.in

Comment 26

[tor]

I'm busy with other things at the moment, reassigning to nobody.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Carrying forward a+,r+. This version address dbaron's issues, I hope.

Comment 28

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

Comment on attachment 187483 [details] [diff] [review]
updated patch 

There are a bunch of different things in the classinfo that have to be in the
same order, otherwise it all breaks.  This mismatches the orders.  And the
comment I was talking about was the one in nsIDOMClassInfo.h.

Also, the commented-out bit of configure.in should probably have an explanation
why it's commented.

The mozconfig2configure doesn't look like it's supposed to be part of this
patch.	If so, why?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #28)
> (From update of attachment 187483 [details] [diff] [review] [edit])
> There are a bunch of different things in the classinfo that have to be in the
> same order, otherwise it all breaks.  This mismatches the orders.

I'm not 100% sure what the constraints are, but at least the enums in
nsIDOMClassInfo are now in the same order as the definitions in nsDOMClassInfo.cpp.

> And the comment I was talking about was the one in nsIDOMClassInfo.h.

OK, I changed that.

> Also, the commented-out bit of configure.in should probably have an
> explanation why it's commented.

OK

> The mozconfig2configure doesn't look like it's supposed to be part of this
> patch.	If so, why?

It is part of this patch. See comments #22 and #23. The problem is that
--disable-svg is a substring of --disable-svg-foreignobject.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch #2

Comment 31

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

Comment on attachment 187487 [details] [diff] [review]
updated patch #2

How about keeping nsDOMClassInfo::Init in order for sanity's sake, and
sr=dbaron.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 187487 [details] [diff] [review]
updated patch #2

carrying forward approval and r+

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 34

[Jonathan Watt [:jwatt]]

in-testsuite-