22820 - Broken IMG 'src' cannot be changed to valid 'src' dynamically

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[cjacob]

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.

Comment 1

[Sean Richardson]

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

Comment 2

[leger]

rickg, who would get this bug?  enhancement?

Comment 3

[rickg]

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

Comment 4

[troy]

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

Comment 5

[Hixie (not reading bugmail)]

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.

Comment 6

[troy]

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

Comment 7

[Hixie (not reading bugmail)]

Reopening and moving to Future...

Comment 8

[Hixie (not reading bugmail)]

Reassigning to buster, troy no longer with us.

Comment 9

[Hixie (not reading bugmail)]

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.

Comment 10

[Fabian Guisset]

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

Comment 11

[Jesse Ruderman]

Attachment: testcase

Comment 12

[Jesse Ruderman]

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

Comment 13

[Stuart Parmenter]

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

Comment 14

[Jesse Ruderman]

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

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

[Christopher Hoess (gone)]

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

Comment 17

[Hungerburg]

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

Comment 18

[Martin Honnen]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Stuart Parmenter]

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?

Comment 21

[scottputterman]

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 [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 22

[Gagan]

per adt, not critical for nsbeta1. hence minus.

Comment 23

[scottputterman]

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

Comment 24

[Brian Ryner (not reading)]

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

Comment 25

[Brian Ryner (not reading)]

carrying over topembed+ from bug 151245.

Comment 26

[Brian Ryner (not reading)]

Attachment: patch to use NS_STYLE_HINT_FRAMECHANGE

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.

Comment 27

[Brian Ryner (not reading)]

Attachment: alternative patch

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

Comment 28

[Brian Ryner (not reading)]

Attachment: revised version of original patch

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

Comment 29

[Brian Ryner (not reading)]

taking

Comment 30

[Johnny Stenback (:jst)]

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 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 103267 [details] [diff] [review]
alternative patch

r=dbaron given jst's comments are handled

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 103267 [details] [diff] [review]
alternative patch

a=roc+moz for trunk

Comment 33

[Brian Ryner (not reading)]

checked in.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that was checked in

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.

Comment 36

[Kathleen :Brade]

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

Comment 37

[Brian Ryner (not reading)]

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.

Comment 38

[Brian Ryner (not reading)]

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

Comment 39

[Brian Ryner (not reading)]

Attachment: stack of GetMappedAttributeImpact being called during parsing

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

Comment 40

[Brian Ryner (not reading)]

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.

Comment 41

[Brian Ryner (not reading)]

Attachment: quick fix for perf regression

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 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

r=dbaron

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 44

[Johnny Stenback (:jst)]

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

sr=jst

Comment 45

[Brian Ryner (not reading)]

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.

Comment 46

[Brian Ryner (not reading)]

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

Comment 47

[Brian Ryner (not reading)]

Attachment: new patch to fix composer regression

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 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 49

[Johnny Stenback (:jst)]

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

sr=jst

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

a=dbaron for trunk checkin

Comment 51

[Brian Ryner (not reading)]

checked in.

Comment 52

[Boris Zbarsky [:bzbarsky]]

This caused bug 176926