SVG image element flickers when updating the value of the xlink:href attribute

VERIFIED FIXED in Firefox 54

Status

()

Core
SVG
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: Nick, Assigned: tnikkel)

Tracking

({regression})

53 Branch
mozilla55
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 wontfix, firefox54+ verified, firefox55- verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

11 months ago
Created attachment 8870152 [details]
HTML file with javascript that demonstrates the bug.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.96 Safari/537.36

Steps to reproduce:

1) Create an SVG element with an <image> element with a xlink:href attribute. Set the value of said attribute to the URL of an image. Example:

<svg viewBox="0 0 1920 1080">
  <image xlink:href="https://placehold.it/1920x1080?1" />
</svg>

2) Create an Image object with javascript and attach an onload callback function. Example:

var image = new Image();
image.onload = imgLoaded;

3) Set the src of the Image object to something other than the current value of the <image> element's xhref:link attribute. Example:

image.src = 'https://placehold.it/1920x1080?2';

4) In the onload callback function, set the value of the xhref:link attribute in the <image> element to the src of the image object. Example:

function imgLoaded() {
  document.getElementsByTagName('image')[0].setAttribute('xlink:href', 'https://placehold.it/1920x1080?2');
}

The bug can be reproduced in Firefox 53 and Nightly with build ID 20170522030207. It can not be reproduced in Firefox 52.


Actual results:

The image flickers (i.e. there's a delay between the first image and the second image causing a blink effect).


Expected results:

The second image should be shown without any flicker.
(Reporter)

Comment 1

11 months ago
I mistakenly referred to the image element's attribute as xhref:link in a couple of places. It should be xlink:href and nothing else.
Given that this is a regression from 52 would you mind using mozregression(http://mozilla.github.io/mozregression/) to find a regression range?
Flags: needinfo?(theg72)
Component: Untriaged → SVG
Product: Firefox → Core

Comment 3

11 months ago

(In reply to Robert Longson from comment #2)
> Given that this is a regression from 52 would you mind using
> mozregression(http://mozilla.github.io/mozregression/) to find a regression
> range?

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f608a5a5d8e2472396a0f4cd04778730866b8ae0&tochange=a511e97862314dc1b90c531205489b972f5d50bd

Timothy Nikkel — Bug 1341881. Don't do sync decode for SVG <image> elements images. r=aosmond
Blocks: 1341881
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
Ever confirmed: true
Flags: needinfo?(theg72) → needinfo?(tnikkel)
Keywords: regression
(Assignee)

Comment 4

11 months ago
Created attachment 8870272 [details] [diff] [review]
patch
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Attachment #8870272 - Flags: review?(mats)

Updated

11 months ago
Attachment #8870272 - Flags: review?(mats) → review+

Comment 5

11 months ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/689dc2a3119a
Apply the same sync decoding heuristic to SVG <image> as we do to HTML <img>. r=mats

Comment 6

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/689dc2a3119a
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Wow, that was fast!  theg72, want to verify this fix in tomorrow's Nightly build?
We should probably also uplift this to beta 54.
status-firefox53: affected → wontfix
tracking-firefox54: ? → +
tracking-firefox55: ? → -
Flags: qe-verify+
Flags: needinfo?(theg72)
(Reporter)

Comment 8

11 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> Wow, that was fast!  theg72, want to verify this fix in tomorrow's Nightly
> build?
> We should probably also uplift this to beta 54.

Awesome, that was really fast indeed! I'd be happy to verify it in tomorrow's nightly. Thanks!

Comment 9

11 months ago
Want to request uplift of this ahead of verification? :)
Flags: needinfo?(tnikkel)
(Reporter)

Comment 10

11 months ago
(In reply to theg72 from comment #8)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> > Wow, that was fast!  theg72, want to verify this fix in tomorrow's Nightly
> > build?
> > We should probably also uplift this to beta 54.
> 
> Awesome, that was really fast indeed! I'd be happy to verify it in
> tomorrow's nightly. Thanks!

I have tested on the latest Nightly build, and the image still blinks once (the first time the image updates), but I can live with that. It's definitely much better than before when it kept flickering continuously. Thanks to everyone involved for resolving this so quickly, much appreciated!
Flags: needinfo?(theg72)
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 11

11 months ago
Comment on attachment 8870272 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1341881
[User impact if declined]: flickr when setting src in svg <image>
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this applies the same sync decoding logic from html <img> that we've had for years to svg <image>
[String changes made/needed]: none
Flags: needinfo?(tnikkel)
Attachment #8870272 - Flags: approval-mozilla-beta?

Comment 12

11 months ago
Comment on attachment 8870272 [details] [diff] [review]
patch

Improve the SVG image flickering issue and was verified. Beta54+. Should be in 54 beta 11.
Attachment #8870272 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This needs a rebased patch for Beta.
Flags: needinfo?(tnikkel)
(Assignee)

Comment 14

11 months ago
Created attachment 8871506 [details] [diff] [review]
patch for beta
Flags: needinfo?(tnikkel)

Comment 15

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9f43724221c5
status-firefox54: affected → fixed
Hello Nick, I could be missing something but I can't seem to reproduce the issue on the same builds you reproduced (Fx 53.0 and Nightly with buildID: 20170522030207). I just loaded the .html you attached and nothing happens, also modified it based on comment 1.

I need to reproduce so I can verify on 54.0 RC2 (https://archive.mozilla.org/pub/firefox/candidates/54.0-candidates/build3/).
Flags: needinfo?(theg72)
(Reporter)

Comment 17

11 months ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #16)
> Hello Nick, I could be missing something but I can't seem to reproduce the
> issue on the same builds you reproduced (Fx 53.0 and Nightly with buildID:
> 20170522030207). I just loaded the .html you attached and nothing happens,
> also modified it based on comment 1.
> 
> I need to reproduce so I can verify on 54.0 RC2
> (https://archive.mozilla.org/pub/firefox/candidates/54.0-candidates/build3/).

Hi Bogdan,

Can you please try this jsfiddle that I put together: https://jsfiddle.net/2dce2fj8/1/. I can reproduce the issue with this fiddle in Firefox 53.0.3 on Windows 10.
Flags: needinfo?(theg72)
(In reply to Nick from comment #17)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #16)
> > Hello Nick, I could be missing something but I can't seem to reproduce the
> > issue on the same builds you reproduced (Fx 53.0 and Nightly with buildID:
> > 20170522030207). I just loaded the .html you attached and nothing happens,
> > also modified it based on comment 1.
> > 
> > I need to reproduce so I can verify on 54.0 RC2
> > (https://archive.mozilla.org/pub/firefox/candidates/54.0-candidates/build3/).
> 
> Hi Bogdan,
> 
> Can you please try this jsfiddle that I put together:
> https://jsfiddle.net/2dce2fj8/1/. I can reproduce the issue with this fiddle
> in Firefox 53.0.3 on Windows 10.

Thanks for this Nick!
Unfortunately I can see the one time flicker on 53.0.3 (bad build) as well as on 54.0 RC2 (good build). I also tried with the Nightly build from 2017-05-22 but it's the same for me, only one time flicker per update, note that I used Windows 64bit and macOS 10.12.5. 

Could you try on your side with the 54 build, if it works for you then I can close this bug? link is in comment 16.
Flags: needinfo?(theg72)
(Reporter)

Comment 19

10 months ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> (In reply to Nick from comment #17)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #16)
> > > Hello Nick, I could be missing something but I can't seem to reproduce the
> > > issue on the same builds you reproduced (Fx 53.0 and Nightly with buildID:
> > > 20170522030207). I just loaded the .html you attached and nothing happens,
> > > also modified it based on comment 1.
> > > 
> > > I need to reproduce so I can verify on 54.0 RC2
> > > (https://archive.mozilla.org/pub/firefox/candidates/54.0-candidates/build3/).
> > 
> > Hi Bogdan,
> > 
> > Can you please try this jsfiddle that I put together:
> > https://jsfiddle.net/2dce2fj8/1/. I can reproduce the issue with this fiddle
> > in Firefox 53.0.3 on Windows 10.
> 
> Thanks for this Nick!
> Unfortunately I can see the one time flicker on 53.0.3 (bad build) as well
> as on 54.0 RC2 (good build). I also tried with the Nightly build from
> 2017-05-22 but it's the same for me, only one time flicker per update, note
> that I used Windows 64bit and macOS 10.12.5. 
> 
> Could you try on your side with the 54 build, if it works for you then I can
> close this bug? link is in comment 16.

Hi Bogdan, sorry for the delayed response. I have tried with 54.0 (the actual release version) on different platforms, and strangely enough the flicker still persists in the fiddle I sent you (at least on my Windows laptop), but it has stopped in the web app I'm developing for a client, which is what really matters. There's still that one time flicker, but I think that's good enough and I will consider the issue as resolved as far as I'm concerned. So please feel free to close this bug. Thank you!
Flags: needinfo?(theg72)
(In reply to Nick from comment #19)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> > (In reply to Nick from comment #17)
> > > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #16)
> > > > Hello Nick, I could be missing something but I can't seem to reproduce the
> > > > issue on the same builds you reproduced (Fx 53.0 and Nightly with buildID:
> > > > 20170522030207). I just loaded the .html you attached and nothing happens,
> > > > also modified it based on comment 1.
> > > > 
> > > > I need to reproduce so I can verify on 54.0 RC2
> > > > (https://archive.mozilla.org/pub/firefox/candidates/54.0-candidates/build3/).
> > > 
> > > Hi Bogdan,
> > > 
> > > Can you please try this jsfiddle that I put together:
> > > https://jsfiddle.net/2dce2fj8/1/. I can reproduce the issue with this fiddle
> > > in Firefox 53.0.3 on Windows 10.
> > 
> > Thanks for this Nick!
> > Unfortunately I can see the one time flicker on 53.0.3 (bad build) as well
> > as on 54.0 RC2 (good build). I also tried with the Nightly build from
> > 2017-05-22 but it's the same for me, only one time flicker per update, note
> > that I used Windows 64bit and macOS 10.12.5. 
> > 
> > Could you try on your side with the 54 build, if it works for you then I can
> > close this bug? link is in comment 16.
> 
> Hi Bogdan, sorry for the delayed response. I have tried with 54.0 (the
> actual release version) on different platforms, and strangely enough the
> flicker still persists in the fiddle I sent you (at least on my Windows
> laptop), but it has stopped in the web app I'm developing for a client,
> which is what really matters. There's still that one time flicker, but I
> think that's good enough and I will consider the issue as resolved as far as
> I'm concerned. So please feel free to close this bug. Thank you!

Thanks you so much Nick! Changing the tracking flag for 54 to verified based on reporter verification.
status-firefox54: fixed → verified

Comment 21

10 months ago
Hi Nick,

Could you try on your side with the 55 build, if it works for you, then I can close this bug?

Thanks, 
       Timea

Updated

10 months ago
Flags: needinfo?(theg72)
(Reporter)

Comment 22

10 months ago
(In reply to Timea Zsoldos from comment #21)
> Hi Nick,
> 
> Could you try on your side with the 55 build, if it works for you, then I
> can close this bug?
> 
> Thanks, 
>        Timea

Hi Timea,

The 55 build works the same as the 54 build for me (see comment #19). As far as I'm concerned, you can go ahead and close this bug. Thanks!
Flags: needinfo?(theg72)

Comment 23

10 months ago
Thanks you very much Nick! I am changing the tracking flag for firefox 55 to verified fixed, close this bug.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.