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

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: img_vs_background_image.html

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: img_vs_background_image.html

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

Vivien, this seems to fix the testcase for me.  Does it help with the original issue?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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 "".

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: alternate_src_empty_with_image.html

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 ?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't force a frame reconstruct when our image state changes if we already have an nsImageFrame and know we'll have one in the new state too.

How does this perform on the original use case?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just tried on the device and I still see the reflows. Will try a new test case on monday :/

Comment 13

[Timothy Nikkel (:tnikkel)]

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.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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