468263 - [FIX]Setting src attribute of <img> element to an empty value doesn't hide the image

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Adam Kowalczyk]

Attachment: testcase

In Firefox 3.0 setting src to an empty string used to hide the image, but in 3.1 it leaves the image displayed.

Interestingly, removing the src attribute doesn't hide the image in either 3.0 or 3.1.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081206 Shiretoko/3.1b3pre

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Regression range would be great.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Maybe regression from Bug 444931?

Comment 3

[Adam Kowalczyk]

Works:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080703 Minefield/3.1a2pre

Fails:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/20080808031528 Minefield/3.1a2pre

Bug 444931 is indeed in that range.

Comment 4

[Patrick McManus [:mcmanus]]

I don't think 3.0 was ever hiding the image per se, when it was changed to "" it was fetching the base uri for it and generating an error when that came back with the wrong content type .. which I guess resulted in the behavior you were looking for, but I suspect you would have gotten the same effect setting it to any HTML based url.

We are essentially treating src="" as a non-existant attribute, so it makes sense to me that they do the same thing and I agree the "same thing" ought to be to remove the image from display.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Removing the attribute shouldn't necessarily hide the image.   We have some XXX comments about the fact that it doesn't, but what it should do is whatever IE does, in my opinion.

For that matter, the behavior of src="" should also match IE.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I do think we need to block on sorting out the desired behavior, by the way.

Comment 7

[Patrick McManus [:mcmanus]]

I am windows-less at the moment, so I cannot test IE here when src is changed to "" or removed. Maybe Adam knows or could test?

But I do know that when it is set to "" originally IE does completely the wrong thing - if it were loaded from /dir/page.html it would try and GET "/dir/", which isn't justifiable or even really useful :)

Comment 8

[Adam Kowalczyk]

When src is dynamically removed, IE7 says the URL is "Not available" and presumably it doesn't attempt to load the image. It shows the missing image icon instead.

When src is changed to an empty value, the behaviour is consistent with what Patrick described.

Comment 9

[Boris Zbarsky [:bzbarsky]]

One last question.  Is there a difference in IE's behavior if you use .src= instead of setAttribute?  I would assume no, but just checking...

It does sound like we want sets to "" and unsets to just drop the image; that's not that difficult to do.

Comment 10

[Adam Kowalczyk]

(In reply to comment #9)
> One last question.  Is there a difference in IE's behavior if you use .src=
> instead of setAttribute?

No.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, say

If we want to play it safe on 1.9.1 we could take just the changes to nsImageLoadingContent, I guess.

Comment 12

[Robert Longson [:longsonr]]

Does nsSVGImageElement need any changes to stay in sync?

Comment 13

[Boris Zbarsky [:bzbarsky]]

No.  It already reacts to the xlink:href being removed.  See bug 455226.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/bfcbbaa6d2c7

Jonas, thoughts on how much of this to take on branch?  I'm still tempted to do only the nsImageLoadingContent changes (so fix src="" but not removeAttribute).

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352060 [details] [diff] [review]
Like so, say

Jonas: is your SR on this implicitly saying we should take it all on branch? Please renominate for branch approval if so. For now, I'm leaving this request since it's unclear if you've answered bz's question.

Comment 16

[Boris Zbarsky [:bzbarsky]]

ccing sicking so he'll actually see comment 14 and comment 15.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Did removing the attr not use to work? If so leaving of out of the branch port might be a good idea. But I do think we should take any regressions that FF 3 caused on branch

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Did removing the attr not use to work? 

That's correct.

> But I do think we should take any regressions that FF 3 caused on branch

We're talking about the 1.9.1 branch here, not the 1.9.0 branch.  There's no issue on the 1.9.0 branch.  On the 1.9.1 branch, we currently have a regression as described in the summary of this bug.

So the real question here is whether to take the attr-removal fix for 1.9.1 in addition to the regression fix, or whether to just wait for 1.9.2 on it.  At this point, I'm leaning towards the latter.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh. I think it's fine to take the whole patch. The regression risk does not seem much smaller if we leave the attr removal part out

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352060 [details] [diff] [review]
Like so, say

a191 = beltzner

let's do this before b3, then

Comment 21

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/231a3777dae6

Comment 22

[Boris Zbarsky [:bzbarsky]]

Also pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/190bfad9f3cf to fix the 1.9.1 test bustage.