Closed Bug 380516 Opened 14 years ago Closed 14 years ago

[FIX]SVG image not rendered - regression between 3.0a4pre and 3.0a5pre

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha6

People

(Reporter: duncan.loveday, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(8 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070512 Minefield/3.0a5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070512 Minefield/3.0a5pre

In the attached test case, no image appears with the latest trunk builds. Sometimes the image appears for a moment and then disappears.

Reproducible: Always

Steps to Reproduce:
1. Open the attached test.html
2.
3.
Actual Results:  
No image appears with the latest trunk builds. Sometimes the image appears for a moment and then disappears.

Expected Results:  
The image, consisting of a rectangle and some text, should be displayed.

Works correctly on FF2 and FF3 3.0a4pre.

The conditions required are that
(i) The SVG is populated from the HTML load handler (OK if loaded from the SVG load handler)
(ii) The SVG contains text.
(iii) The attribute "font-family" is set on a <g> element after adding the <g> to the root element (OK if don't set "font-family" or set it before adding <g> to root element)
Attached image test SVG source
Attached file test HTML source
This might possibly be related to another bug of mine, bug 374293, although that bug didn't regress at the same time. It is to do with changing font attributes causing the image to disappear though.
Keywords: regression
I can see the image after resizing the window. And also if I have the pref browser.display.use_document_fonts set to 0.
I found this regression range: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1178037420&maxdate=1178040899

Blocks: 378975
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duncan: would you be able to take the time to create a version of the test that works correctly (exactly the same rendering as you'd expect from the buggy testcase), without any of the complicating factors that make it fail?  It'll make it easier to keep this from regressing in the future.
This version works OK because font-family is set before adding the element to the document.
Attached file test HTML source - works OK 2 of 3 (obsolete) —
This version works OK because there is no text in the SVG
Attached file test HTML source - works OK 3 of 3 (obsolete) —
This version works OK because the SVG is populated from the SVG load handler rather than from the HTML load handler.
This version appears to work correctly simply by virtue of having an alert message in it. Maybe there is a timing dependency ?

Something I haven't been able to explain yet is that not all of my SVG images are affected by this bug. I have some SVG images that do the same thing, setting setting font-family on a <g> etc and they carried on working with 3.0a5pre whilst others broke.
Re-attaching version that works libe
Attachment #264646 - Attachment is obsolete: true
Finger trouble - original had both workarounds 1 and 3
Attachment #264645 - Attachment is obsolete: true
Finger trouble - original had both workarounds 1 and 2
Attachment #264644 - Attachment is obsolete: true
(In reply to comment #5)
> Duncan: would you be able to take the time to create a version of the test that
> works correctly (exactly the same rendering as you'd expect from the buggy
> testcase), without any of the complicating factors that make it fail?  It'll
> make it easier to keep this from regressing in the future.
> 

Jeff, not sure exactly what you were after but have attached four versions that all work correctly

1 - Set font-family before adding to the document
2 - No text
3 - Populate from SVG load handler instead of html load handler
4 - Adding an alert

If you're looking for something to put into a test suite to provide "expected results", avoid 2 as it gives a different result (having no text). Let me know if you need something more or something different.
It's possible this bug might be related to one or both of bug 374293 and bug 375342 which both involve SVG images disappearing with timing dependencies (although those two regressed at an earlier point than this). Not that I'm lobbying to get three of my bugs addressed for the price of one of course.
Having re-read bug 375342, the symptoms are very very similar. I think it would be worthwhile someone taking a look at both of these two together (had no interest in bug 375342 as yet). I've been working around bug 375342 for ages by adding a small delay after setting font-family and that might explain why some of my SVG images were unaffected by this bug.
(In reply to comment #13)
> Jeff, not sure exactly what you were after but have attached four versions
> that all work correctly

Sorry, I could have been clearer.  The final goal of the tests is to make them reftests, as described in this (rather long) document:

http://developer.mozilla.org/en/docs/Creating_reftest-based_unit_tests

I can summarize what's relevant to a test author as follows:

- a reftest consists of a test document and a reference document
  - the test document determines how it looks in a "tricky" way
    - e.g., looking like a single box of a single color by being made up of
      the colored borders of a bunch of individual boxes, as seen through
      colored, alpha-transparent boxes
  - the reference document determines how it looks in a simple,
    easy-to-understand way
    - e.g. just being one box with its colors set using a background: rule
- the test passes if one of the following is true (as chosen by the author):
  - the documents have pixel-equivalent renderings
  - the documents render differently (useful for, e.g., comparing various
    text alignments to "justify" to ensure justification is different from
    centering)

Note that success/failure is determined entirely by how the documents render -- alerts can't be used to indicate success or failure because they don't affect rendering.

There are a couple other complications related to exactly when renderings are compared, but if you can set everything up from the onload handler you don't need to worry about them.

As far as what you've posted goes, except for the alert test it sounds like the tests are fine (I think; I haven't followed this bug super-closely).  The only thing I'd suggest is making the pairing between a test and its reference more obvious; currently the attachments aren't particularly organized.  Just uploading a zip containing document pairs would probably work, with the test files following this naming convention:

380516-01.html
380516-01-ref.html
380516-02.svg
380516-02-ref.svg
380516-03.xhtml
380516-03-ref.xhtml
...

Thanks again for helping!
Attached file reftests zip file
Jeff, having read up about reftests I think the attached is what you need.
One thing that bothers me slightly is that sometimes these tests might pass because of time dependency. Just now, I've observed the image to render OK on occasion.
The problem is that when we do an incremental reflow of the outer SVG frame (triggered by the style change on the kid), it doesn't set its desired size (which in practice means it collapses to 0x0).

I didn't roll the other reftests into this patch, based on the comment about them not being reliable, and on the fact that the reference files are much too complicated.  I'd expect to see no script (or HTML) in the reference files; I'd rather expect to not see any HTML in the test files either, unless the bug is completely impossible to demonstrate with a standalone SVG document...
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Attachment #264714 - Flags: superreview?(dbaron)
Attachment #264714 - Flags: review?(jwatt)
Oh, and I bet my testcase shows the problem with builds from before bug 378975 too -- this looks like a reflow branch regression to me.
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Summary: SVG image not rendered - regression between 3.0a4pre and 3.0a5pre → [FIX]SVG image not rendered - regression between 3.0a4pre and 3.0a5pre
Target Milestone: --- → mozilla1.9alpha6
Comment on attachment 264714 [details] [diff] [review]
Proposed fix and simple test

sr=dbaron
Attachment #264714 - Flags: superreview?(dbaron) → superreview+
Boris, great if you've fixed this - interesting the fix would suggest an earlier regression than what I observe for this bug. Is there any chance at all the same issue might explain bug 375342 and/or bug 374293 (even though they regressed earlier) ?

I think those two and this are all timing dependent which might explain why the exact regression range is hard to pin down.
Yeah, this patch fixes both of those bugs, which makes sense.
Blocks: 374293, 375342
That is very excellent news. Three for the price of one - that's value for money that is.

Out of interest, where do you reckon the time dependency arises from ? Is it that the document will resize anyway at some point (e.g. when the HTML is parsed) and it depends if the SVG rendering completes first (or something like that) ?
The dependency comes from the ordering of the action that posts the incremental reflow event and the initial/resize reflows we do during pageload.  If the event fires after those, you see the bug.
Got it. That would explain everything I've seen on all three bugs I think. And why its been such a pig to produce test cases and why the test cases are so complex. And probably why we didn't see this bug regress earlier (something that changed in the latest trunk slightly affecting this ordering and making the bug obvious).
Comment on attachment 264714 [details] [diff] [review]
Proposed fix and simple test

r=jwatt. Thanks!
Attachment #264714 - Flags: review?(jwatt) → review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Blocks: 374345
You need to log in before you can comment on or make changes to this bug.