Last Comment Bug 756419 - White standalone image background flashes into view when switching tabs
: White standalone image background flashes into view when switching tabs
[qa:see comment 17]
: regression, testcase
Product: Toolkit
Classification: Components
Component: Themes (show other bugs)
: 15 Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jared Wein [:jaws] (please needinfo? me)
: Virgil Dicu [:virgil] [QA]
: Dão Gottwald [:dao]
Depends on: 788418 CVE-2013-0775 841547
Blocks: 754133 783874 1089880
  Show dependency treegraph
Reported: 2012-05-18 04:25 PDT by ferongr
Modified: 2014-10-27 14:54 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (12.13 KB, patch)
2012-07-19 12:11 PDT, Jared Wein [:jaws] (please needinfo? me)
roc: review+
bobbyholley: feedback-
Details | Diff | Splinter Review
Patch v2 (12.60 KB, patch)
2012-08-05 03:14 PDT, Jared Wein [:jaws] (please needinfo? me)
roc: review+
bobbyholley: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description ferongr 2012-05-18 04:25:57 PDT
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] - (ni? me) 2012-05-18 14:44:22 PDT
Confirmed. It's really annoying!
Comment 2 Virtual_ManPL [:Virtual] - (ni? me) 2012-07-12 14:27:38 PDT
Gentle ping
and what are we going to do with this bug?
Comment 3 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-12 15:41:02 PDT
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] - (ni? me) 2012-07-12 23:20:41 PDT
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] 2012-07-13 03:29:19 PDT
(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) 2012-07-13 07:53:39 PDT
I agree with Dao.

It looks like this could be achieved by hooking in to the 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 2012-07-18 15:52:52 PDT
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) 2012-07-19 12:11:22 PDT
Created attachment 643958 [details] [diff] [review]

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 <>

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) (busy with Stylo) 2012-07-20 00:59:18 PDT
Comment on attachment 643958 [details] [diff] [review]

> 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) 2012-08-05 03:14:48 PDT
Created attachment 649087 [details] [diff] [review]
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) (busy with Stylo) 2012-08-08 12:37:17 PDT
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) 2012-08-08 12:44:12 PDT
Yeah, we're ok unprefixed because this class is only applied on images in top-level synthetic documents.
Comment 13 Ed Morley [:emorley] 2012-08-09 04:53:28 PDT
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-08-09 22:32:02 PDT
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 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-15 16:31:28 PDT
Is there a testcase QA can use to verify this is fixed?
Comment 17 ferongr 2012-08-15 16:48:40 PDT
While not a real testcase, any sufficiently large image that doesn't decode instantly (e.g. )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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-15 17:10:07 PDT
Thanks John. We'll try to verify it using that test.
Comment 19 Virgil Dicu [:virgil] [QA] 2012-08-17 05:03:07 PDT
Tested with 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.

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