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

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Duncan Loveday]

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)

Comment 1

[Duncan Loveday]

Attachment: test SVG source

Comment 2

[Duncan Loveday]

Attachment: test HTML source

Comment 3

[Duncan Loveday]

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.

Comment 4

[Ria Klaassen (not reading all bugmail)]

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

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Duncan Loveday]

Attachment: test HTML source - works OK 1 of 3

This version works OK because font-family is set before adding the element to the document.

Comment 7

[Duncan Loveday]

Attachment: test HTML source - works OK 2 of 3

This version works OK because there is no text in the SVG

Comment 8

[Duncan Loveday]

Attachment: test HTML source - works OK 3 of 3

This version works OK because the SVG is populated from the SVG load handler rather than from the HTML load handler.

Comment 9

[Duncan Loveday]

Attachment: test HTML source - works OK after adding an alert

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.

Comment 10

[Duncan Loveday]

Attachment: test HTML source - works OK after adding an alert

Re-attaching version that works libe

Comment 11

[Duncan Loveday]

Attachment: test HTML source - works OK 3 of 3

Finger trouble - original had both workarounds 1 and 3

Comment 12

[Duncan Loveday]

Attachment: test HTML source - works OK 2 of 3

Finger trouble - original had both workarounds 1 and 2

Comment 13

[Duncan Loveday]

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

Comment 14

[Duncan Loveday]

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.

Comment 15

[Duncan Loveday]

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.

Comment 16

[Jeff Walden [:Waldo]]

(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!

Comment 17

[Duncan Loveday]

Attachment: reftests zip file

Jeff, having read up about reftests I think the attached is what you need.

Comment 18

[Duncan Loveday]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix and simple test

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...

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

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

Comment on attachment 264714 [details] [diff] [review]
Proposed fix and simple test

sr=dbaron

Comment 22

[Duncan Loveday]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah, this patch fixes both of those bugs, which makes sense.

Comment 24

[Duncan Loveday]

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) ?

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Duncan Loveday]

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 27

[Jonathan Watt [:jwatt]]

Comment on attachment 264714 [details] [diff] [review]
Proposed fix and simple test

r=jwatt. Thanks!

Comment 28

[Boris Zbarsky [:bzbarsky]]

Checked in.