Last Comment Bug 697978 - reftest-analyzer images do not display using Nightly
: reftest-analyzer images do not display using Nightly
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
:
Mentors:
Depends on:
Blocks: 693183
  Show dependency treegraph
 
Reported: 2011-10-28 07:12 PDT by Jonathan Kew (:jfkthame)
Modified: 2015-11-21 23:32 PST (History)
9 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch (5.27 KB, patch)
2011-10-28 09:19 PDT, Robert Longson
jfkthame: feedback+
Details | Diff | Review
updated (7.68 KB, patch)
2011-10-28 12:48 PDT, Robert Longson
no flags Details | Diff | Review
updated with reftest (9.71 KB, patch)
2011-10-28 13:36 PDT, Robert Longson
dholbert: review+
bzbarsky: review+
Details | Diff | Review

Description Jonathan Kew (:jfkthame) 2011-10-28 07:12:50 PDT
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?
Comment 1 Jonathan Kew (:jfkthame) 2011-10-28 08:24:25 PDT
Seeing the same problem on Linux and Windows, so it's not a platform-specific issue.
Comment 2 Jonathan Kew (:jfkthame) 2011-10-28 08:36:57 PDT
(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.
Comment 3 Robert Longson 2011-10-28 08:43:59 PDT
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?
Comment 4 Jonathan Kew (:jfkthame) 2011-10-28 08:52:45 PDT
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?
Comment 5 Robert Longson 2011-10-28 09:01:08 PDT
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.
Comment 6 Jonathan Kew (:jfkthame) 2011-10-28 09:15:49 PDT
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.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-28 09:17:20 PDT
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?
Comment 8 Robert Longson 2011-10-28 09:19:00 PDT
Created attachment 570279 [details] [diff] [review]
patch
Comment 9 Robert Longson 2011-10-28 09:19:35 PDT
Comment on attachment 570279 [details] [diff] [review]
patch

Does this fix it for you?
Comment 10 Robert Longson 2011-10-28 09:21:14 PDT
(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.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-28 09:24:13 PDT
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.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-28 09:24:50 PDT
> 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.
Comment 13 Robert Longson 2011-10-28 09:26:21 PDT
(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.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-28 09:26:38 PDT
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?
Comment 15 Robert Longson 2011-10-28 09:27:14 PDT
(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.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-28 09:36:29 PDT
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 17 Jonathan Kew (:jfkthame) 2011-10-28 09:38:21 PDT
Comment on attachment 570279 [details] [diff] [review]
patch

Seems to fix the reftest-analyzer display problem for me.
Comment 18 Robert Longson 2011-10-28 09:40:08 PDT
(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.
Comment 19 Robert Longson 2011-10-28 09:47:26 PDT
I imagine I can turn comment 11 into a reftest.
Comment 20 Robert Longson 2011-10-28 12:48:29 PDT
Created attachment 570331 [details] [diff] [review]
updated
Comment 21 Robert Longson 2011-10-28 13:36:47 PDT
Created attachment 570346 [details] [diff] [review]
updated with reftest
Comment 22 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-10-28 14:26:52 PDT
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.
Comment 23 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-10-28 14:29:44 PDT
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/
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-28 20:20:53 PDT
Comment on attachment 570346 [details] [diff] [review]
updated with reftest

r=me; sharing code == good.
Comment 26 Ed Morley [:emorley] 2011-10-30 10:36:40 PDT
https://hg.mozilla.org/mozilla-central/rev/c91cdd0896c7
Comment 27 Jorg K (GMT+2, PTO during summer, NI me) 2015-11-21 15:20:42 PST
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.
Comment 28 Robert Longson 2015-11-21 23:22:37 PST
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).
Comment 29 Robert Longson 2015-11-21 23:32:56 PST
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.

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