Platform: WinNT server 4.0 UK - SP6a --- Images that are not found usually get a broken image icon. But when the image source is changed by javascript code, and the image is not available, the broken image is replaced by the ALT text. --- Curiously, this text can't be change back to an image by the same javascript code.

Punting from "Architecture" product back to "Browser". Mozilla will be complying with HTML 4.0 for ALT text, see http://www.w3.org/TR/html4/struct/objects.html#adef-alt In particular that means that the ALT text will replace the image when it does not load and no "broken image" icons will appear -- see bug 1994. Ian, is there any limitation imposed by a CSS spec, that prevents, after an image is replaced by its ALT text when it fails to load, a reload of the image replacing the ALT text again, or is this a limitation of Mozilla's implementation or the site's code? Note that mapping the units of meaning made available at this site as words onto specific images is not even theoretically possible...

rickg, who would get this bug? enhancement?

Troy -- this is a feature request. Please disposition as you see fit.

I'm not sure I follow every step here, but it seems reasonable that if you use JS to change the SRC attribute of the image, then we should try and load that image. Even if we have replaced the image with its alternate content because it failed to render That said, we don't do that today, and this seems too low of priority to consider for version 1.0 What will probably happen is that CSS will introduce more flexibility in this area, so the content creator can specify what's displayed at each step of the way: - while the image is loading (today hardcoded by ua to be placeholder icon and ALT text) - once it's loaded (that's what the style rule controls today) - if the image fails to load (today per HTML4 spec) Ian suggested we introduce some Mozilla specific peseudo-element rules, but that was LATER'D for version 1.0 as well

Ok, verifying for LATER. Troy, just out of interest, why is it that we don't pick up changes in the 'src' attribute of broken IMG elements? Is it that the image frame is the one which deals with that and we remove the IMG frame when we put in the alt text's frame? If so, I would guess that we'll have to make the IMG frame and the alt text frame have a common ancestor which knows about the 'src' attribute, right? (I'm just guessing here, I have not even remotely looked at the source...) This is probably an HTML4 compliance issue (albeit a low priority one), so marking dependencies. Also marking css-moz, since Troy thinks CSS extensions may come in useful here. Lowering priority, updating summary, platform, os.

Yes, we throw away the IMG frame and replace it with either a block or inline frame. We could try and have a common parent frame for both, but we've been hesitant to do that in the past because things get thorny. The IMG can be either block-level or inline-level and that makes it complicated. We like frames to be one or the other I think what we'll probably do is make the frame construction code re-create an image frame and try and load the load. I don't think it's an HTML4 compliance issue, because HTML4 has no way to change the SRC attribute. It probably is a DOM compliance issue. If people feel it's something that needs to get fixed (I don't), then we'll have to disable displaying the alternate content (and not be HTML4 compliant) and instead display the broken icon like Nav and IE do

Reopening and moving to Future...

Reassigning to buster, troy no longer with us.

This bug (probably) blocks bug 42499, where an image is replaced by another in a mouseOver handler, but since the new image is not yet loaded we replace it with the alt text and then can never get an image back. If this is indeed the underlying cause of bug 42499, then this will happen a lot on the web.

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

See also: bug 70820 Broken image 'alt' can't be changed through dom. bug 77279 Replacing loading image's src breaks image.

buster *so* isn't going to fix this.

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

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

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

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

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

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

this works with non-strict mode images. We need to add a NS_STYLE_HINT_FRAMECHANGE when 'src' is set for some element.. just not sure which one yet. Marc? any hints?

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+, topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword. Please send any questions or feedback about this to adt@netscape.com. You can search for "Moving bugs not scheduled for a project" to quickly delete this bugmail.

per adt, not critical for nsbeta1. hence minus.

It looks like Gagan didn't change the keyword, so adding nsbeta1- per ADT triage with Gagan.

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

carrying over topembed+ from bug 151245.

Slightly less hacky than my patch in bug 151245. Here I make GetMappedAttributeImpact return NS_STYLE_HINT_FRAMECHANGE if the 'src' attribute changes and we have no image frame. In order to make this possible from GetMappedAttributeImpact, which is declared |const|, I had to spread the const love elsewhere. Fortunately, it turns out that the whole operation really _is_ |const| as far as the content node goes.

This is a more contained fix; instead of propagating |const| I just cast it away.

Fixed a bug in the original patch that caused some weird rendering bugs due to nsTextNode not using the subclass version of IsContentOfType().

taking

Comment on attachment 103267 [details] [diff] [review] alternative patch + else if (aAttribute == nsHTMLAtoms::src) { + // If 'src' changed and we don't have a real image frame, + // we need to cause a reframe. + nsIImageFrame* imageFrame; + // cast away |const| because the underlying interfaces don't use it. + nsHTMLImageElement* self = NS_CONST_CAST(nsHTMLImageElement*, this); + if (self->GetImageFrame(&imageFrame) == NS_ERROR_NO_INTERFACE || + !imageFrame) + aHint = NS_STYLE_HINT_FRAMECHANGE; GetImageFrame() will never return NS_ERROR_NO_INTERFACE so no need to check that, just call it and ignore the return value and check for a null imageFrame. If you want to, you could even make nsHTMLImageElement::GetImageFrame() be a void method since there's no useful information we'll ever want to return from it. Also, please add braces around the (one line) body of the if statement, all other one-line if's in that file uses braces. With that, sr=jst

Comment on attachment 103267 [details] [diff] [review] alternative patch r=dbaron given jst's comments are handled

Comment on attachment 103267 [details] [diff] [review] alternative patch a=roc+moz for trunk

checked in.

Reopening. I backed this out due to a large pageload regression (6.6% on btek; 5.8% on luna). (It was initially a test-backout to see if it was the problem, but since it was I'm leaving it backed out.)

This is the patch that was checked in, since it's been backed out. For the record, I don't see any reason to null-check aImageFrame.

The patch checked in by bryner also caused this regression in Composer: bug 176020 It also caused a broken icon image to appear in the activation dialogs in Netscape builds (I don't know of a bug filed on that issue).

Well, we hit this during parsing, which I wasn't expecting, which is why it impacts pageload times. At that point, we have no image frame, and so it sets NS_STYLE_HINT_FRAMECHANGE. Still trying to figure out whether the slowdown is from setting the framechange hint or just from looking for the frame.

I measured a 7% slowdown on jrgm's pageload test on my machine with _only_ the image frame lookup when the 'src' attribute is set (without setting the framechange hint).

This is from an optimized build, so the line numbers might be a bit wacky, but the function names are correct.

So, we could key off of whether mParent is null in nsHTMLImageElement::GetMappedAttributeImpact to decide that a frame couldn't possibly exist yet. I think that would solve the performance regression with this patch. Another possibility would be to set a flag on the element when the image load fails, then return FRAMECHANGE from GMAI if the flag is set (and clear the flag), but that would create a somewhat fragile dependence on the behavior of the image frame, and increase the size of image elements. What might be more interesting is to avoid calling GetMappedAttributeImpact on every attribute of every node during parsing. It's a near-constant-time implementation on every element (except for image, with my patch), but it is a virtual function call that's happening hundreds of times for a typical page, for no reason. We do use the impact to decide whether the attribute is mapped to style, which determines where it's stored in the attributes collection. Since the vast majority of elements on a page are never changed, it may be possible to delay determining if the attribute has a style impact until the attribute is actually changed, _after_ the initial parse.

Check |mParent| before doing a frame lookup; assume no frame if there is no parent. I also removed the null check as dbaron pointed out I could do, and removed some unneeded null initialization of pointers passed to GetImageFrame (since it always initializes the return value).

Comment on attachment 103796 [details] [diff] [review] quick fix for perf regression r=dbaron

I filed bug 176139 on the general issue of calling GetMappedAttributeImpact during HTML parsing.

Comment on attachment 103796 [details] [diff] [review] quick fix for perf regression sr=jst

Comment on attachment 103796 [details] [diff] [review] quick fix for perf regression Actually, this patch still causes the composer "Insert image" regression on Mac. Investigating.

The composer "insert image" problem happens on Linux as well.

Make sure to return NS_STYLE_HINT_CONTENT for 'src' attribute changes when there _is_ an image frame, like we did before the patch.

Comment on attachment 103924 [details] [diff] [review] new patch to fix composer regression r=dbaron, although it might be consistent with other code in the tree (mostly scc's) to call the variable |mutable_this| instead of |self|.

Comment on attachment 103924 [details] [diff] [review] new patch to fix composer regression sr=jst

Comment on attachment 103924 [details] [diff] [review] new patch to fix composer regression a=dbaron for trunk checkin

checked in.

This caused bug 176926