370210 - SVG onload attribute broken since incremental XML landing

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[tor]

Attachment: shuffle code in nsXMLContentSink::HandleEndElement

SVG "onload" handling is a hack at this point, currently fired from the nsXMLContentSink during document loading (specifically, at HandleEndElement).  While changing the behavior to fire these at a more reasonable time, like when the frames are ready, is on the long term development list, we'd like to keep this hack limping along.

The incremental XML landing added some code to HandleEndElement, part of which was added after the #ifdef MOZ_SVG.  It seems as though lines need to happen before the onload event is fired, or things don't work properly.

This testcase from bug 282579 demonstrates the problem - should show some text on a curve:

  https://bugzilla.mozilla.org/attachment.cgi?id=194178

Comment 1

[Henri Sivonen (:hsivonen)]

Comment on attachment 254862 [details] [diff] [review]
shuffle code in nsXMLContentSink::HandleEndElement

Looks good to me, but I should point out that I am not a qualified reviewer.

Comment 2

[tor]

Comment on attachment 254862 [details] [diff] [review]
shuffle code in nsXMLContentSink::HandleEndElement

Henri, I think making XML incremental gives you pretty good review qualifications.

Comment 3

[tor]

Testcases in bug 371115.

Comment 5

[jonathan chetwynd]

#4 Tim,

bug 371115 is a regression, as it is a dupe of this one, 
howcome this isn't a regression? in plain english if possible ~:"

whether this bug is a regression or not, will there be a partial fix to cover the regression aspect before it gets to the top of the "long term development list"?

if not why not?

regressions are pretty serious for web developers, as for instance my homepage doesn't currently display anything, and it is almost impossible to 'improve' the situation, not knowing in which order bugs are going to be resolved.

more detailed advice would be helpful

Comment 6

[tor]

Yes, it is a regression as noted in comment 0.  The patch on this bug, from February 12th and currently waiting for review, should restore the prior behavior.

Comment 7

[jonathan chetwynd]

tor,

I just can't read the word regression in comment 0.
I've now read it six times and done a word search....

once again my apologies if I've overstepped the mark.
can't make an omlette etc...

Comment 8

[jonathan chetwynd]

oops aqnd forgot to add thanks for the timeline.

I'm beginning to panic as this has been hanging around for a while.
One talk had to be abandoned, and twitchy over another due soon...

tx ~:"

Comment 9

[Jesse Ruderman]

Comment 0 doesn't use the word "regression", but both comment 0 and the summary say that this broke with the "incremental XML landing" (meaning this is a regression from bug 18333).

Comment 10

[jonathan chetwynd]

#9 Jesse, 

thanks for the explanation in plainer English, grateful I didn't misinterpret previous comments.

Comment 11

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

Comment on attachment 254862 [details] [diff] [review]
shuffle code in nsXMLContentSink::HandleEndElement

Hmm.. this code is really scary. We shouldn't be firing events if we haven't flushed all notifications. 

You should really add a call to FlushTags() inside that |if|.

Unrelated to this, but please file a bug on it or add it to the current patch.

With that sr=me

Comment 12

[tor]

Attachment: version for checkin - add FlushTags()

Yes, we're painfully aware that this isn't the right place to be generating this event and are interested in moving it elsewhere for a number of reasons.

Comment 13

[tor]

Checked in.