Closed Bug 468263 Opened 16 years ago Closed 16 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: ancestor.ak, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.9.1, regression)

Attachments

(2 files)

Attached file 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
Flags: blocking1.9.1?
Regression range would be great.
Maybe regression from Bug 444931?
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.
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.
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.
I do think we need to block on sorting out the desired behavior, by the way.
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 :)
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.
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.
(In reply to comment #9)
> One last question.  Is there a difference in IE's behavior if you use .src=
> instead of setAttribute?

No.
Attached patch Like so, saySplinter Review
If we want to play it safe on 1.9.1 we could take just the changes to nsImageLoadingContent, I guess.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #352060 - Flags: superreview?(jonas)
Attachment #352060 - Flags: review?(jonas)
Summary: Setting src attribute of <img> element to an empty value doesn't hide the image → [FIX]Setting src attribute of <img> element to an empty value doesn't hide the image
Does nsSVGImageElement need any changes to stay in sync?
No.  It already reacts to the xlink:href being removed.  See bug 455226.
Attachment #352060 - Flags: superreview?(jonas)
Attachment #352060 - Flags: superreview+
Attachment #352060 - Flags: review?(jonas)
Attachment #352060 - Flags: review+
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).
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #352060 - Flags: approval1.9.1?
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.
ccing sicking so he'll actually see comment 14 and comment 15.
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
> 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.
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
Attachment #352060 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 352060 [details] [diff] [review]
Like so, say

a191 = beltzner

let's do this before b3, then
Also pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/190bfad9f3cf to fix the 1.9.1 test bustage.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Depends on: 726919
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: