Closed Bug 697978 Opened 13 years ago Closed 13 years ago

reftest-analyzer images do not display using Nightly

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla10
Tracking Flags:
Tracking Status
firefox10 - ---

People

(Reporter: jfkthame, Assigned: longsonr)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

updated with reftest
9.71 KB, patch
dholbert
: review+
bzbarsky
: review+
Details | Diff | Splinter Review 
Using current Nightly (or my local build) on OS X, test images do not display properly in reftest-analyzer.xhtml; the main part of the window content, where the testcase image should be, is blank. Sometimes "Image 1" displays OK but "Image 2" is blank. However, moving the mouse around in the blank area shows (via the magnified portion at top-left) that the image data was loaded, and the "Circle differences" option still highlights the correct areas of the canvas.

Good: http://hg.mozilla.org/mozilla-central/rev/311fdb9b38b7 (2011-10-20)
Bad:  http://hg.mozilla.org/mozilla-central/rev/2afc252b4d39 (2011-10-21)

Pushlog range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=311fdb9b38b7&tochange=2afc252b4d39

Maybe a regression from one of the GLContext-related patches in that range?
Keywords: regression
Seeing the same problem on Linux and Windows, so it's not a platform-specific issue.
OS: Mac OS X → All
Summary: reftest-analyzer images do not display using Nightly on OS X → reftest-analyzer images do not display using Nightly
(In reply to Jonathan Kew (:jfkthame) from comment #0)
> Maybe a regression from one of the GLContext-related patches in that range?

Wrong guess; the regression actually comes from bug 693183 (changeset dfa418ec22f1). Moving to SVG.
Blocks: 693183
Component: Graphics → SVG
QA Contact: thebes → general
The reftest-analyzer uses display:none images. Moving the image attribute processing to occur in frames means that display:none images don't really work so well now. Perhaps reftest-analyzer could use visibility:hidden instead?
Possibly, but this is a change in behavior that could affect other sites, too; surely the primary question here is whether or not it's correct. Is it?
SVG is supposed to work in a display:none context but firefox basically doesn't and never has properly. bug 376027 tracks that. Animation of any attribute e.g. the image x, y, width and height requires a frame for instance.

We could put the AfterSetAttr call back in the nsSVGImageElement and check in there whether the image has a frame, if there is return in that method as the frame will handle things and if there isn't then do what happened before. That will make this case work.
OK, thanks for the explanation. Seems like we ought to fix this, so that we don't regress something that was previously working, even if it wasn't complete.
Moving _loading_ of the image into the frame is just broken.  Why was that done, exactly?  Why can't you just move that whole xlink:href block back into the content class and take it out of the frame completely?
Attached patch patch (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 570279 [details] [diff] [review]
patch

Does this fix it for you?

        Attachment #570279 -
        Flags: feedback?(jfkthame)

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #7)
> Moving _loading_ of the image into the frame is just broken.  Why was that
> done, exactly?  Why can't you just move that whole xlink:href block back
> into the content class and take it out of the frame completely?

All our SMIL animation works off frames. If we have it in content then we need special overrides for frames which I'm trying to get rid of.

    
    

    
  
And in particular, unlike the other attributes that were moved, which affect just the rendering, the loading business affects all sorts of things that have nothing to do with frames.  The result of that change is that setting the xlink:href attribute on an image may now work or not pretty randomly depending on timing and what other DOM changes are going on, without even involving display:none.  For example, this testcase:

  <body>
    <svg></svg>
    <script>
      var img = document.createElementNS("http://www.w3.org/2000/svg", "image");
      img.setAttribute("width", "100");
      img.setAttribute("height", "100");
      document.querySelector("svg").appendChild(img);
      img.setAttributeNS("http://www.w3.org/1999/xlink", "xlink:href", "test.png");
    </script>
  </body>

doesn't show the image in a current nightly.  But if I reorder the last two lines of the script, or add a style flush right before the setAttributeNS call or any of a variety of other changes (e.g. put the <svg> inside a contenteditable div) that affect the exact timing of frame construction it will suddenly start working.

As I said, broken.  We need to fix this.

    
    

    
  
> All our SMIL animation works off frames.

I think animating xlink:href on <svg:image> is a much lower priority than having it actually work in the first place.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #11)
> 
> As I said, broken.  We need to fix this.

The patch I've just attached should do it I think.

    
    

    
  
And maybe that just means we need to fix SMIL...

Does SMIL invoke the AttributeChanged method of the frame without actually changing the attribute on the content or something?

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #14)
> And maybe that just means we need to fix SMIL...
> 
> Does SMIL invoke the AttributeChanged method of the frame without actually
> changing the attribute on the content or something?

It does.

    
    

    
  
Is there a reason it could not call BeforeSetAttr/AfterSetAttr on the content?  Would that eliminate the frame dependency?  Or does it need frames for basic functioning (e.g. getting style data)?

    
    

    
  
Comment on attachment 570279 [details] [diff] [review]
patch

Seems to fix the reftest-analyzer display problem for me.

        Attachment #570279 -
        Flags: feedback?(jfkthame) → feedback+

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #16)
> Is there a reason it could not call BeforeSetAttr/AfterSetAttr on the
> content?  Would that eliminate the frame dependency?  Or does it need frames
> for basic functioning (e.g. getting style data)?

Style data is the main issue I think.

    
  

        Attachment #570279 -
        Flags: review?(dholbert)

    
    

    
  
I imagine I can turn comment 11 into a reftest.

    
  

        Attachment #570279 -
        Flags: review?(dholbert)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated (obsolete)
        — 
          — Splinter Review
      

    


  
Assignee: nobody → longsonr

        Attachment #570279 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #570331 -
        Attachment is obsolete: true

        Attachment #570346 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 570346 [details] [diff] [review]
updated with reftest

>diff --git a/layout/reftests/svg/image/image-load-01.svg b/layout/reftests/svg/image/image-load-01.svg
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/image/image-load-01.svg
>@@ -0,0 +1,20 @@
>+<!--
>+     Any copyright is dedicated to the Public Domain.
>+     http://creativecommons.org/publicdomain/zero/1.0/
>+-->
>+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
>+
>+  <title>Testcase to ensure that fill is ignored for images</title>

This <title> seems incorrect.

I'm also not sure I can review additions to nsContentUtils -- perhaps bz can give the thumbs-up on that tweak.

Other than that, r=me.

        Attachment #570346 -
        Flags: review?(dholbert)

        Attachment #570346 -
        Flags: review?(bzbarsky)

        Attachment #570346 -
        Flags: review+

    
    

    
  
Comment on attachment 570346 [details] [diff] [review]
updated with reftest

>+nsresult
>+nsSVGImageElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>+                                const nsAString* aValue, bool aNotify)
>+{
[...]
>+      if (aValue) {
>+        LoadSVGImage(PR_TRUE, aNotify);

Also: s/PR_TRUE/true/
Hardware: x86 → All

    
    

    
  
Comment on attachment 570346 [details] [diff] [review]
updated with reftest

r=me; sharing code == good.

        Attachment #570346 -
        Flags: review?(bzbarsky) → review+

    
  
Target Milestone: --- → mozilla10

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c91cdd0896c7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
    

    
  
Hi, just as a matter of interest, what is the !IsCallerChrome() for in:

nsContentUtils::IsImageSrcSetDisabled()
  return Preferences::GetBool("dom.disable_image_src_set") && !IsCallerChrome();
}

We're getting a crash from IsCallerChrome(), see bug 1219253 comment #12.
Flags: needinfo?(longsonr)

    
    

    
  
it's there because if dom.disable_image_src_set is true Chrome should still be able to display images. It mirrors the same line in html image display (or at least it certainly used to).
Flags: needinfo?(longsonr)

    
    

    
  
Also if you look carefully at the patch it just moves the existing code for html images into a common function that SVG images can call.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: