109418 - loading image icon leaving garbage in layout

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[tor]

If the final image has a dimension smaller than that of the loading-image.gif
standin, the "outside" bits are never repainted when the image finishes loading.
Attached is an example of the problem on www.apple.com - look for the bits below
the top link bar.  www.mozillazine.org also shows similar garbage around the
thumbs up/down icons for the builds.

Comment 1

[tor]

Attachment: www.apple.com showing loading-image garbage

Comment 2

[Tony Garnock-Jones]

Attachment: Mozillazine picture

Here's a screenshot (that I've scribbled on in red) showing the problem on
MozillaZine. I'm on Linux, using build 2001110815.

Comment 3

[Christopher Blizzard (:blizzard)]

Has this been fixed?  Marc checked in a fixed image.

Comment 4

[tor]

Attachment: geocrawler image showing lingering garbage

Unfortunately it's still there on a tip build (built after tree closure on
11/14).
See the "register" and "login" buttons on the geocrawler snapshot.

Comment 5

[Christopher Blizzard (:blizzard)]

So maybe it wasn't the image?

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: testcase for broken image icon

Same problem occurs whith the broken-image icon. See the attached testcase.

Comment 7

[Jeffrey Baker]

This was in 0.9.6 and 0.9.7 and it hoses up the presentation of lots of sites. 
I'm seeing remnants of loading-image.gif all over the page at sites like
cnn.com, etrade.com, and so forth.

Comment 8

[Jeffrey Baker]

*** Bug 114768 has been marked as a duplicate of this bug. ***

Comment 9

[Jeffrey Baker]

I wrote a tiny testcase to see if the problem was a generic problem of not
repainting when an image is replaced by a smaller image.  There was no problem.
 So the issue seems to be a special case of loading-image.gif.

Comment 10

[Frederic Crozat]

After checking nsImageFrame::DisplayAltFeedback and nsImageFrame::Paint, 
I think I've found why the problem appears :

when image loading is complete, first drawing of complete image doesn't
invalidate  loading icon rectangle.. It is usually not needed since loaded image
will hide the loading icon, EXCEPT when image.y+image.height < loading_icon.y +
loading_icon.height or when image.x + image.width < loading_icon.x +
loading_icon.width.. 

And of course, this garbage disappears when page is displayed again..

There is two alternatives to fix this garbage :
-always call invalidateIcon when loading is complete, before displaying image
(much simple but maybe a little overkill)
-invalidate the "garbage" rectangle (much more complex to compute)..

I'll try to make a patch using the simple solution tomorrow

Comment 11

[Jeremy M. Dolan]

> EXCEPT when image.y+image.height < loading_icon.y + loading_icon.height or when

Then how about we don't draw the loading icon in the first place in those cases?

Comment 12

[Frederic Crozat]

> Then how about we don't draw the loading icon in the first place in those cases?

Since we are loading image, we don't know yet its size (except if it is
specified in the HTML code). It will be available only when loading is complete
(ie after loading-icon has been drawn...)

Comment 13

[Chris Abbey]

The icon is being rendered in nsImageFrame.cpp at line 1143, the rendering
context used here has a clipRect set (line 1123) to the size of the frame, in
the attached testcase this is 15x15px. Why then is the icon not clipped to this
size?

Comment 14

[Chris Abbey]

With 47 if the image size is not specified then you get the full icon, if it is
and it's too small, such as this, you don't get any icon at all. Today in the
case that size is not given in the html, it appears as if the problem is solved
thanks to the reflow that is triggered when the image finally loads and it's
size is set. 

If so, then Jeremy's suggestion could handle the case that the html contains
size and behave much like 47 does and the reflow could handle the case that it
doesn't.

(but I still think the icon should be clipped.)

Comment 15

[Jeremy M. Dolan]

> Since we are loading image, we don't know yet its size

There are cases when we do have size, but draw an icon anyway. news.bbc.co.uk,
"IN DEPTH" bullet graphic: (on the right side, 600 or so pixels down):

<img SRC="/sport/furniture/in_depth.gif" WIDTH=15 HEIGHT=15 ALT="In Depth"
ALIGN=LEFT HSPACE=4>

15x15 won't possibly fit the image icon, but it's drawn, and the part that
sticks out is bigger then the image is to begin with.

If we don't know the size, are we even drawing a box anyway? That doesn't seem
right.

Comment 16

[Marc Attinasi]

Accepting, assigning 0.9.8 milestone.

Comment 17

[Marc Attinasi]

The icon is suppossed to be clipped. It *is* clipped on Mac and Windows, but not
on Linux for some reason (!)  - the code in nsImageFrame::DisplayAltFeedback
sets the clip rect, but on Linux it seems to be ignored for the icon, though not
for the ALT text.  Maybe the RenderingContext impl for Linux does not support
clip for image drawing?

I think it might be cleaner to just skip the display of the icons if the size is
less than that required to display the icon
(ICON_SIZE+(2*ICON_PADDING)+(2*ALT_BORDER_WIDTH) - but first I'll see why
clipping does not work on the Linux RenderingContext

Comment 18

[Marc Attinasi]

bug 78497 covers the problem with images not being clipped in Linux.

I have a patch for doing the clipping in the image frame instead of relying on
the rendering context to clip, because of the bug, and because it is easy. 
Also, I have a fix for the invalidation problem (the less elegant solution that
Frederic suggested - IMO invalidating the 'garbage' region is an unnecessary
optimization).

Comment 19

[Marc Attinasi]

Attachment: PATCH: invalidate the icon area when image loads, and clip the icons when image area is too small to work around Linux bug

Comment 20

[Marc Attinasi]

I have attached a patch that should clear up these garbage pixels.

First, it invalidates the icon area when the image has successfully loaded.  It
was always doing this when transitioning from the loading icon to the broken
icon, I just forgot to do it for the actual image.  Also, I expanded the area
that is invalidated to contain the entire border, padding and icon instead of
just the icon.

Second, I put in a manual clipping to skip drawing the icons if the size of the
image is too small to contain it.  This works around a bug in Linux's rendering
context where images are not clipped (it also skips rendering of the ALT text
since the are is obviously too small to show any interesting text).

I tried it on Mac, Windows and Linux and it seems to work for me.  If anyone
else can try it and maybe review it i'd appreciate it.  I have been asked to get
this in for 0.9.8, so time is of the essence.

Comment 21

[Chris Abbey]

Marc's patch works great here, and to my eyes looks good (not sure what, if
anything, that counts for ;) except for the added call to |invalidateIcon| at
line 612. I can't envision a case where it matters. (assuming the clip either
functions correctly, or is bypassed as in his patch.)

If the image is already loaded then
   this isn't an issue at all.
else if the image is not yet loaded then
   if we know the size (from the image tag) then
      if the icon is smaller or the same size as the image box it
        will be overprinted when image loads (no invalidate needed).
      else the icon is larger than the image box so it will not
       be rendered at all (again, no invalidate needed).
   else we don't know the size yet so we don't render it yet and
      if the load fails, render the broken icon (no invalidate)
      else render the image (once again, no invalidate needed)


Since invalidate could be a costly operation is there really a good
reason to do it here? 

Comment 22

[Chris Abbey]

In an attempt to validate my theory, I've been running a build with the
invalidateIcon call and a build without it side by side for a couple hours... I
can't get it to make any difference.

Comment 23

[Frederic Crozat]

I can confirm Chris "theory" : the InvalidateIcon call is not longer needed..

Comment 24

[Marc Attinasi]

I think you guys are right.  I was thinking of the case where the loading image
icon is not drawn, but the box around it is, and then the image comes in and is
transparent - but the image itself will invalidate the entire area of the box,
so, as you said, the extra invalidate is unnecessary.  I'll remove it.

Anyone wanna r= the patch, assuming the InvalidateIcon call is removed? I'm
going to be out for 2 weeks after today so if I don't get this in today, someone
else will have to shepard the patch to the tree for me next week...

Comment 25

[Christopher Blizzard (:blizzard)]

Upload a new patch and I'll review.

Comment 26

[Marc Attinasi]

Attachment: New Patch: clips the icon if the image size is too small (since Linux has a bug where the RenderingContext will not clip image drawing)

Makes previous patch obsolete: this one does NOT change the way the icons are
invalidated.

Comment 27

[Christopher Blizzard (:blizzard)]

Comment on attachment 65655 [details] [diff] [review]
New Patch: clips the icon if the image size is too small (since Linux has a bug where the RenderingContext will not clip image drawing)

r/sr=blizzard

Comment 28

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.8

Comment 29

[Stuart Parmenter]

Comment on attachment 65655 [details] [diff] [review]
New Patch: clips the icon if the image size is too small (since Linux has a bug where the RenderingContext will not clip image drawing)

r=pavlov

Comment 30

[Marc Attinasi]

Fix checked in.  nsImageFrame.cpp revision: 1.223

Comment 31

[R.K.Aa.]

*** Bug 113584 has been marked as a duplicate of this bug. ***

Comment 32

[R.K.Aa.]

*** Bug 117340 has been marked as a duplicate of this bug. ***

Comment 33

[R.K.Aa.]

I am not seeing this bug on Linux anymore. Verifying fixed.

Comment 34

[André Dahlqvist]

I haven't seen this in ages either. Really verify:-)