Closed Bug 1395964 Opened 7 years ago Closed 6 years ago

Image flash when current source changes

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Version:
54 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: oliverjash, Assigned: emilio)

References

Details

(Keywords: regression, testcase, Whiteboard: [webcompat] [geckoview:klar:p2][layout:backlog:2019q1])

Attachments

(1 file)

Bug 1395964 - Make image loading changes not flash. r=tnikkel
(deleted), text/x-phabricator-request
Details
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.25 Safari/537.36 Steps to reproduce: Example: http://output.jsbin.com/lacufep/2 In the example, click the image. This will modify the `sizes` attribute, forcing the evaluated source to change. Alternatively: 1. Create a responsive image 2. Trigger a change to the parsed image size (from `sizes` attribute; e.g. resize the document), forcing the evaluated `currentSrc` to change Actual results: Visually, the old image source is replaced with the new one, i.e. the current source is changed, however there is a brief flash in between the old source showing and the new source being shown. Expected results: There should be no flash, as this is distracting for users.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Images
Ever confirmed: true
Keywords: regression, regressionwindow-wanted, testcase
Product: Firefox → Core
(FYI, It is considerably improved if setting image.decode-immediately.enabled=true. And the issue disappear if disabling e10s and setting image.decode-immediately.enabled=true both.)
At least, Regression window of the problem(the entire image once becomes white) is as follows. Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8c07b084c0f8fb17885979c3322f5e15a1298bbe&tochange=aafd6db2fbb452350c42a1d839ce96ec6e183b53 Regressed by: Bug 1207355 Although there is a small flicker(bottom part of image) sometimes even before the landing of Bug 1207355.
Blocks: 1207355
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
Priority: -- → P3
Too late for a fix for 57 since we're a few days away from release.
status-firefox57: affected → wontfix
status-firefox58: --- → fix-optional
See Also: → https://webcompat.com/issues/8648
Whiteboard: [webcompat]
status-firefox59: ? → wontfix
status-firefox63: --- → affected
Whiteboard: [webcompat] → [webcompat] [geckoview:klar:p2]
Product: Core → Core Graveyard
Product: Core Graveyard → Core
Too late to fix in 63, but we could still take a patch in 65 and potentially, 64 beta.
status-firefox63: affected → wontfix
status-firefox64: --- → fix-optional
Sean, putting this on your radar. It might affect other similar sites as well.
Flags: needinfo?(svoisen)
Bug 1253995 looks related. Probably we're clearing mPrevImage too early, or something like that...
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1253995
Given this is a webcompat bug that affects the Google image search user experience, I'm going to bump to P2 and see if we can get some movement on it.
Flags: needinfo?(svoisen)
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1222711
Priority: P3 → P2

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Bug 1253995 looks related. Probably we're clearing mPrevImage too early, or
something like that...

The reason this happens is because each time we change the load state we reframe the image, loosing the previous image of course:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/layout/base/RestyleManager.cpp#490

Depends on: 1472637
Depends on: 1531582

Tentatively taking.

Assignee: nobody → emilio

There are two parts for this fix.

The first one is trivial, and consists on not reframing the image when the
loading state changes. Bug 1472637 makes the decision of whether to construct an
image frame not depend on this, so this is sound.

We need to avoid reframing because otherwise we lose track of the previously
painted image.

The second part is not clearing mPrevImage if our intrinsic size changes.

This is needed if we want to handle cases like the reporter's example or Google
Images. Rendering the old image upscaled is much better than not rendering
anything while it's loading.

I added a test for the reframing bit, but I don't know how to add a test for the
second bit.

Blocks: 997153
Whiteboard: [webcompat] [geckoview:klar:p2] → [webcompat] [geckoview:klar:p2][layout:backlog:2019q1]
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/3f1e9dae2467 Make image loading changes not reframe. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/fdc7b66d76a4 Don't clear the previously painted image on intrinsic size changes. r=tnikkel

https://hg.mozilla.org/mozilla-central/rev/3f1e9dae2467
https://hg.mozilla.org/mozilla-central/rev/fdc7b66d76a4

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

We have shipped multiple releases with this bug, let's make the fix the trains, marking 67 as wontfix.

status-firefox67: affected → wontfix
Regressions: 1546739
Regressions: 1554664
Regressions: 1589955
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: