reftest-analyzer images do not display using Nightly

RESOLVED FIXED in mozilla10

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: Robert Longson)

Tracking

({regression})

Trunk
mozilla10
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Keywords: regression
tracking-firefox10: --- → ?
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Comment 2

6 years ago
(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
(Assignee)

Comment 3

6 years ago
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?
(Reporter)

Comment 4

6 years ago
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?
(Assignee)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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?
(Assignee)

Comment 8

6 years ago
Created attachment 570279 [details] [diff] [review]
patch
(Assignee)

Comment 9

6 years ago
Comment on attachment 570279 [details] [diff] [review]
patch

Does this fix it for you?
Attachment #570279 - Flags: feedback?(jfkthame)
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
(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?
(Assignee)

Comment 15

6 years ago
(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)?
(Reporter)

Comment 17

6 years ago
Comment on attachment 570279 [details] [diff] [review]
patch

Seems to fix the reftest-analyzer display problem for me.
Attachment #570279 - Flags: feedback?(jfkthame) → feedback+
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #570279 - Flags: review?(dholbert)
(Assignee)

Comment 19

6 years ago
I imagine I can turn comment 11 into a reftest.
(Assignee)

Updated

6 years ago
Attachment #570279 - Flags: review?(dholbert)
(Assignee)

Comment 20

6 years ago
Created attachment 570331 [details] [diff] [review]
updated
Assignee: nobody → longsonr
Attachment #570279 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 570346 [details] [diff] [review]
updated with reftest
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+
(Assignee)

Comment 25

6 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/c91cdd0896c7
Flags: in-testsuite+
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/c91cdd0896c7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
tracking-firefox10: ? → -

Comment 27

2 years ago
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)
(Assignee)

Comment 28

2 years ago
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)
(Assignee)

Comment 29

2 years ago
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.