Closed Bug 370210 Opened 15 years ago Closed 15 years ago

SVG onload attribute broken since incremental XML landing

Categories

(Core :: XML, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tor, Assigned: tor)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

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
Attachment #254862 - Flags: review?(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.
Attachment #254862 - Flags: review?(hsivonen) → review+
Comment on attachment 254862 [details] [diff] [review]
shuffle code in nsXMLContentSink::HandleEndElement

Henri, I think making XML incremental gives you pretty good review qualifications.
Attachment #254862 - Flags: superreview?(jonas)
Blocks: 369402
Blocks: 371115
Testcases in bug 371115.
Duplicate of this bug: 371115
#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
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.
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...
Keywords: regression, testcase
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 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).
#9 Jesse, 

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

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
Attachment #254862 - Flags: superreview?(jonas) → superreview+
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.
Checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.