Closed Bug 997153 Opened 11 years ago Closed 5 years ago

Extra reflow when setting the src of an <img> element

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vingtetun, Assigned: emilio)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached file img_vs_background_image.html (obsolete) (deleted) —
While working on bug 967277 which is about panning on Firefox OS application, we noticed that there was a reflow when the src of an <img> element is set. We were able to get rid of this reflow by replacing <img> element by <div> with a background-image. That seems like a workaround and I forgot to open a followup to fix the underlying platform issue. Hopefully bz remind me that I should do it. thanks! I made a small testcase that loads the same image using the src of <img> element, and the style.backgroundImage of a <div> element. The testcase load first the <img> element, and you can observe that there is a reflow in the console. (By switching the pref devtools.browserconsole.filter.csslog to true in about:config). Loading the image as a background-image for the <div> there is no reflow. I tried with image with different sizes, < 60px, == 60px, > 60px, in order to see if it changes anything. But there is always the same reflow. I end up using a data: url just for the convenience of making a testcase in a single web page.
In the attached testcase, there is no way to avoid the reflow, because the image starts off with a block frame (because it has no src) but then later has to get an image frame, so we have to lay out the new frame. And that behavior is more or less required by specs in this case, so it's not trivial to change: we'd need a single frame class that can switch between blockframe behavior and imageframe behavior without relayout. I think you have two choices for maybe avoiding the reflow: 1) Give the <img> an src attribute initially. src="" might be enough. We will use an image frame for an <img> with an src and no alt attribute. Of course that assumes that your actual use case has no alt attribute... 2) Style the image with "-moz-force-broken-image-icon: 1". Not great, obviously. All this assumes that the testcase is a faithful reduction of the original page, of course.
Attached file img_vs_background_image.html (deleted) —
(In reply to Boris Zbarsky [:bz] from comment #1) > In the attached testcase, there is no way to avoid the reflow, because the > image starts off with a block frame (because it has no src) but then later > has to get an image frame, so we have to lay out the new frame. And that > behavior is more or less required by specs in this case, so it's not trivial > to change: we'd need a single frame class that can switch between blockframe > behavior and imageframe behavior without relayout. > > I think you have two choices for maybe avoiding the reflow: > > 1) Give the <img> an src attribute initially. src="" might be enough. We > will use an image frame for an <img> with an src and no alt attribute. Of > course that assumes that your actual use case has no alt attribute... > > 2) Style the image with "-moz-force-broken-image-icon: 1". Not great, > obviously. > I can see the reflow with both src="" and -moz-force-broken-image-icon: 1; I updated the testcase to make that more obvious. Once an image has successfully loaded (if I replace <img src=""> by <img src="any_image_that_works.png">) I don't see the reflow anymore.
Attachment #8407512 - Attachment is obsolete: true
Attached patch Possible fix (obsolete) (deleted) — Splinter Review
Vivien, this seems to fix the testcase for me. Does it help with the original issue?
Flags: needinfo?(21)
(In reply to Boris Zbarsky [:bz] from comment #3) > Created attachment 8407627 [details] [diff] [review] > Possible fix > > Vivien, this seems to fix the testcase for me. Does it help with the > original issue? If I apply the your attachment, and revert the changes made in bug 967277 I can still see the issue :/ I modify the contacts app to also use |-moz-force-broken-image-icon: 1;| and src="" by default. The only way I can get rid of the extra reflow is by setting the src of the <img> element to point to a real image or a data uri instead of "".
Flags: needinfo?(21)
Hmm. Do you also see an extra reflow on the testcase in this bug? If not, is can you extract a testcase (doesn't have to be too reduced) that shows the problem with this patch?
Flags: needinfo?(21)
(In reply to Boris Zbarsky [:bz] from comment #5) > Hmm. Do you also see an extra reflow on the testcase in this bug? If not, > is can you extract a testcase (doesn't have to be too reduced) that shows > the problem with this patch? I don't see a reflow in the testcase with this patch. I will try to made a different testcase.
Flags: needinfo?(21)
Attached file alternate_src_empty_with_image.html (deleted) —
Still investigating for the right testcase. In the meantime I made a small new testcase, is this one supposed to reflow as well with your patch ?
That would be expected to reflow with my patch, yes, when it goes from having an image set to not having an image, since in general that can cause us to need to create a non-nsImageFrame. We could change things so that "-moz-force-broken-image-icon: 1" prevents that reflow, of course, but I'm not 100% convinced we want to add more dependencies on that. We could also change things so that having no alt and having @src will avoid reflowing there. That might be more palatable....
(In reply to Boris Zbarsky [:bz] from comment #8) > That would be expected to reflow with my patch, yes, when it goes from > having an image set to not having an image, since in general that can cause > us to need to create a non-nsImageFrame. > That may explain some of the reflow on the contacts app as it reset the src when the image is offscreen, and set it back to a new blob url when the image is onscreen. > We could change things so that "-moz-force-broken-image-icon: 1" prevents > that reflow, of course, but I'm not 100% convinced we want to add more > dependencies on that. > fwiw, I don't think we will end up using -moz-force-broken-image-icon in apps as it is pretty ugly :) > We could also change things so that having no alt and having @src will avoid > reflowing there. That might be more palatable.... That may help. As I said I'm still trying to figure out the right testcase. But the app is definitively doing src="" when the <img> element is offscreen, and src= blobUrl when the <img> is onscreen.
In theory if we used img elements with the new visibility optimizations, maybe we don't need to explicitly clear the src attribute? The CSS background-image approach keeps them locked in memory, so we definitely need this if we stay with background-image. Of course, the visibility code seems to be causing some issues of its own. See bug 981899.
Attachment #8407627 - Attachment is obsolete: true
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(21)
Just tried on the device and I still see the reflows. Will try a new test case on monday :/
Flags: needinfo?(21)
Vivien, what are the steps you are taking on device to produce the reflow? I'm not asking for a reduced testcase, just want to know how to reproduce what you are seeing on a device myself.
Flags: needinfo?(21)
(In reply to Timothy Nikkel (:tn) from comment #13) > Vivien, what are the steps you are taking on device to produce the reflow? > I'm not asking for a reduced testcase, just want to know how to reproduce > what you are seeing on a device myself. If you have a device with b2g/Gaia installed, you can: cd $GAIA make production (this will reset everything, so don't use your dogfooding device if any) // Revert to the version before the switch from <img> -> background-image git checkout 3ad11b1 APP=communications make install-gaia // Install some contacts + images make reference-workload-light Once the workload is installed, open the Contacts app On the first page of the contacts app you should see some contacts + some images Scroll down until you see a reflow Hope it helps.
Flags: needinfo?(21)
Product: Core → Core Graveyard
Product: Core Graveyard → Core
Depends on: 1395964

Patch in bug 1395964 should have fixed this, as far as I can tell.

Assignee: bzbarsky → emilio
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: