70820 - ALT in broken image cannot be changed by the DOM

Status: RESOLVED FIXED

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

Description

[Ronald Buckman]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; 0.9) Gecko/20010302
BuildID:    2001030205

When I click the Change button the ALT fails to change to the
message I placed in the text field.  Here is the function that is 
supposed to change the ALT.

function alt1(){document.IMG1.alt = document.FORM.form.value}

Here is the image.

<img SRC="im.gif" name="IMG1" WIDTH="300" HEIGHT="100" ALT="YouCanChangeThisText">

Here is the Change button.

<input TYPE="button" VALUE="Change" onClick="alt1()">






Reproducible: Always
Steps to Reproduce:
1.Point Mozilla to
http://www.bekkoame.ne.jp/~hamba/mozilla/HTMLImageElement/alt.html
2. Click the change button.


Actual Results:  The ALT remains the same.

Expected Results:  The ALT should change to the text in the text field.

This DOM script works fine on IE 5.5 .

Comment 1

[Brian Haskin (Janzert)]

First, document.IMG1.alt = foo isn't valid. You need to either use
document.getElementByID or document.getElementsByName. That said, after changing
the above example it still doesn't work so I'm confirming this with
user agent: Mozilla/5.0 (Windows; U; Win98; en-US; 0.9) Gecko/20010302
build: 2001030205

I have a feeling this is another symptom of the problem talked about in bug 22820.

Comment 2

[Brian Haskin (Janzert)]

Attachment: Here is a corrected and slightly simplified testcase

Comment 3

[Prashant Desale]

QA contact Update

Comment 4

[Johnny Stenback (:jst)]

Future for now.

Comment 5

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 6

[Sivakiran Tummala]

{
       var altVal = document.getElementById("text").value;
        document.getElementById("image").alt = altVal;
	alert(document.getElementById("image").alt);
}
alerts the new value but does not show on the image 

Comment 7

[Harshal Pradhan]

This does not look like a DOM problem. Layout is being provided the 
attributechanged notification. It just does not do anything with it.

Switching to another window and then back will show the new (changed) text. 
Layout simply needs to repaint when the alt text changes.

Attaching patch in a minute.

Comment 8

[Harshal Pradhan]

Attachment: one-liner that seems to fix the problem

Simply forces reflow when the alt attribute changes.
This makes the correct thing happen with the test case above, and does not do
anything funny when the image actually loaded correctly. But I'm still
wondering if we should check if the image failed to load before doing this.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 73851 [details] [diff] [review]
one-liner that seems to fix the problem

From what I can tell, this does the right thing, and I'm willing to sr it.
Someone who understands layout better should have a look at this one-liner and
review though. attinasi, dbaron?

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 73851 [details] [diff] [review]
one-liner that seems to fix the problem

From what I can tell, this does the right thing, and I'm willing to sr it.
Someone who understands layout better should have a look at this one-liner and
review though. attinasi, dbaron?

sr=jst

Comment 11

[Johnny Stenback (:jst)]

Errr, I hate it when that happens...

Comment 12

[Sivakiran Tummala]

is this a dom problem, comment 6

Comment 13

[Fabian Guisset]

attinasi, could you please take a look at the one-liner that seems to fix this
bug? thanks in advance :-)

Comment 14

[Harshal Pradhan]

Just a reminder that this one liner is *still* waiting for a review.
Does anyone have any suggestions about who in order to get someone to look at this?

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 73851 [details] [diff] [review]
one-liner that seems to fix the problem

I am inclined to say that we should check that the image request failed here. 
It won't cause anything bad to happen because it will just reflow things that
don't need it, but that is unnecessary.  Harshal, if you want a review, you
need to send email to people directly instead of asking in bugs....

Comment 16

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

Comment on attachment 73851 [details] [diff] [review]
one-liner that seems to fix the problem

I'm surprised that only a reflow fixes it (and no changes to the frame tree are
required).  Since testing shows that seems to be the case, r=dbaron.  I'm not
worried about the performance if the image exists because this seems rare
enough that there's no need to optimize.

Comment 17

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

well, I stumbled over this rather old patch today... I checked it in, hope
that's ok with you all (after I verified that the bug still occurs and that the
patch fixes it). I had to apply the patch by hand because it didn't apply.

Checking in mozilla/layout/html/base/src/nsImageFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v  <--  nsImageFrame.cpp
new revision: 1.274; previous revision: 1.273
done

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sorry, this is not fixed quite yet.  It's fixed in quirks mode.  It's not fixed
in standards mode; as dbaron was guessing, changes to alt text in standards mode
require a reframe if the image is not showing.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Standards testcase (bogus doctype and all)

Comment 20

[Boris Zbarsky [:bzbarsky]]

One way to fix this would be to use:

  img:-moz-broken-img::before { content: attr(alt) }

once we support the -moz-broken-img pseudo-class...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed in standards mode by bug 11011