756419 - White standalone image background flashes into view when switching tabs

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[ferongr]

Ever since bug 754133 landed, switching between standalone image tabs has become a disorienting procedure. With discarded decoded image data the system takes some time to re-decode the image. In that interval the white background is displayed instead of the image, resulting in brief a white flash.

Also, when slowly loading a large standalone image the experience is not ideal either.

Ideally the white background should only appear if there's a fully decoded image in view.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Confirmed. It's really annoying!

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Gentle ping
and what are we going to do with this bug?

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this regression in 15 and bring Jared in to the discussion to see if we can forward fix this before 15 ships or whether we have to look at backing out bug 754133.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm recommending:
1) changing image background from white to black only when image is loading and is not completely rendered - this will prevent this flashing issue
2) changing background from dark gray to black - will have win in battery saving on mobiles, and dark grey isn't needed as we have now bug #754133 fixed. But this probably need a new bug.

What do you guys think about these ideas?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Lukas Blakk [:lsblakk] from comment #3)
> Tracking this regression in 15 and bring Jared in to the discussion to see
> if we can forward fix this before 15 ships or whether we have to look at
> backing out bug 754133.

This isn't severe enough to back out bug 754133.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I agree with Dao.

It looks like this could be achieved by hooking in to the https://developer.mozilla.org/en/XPCOM_Interface_Reference/imgIDecoderObserver#onStopDecode() event. However, I don't think I'll have time to get this bug fixed before Aurora 15 merges to Beta 15.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Jared, assigning to you for now unless you can find someone else to take it. It's still early in the Beta cycle so we could take a fix in the coming weeks if you have time to prepare something.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch sets and unsets the "decoded" class on the image when the image is decoded and discarded, respectively.

This *should* reduce instances of the white flashing, but I'm unable to reproduce it locally.

bholley: I flagged you for feedback due to the note in onStopDecode here <https://developer.mozilla.org/en/XPCOM_Interface_Reference/imgIDecoderObserver#onStopDecode%28%29>

I think we'll be OK without the code to remove the observer inside of onStopDecode since the observer is still removed in ImageDocument::Destroy.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 643958 [details] [diff] [review]
Patch


> 
> NS_IMETHODIMP
> ImageDocument::OnStopDecode(imgIRequest *aRequest,
>                             nsresult aStatus,
>                             const PRUnichar *aStatusArg)
> {
>   UpdateTitleAndCharset();
> 
>-  nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mImageContent);
>-  if (imageLoader) {
>-    mObservingImageLoader = false;
>-    imageLoader->RemoveObserver(this);
>-  }
>+  // Update the background-color of the image only after the
>+  // image has been decoded to prevent flashes of just the
>+  // background-color.
>+  mImageContent->SetAttr(kNameSpaceID_None, nsGkAtoms::_class,
>+                         NS_LITERAL_STRING("decoded"), false);


This is wrong, because OnStopDecode is called at _load_ time (once per image), rather than at decode time. The comment on MDN and in the source sort of explains this, but is unfortunately hampered by a very terrible typo (it should say that this method is a companion to OnStopRequest, not to OnStopDecode, which doesn't make sense).

If you want to hear about when an image finishes decoding, listing for OnStopContainer.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This patch uses OnStopContainer to get the proper decode notification.

Note that the observer is no longer removed in OnStopDecode, since it is still needed for OnDiscard. We could remove the observer in OnDiscard, but then we would need to re-add the observer when the image starts decoding again (but wouldn't get that notification since the necessary observer would be removed).

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 649087 [details] [diff] [review]
Patch v2

r=bholley. Presumably roc's r+ means we're ok exposing .decoded naked/unprefixed?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, we're ok unprefixed because this class is only applied on images in top-level synthetic documents.

https://hg.mozilla.org/integration/mozilla-inbound/rev/35ac9dfcef9b

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35ac9dfcef9b

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 649087 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 754133
User impact if declined: users will see a brief flash of white before the image reloads when switching tabs
Testing completed (on m-c, etc.): landed on m-c on 8/9, tested locally
Risk to taking this patch (and alternatives if risky): minor risk, involves keeping an observer around longer than before, but no issues are expected
String or UUID changes made by this patch: none

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6bc2cad5a3b4
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bd72ea6ab99

Comment 16

[u279076]

Is there a testcase QA can use to verify this is fixed?

Comment 17

[ferongr]

While not a real testcase, any sufficiently large image that doesn't decode instantly (e.g. http://kurtjohnson.net/Photos/2006.12.30.MP_pano.jpg )should suffice to verify.

1. Open and wait for the image to load.
2. Switch to another tab and wait for the image data to be discarded.
3. Switch back to the tab with the image.

Without the fix the white background intended to help with transparent images shows up until the image is decoded again and displayed.

With the fix, while the image is decoded you see no white background.

Comment 18

[u279076]

Thanks John. We'll try to verify it using that test.

Comment 19

[Virgil Dicu [:virgil] [QA]]

Tested with http://www.free-pictures-photos.com/ and suggestions in comment 17 on 15b5 - Windows 7, Ubuntu and Mac OS.

No white background is displayed anymore and no other glitches spotted while switching tabs with large images.