288064 - We aren't showing the alt text of images that are loading

Status: RESOLVED FIXED

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

Description

[Hixie (not reading bugmail)]

According to the spec we are following for alt text processing:
   http://www.hixie.ch/specs/alttext
...we should be rendering the alt text inline for images that have alt text,
have no height and width attributes, and have not yet loaded.

Currently we don't show anything, which means if such an image is a link, the
link is unclickable until we display the image. Which makes the link permanently
unclickable if the remote site is having DNS issues or similar, since then we'll
be waiting for ages before deciding the image is broken.

I see this on my portal:
   http://☺.damowmow.com/ 
...quite often (the comics at the bottom are often on dodgy servers).

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the problem with doing this is that it'd involve a reframe or _attempted_
reframe when we get OnStopContainer or something.... which means flushing out
frame construction first.  Which means a pageload time hit, possibly a pretty
substantial one (the initial patch for bug 11011 made pageload times 15% higher
for just such a reason).

I suppose we could try to mitigate the impact by only doing the update batch if
MayHaveFrame() is true in OnStopContainer.  Not sure what the perf impact might
be then; maybe worth testing.

There's another issue here.  Should we be showing the alt text till the image
"has loaded" or till it "has started loading"?  I would say the latter, where
"started" means we've at least decoded the image format header and know the
intrinsic image size.  This way large images (eg digital photos) show up
gradually, and progressive jpegs actually work.  Not to mention that some image
formats (JPEG push stuff comes to mind) never actually finish loading.

David, biesi, what do you think?

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

well, I don't think you can avoid the reframe... I'd expect that in most cases a
page will have more than two images from the same server, and necko only does 2
persistent connections per server... which means it's likely that you need the
alt text frame, which means you have to do that frame reconstruction.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm...  That's true...  I guess with the changes I want to make in bug 309986 at
least the content state change itself won't flush the sink.  The restyle will
have to, but with any luck it'll be sorta batched with other things (if nothing
else, the restyle won't happen till we go back out to the event loop).  So it
might not cause the sort of impact the initial bug 11011 patch did...

In any case, I might try doing this after I land bug 309986 and see how Tp deals.

Comment 4

[Hixie (not reading bugmail)]

Why do we not require a reframe today? Are we inserting a 0x0 replaced inline
frame (or whatever Gecko calls it) as we wait for image data?

Is there no way to make replacing an inline frame with a replaced element frame
more efficient? :-)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Why do we not require a reframe today? Are we inserting a 0x0 replaced inline
> frame (or whatever Gecko calls it) as we wait for image data?

The layout object is an nsImageFrame while we wait for image data.  It typically
draws a little placeholder icon if it actually has to paint before the image's
size is known (which is pretty rare, what with paint suppression; you can get
this on long pages with lots of images like some specs, though).

> Is there no way to make replacing an inline frame with a replaced element
> frame more efficient? :-)

The problem is not the replacement itself.  The problem is that said replacement
must happen when the DOM and frame model are in a consistent state.  During most
of the load the DOM and frame model are NOT in such a state, because frame
construction works much faster in chunks; the bigger the chunks the smaller the
total time taken.  This needs to be balanced against incremental rendering, of
course.  So the frame model will typically lack behind the DOM in terms of being
constructed, and we will every so often build whole pieces of it.  If we need to
reconstruct a frame, we first need to bring the frame model up to date, and only
then mess with it.  If this happens a lot, we're basically just constructing the
frame model in smaller chunks, which is slower.  This is where the performance
hit really comes from.

All that said, with bug 309986 fixed we at least won't flush out the content
sink's notifications on the state change itself.  We'll still do it when the
restyle event fires, but that should hopefully batch together multiple images
and such... More importantly, we already have built-in flushes when we reflow,
so as long as the reframes happen at points where we'd normally have seen
reflows anyway (that is, off events) we might be ok perf-wise.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Some testcases:

Quirks, no size: data:text/html,<img
src="http://landfill.mozilla.org/ryl/slowResponse3.cgi?time=20" alt="Real image
in 20 seconds">

Quirks, size: data:text/html,<img
src="http://landfill.mozilla.org/ryl/slowResponse3.cgi?time=20" alt="Real image
in 20 seconds" width="100" height="200">

Standards, no size: data:text/html,<!DOCTYPE HTML PUBLIC "" []><img
src="http://landfill.mozilla.org/ryl/slowResponse3.cgi?time=20" alt="Real image
in 20 seconds">

Standards, size: data:text/html,<!DOCTYPE HTML PUBLIC "" []><img
src="http://landfill.mozilla.org/ryl/slowResponse3.cgi?time=20" alt="Real image
in 20 seconds" width="100" height="200">

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This implements the following:

1)  When deciding whether to show a sized box for an image, if it's still
loading do so only if its size is "specified" (which actually means its size is
set and not set to percentages) or if we would show a sized box if it were
broken.
2)  Once enough image data comes in that we know the image size, reframe.
3)  Matching on ::-moz-loading for the state involved (that is, prior to us
getting enough data from the server to know what the image size is).

This seems to pass all four of the testcases fine; I'd have to try it on
tinderbox to see the perf impact.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

might make sense to make nsObjectLoadingContent return this new state for
eType_Loading, even though nothing uses it yet.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sure, I can do that.

Comment 10

[Hixie (not reading bugmail)]

(btw bz the simplest strict mode DOCTYPE is just "<!DOCTYPE HTML>".)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 197508 [details] [diff] [review]
Proposed patch

r=biesi with that change

>size is "specified" (which actually means its size is
>set and not set to percentages)

that doesn't match my reading of HaveFixedSize, btw.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  I'll fix that comment.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I tried checking this in and backed it out again -- it caused a 3% regression in
Tp overall and on some of our test pages it was a 50% regression.

I'll try to find some time to look into why these last were so hard-hit.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, so I've found two problems:

1)  One of the test pages has markup like:

<font>
lots of stuff including blocks
<img>
</font>

so reframing the image was very expensive because of bug 311615.  Enough so that
pageload on that page went up about 50% and that made the overall Tp number go up.

2)  The end of a view update batch (eg the sort that happens after the restyle
event is processed) flushes out reflow if there are any pending invalidates on
the viewmanager.  This seems to be happening just as much without this patch,
though, so perhaps it's not an issue.  So once bug 311615 is fixed I'll try
relanding this.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in again.  This time, the Tp hit was less than 0.5%, if any, which I
think is ok...

Comment 17

[Samuel Sieb]

Could this be what's breaking smileys on chatzilla?  The first time a smiley is used, it either doesn't show up or blinks and disappears.  The next time it's used, it shows up fine, probably because it's already in cache.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> Could this be what's breaking smileys on chatzilla?  The first time a smiley is
> used, it either doesn't show up or blinks and disappears.  The next time it's
> used, it shows up fine, probably because it's already in cache.

That sounds like bug 309141.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Samuel, that's possible, yes.  Is there a bug on it?  I can't test right now because chatzilla is completely busted by the js component fastload, but once that's fixed I can look...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Samuel, this was in fact causing the smily problem.  I've filed bug 313656 on it.

Comment 21

[Martijn Wargers (dead)]

Fwiw, this is now causing a wide page with scrollbars while the page is loading at:
http://www.lidl.de/de/home.nsf/pages/c.o.pc.index