Closed Bug 75185 Opened 24 years ago Closed 23 years ago

IMG alt text should go inline when image cannot be displayed

Categories

(Core :: Graphics: ImageLib, defect, P2)

Product:
Core
Component:
Graphics: ImageLib
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9

People

(Reporter: ian, Assigned: pavlov)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: regression, testcase, Whiteboard: [imglib])

Attachments

(6 files)

patch to fix bug (has some extra stuff in it... ignore :))
6.89 KB, patch
Details | Diff | Splinter Review
cleaner patch
3.46 KB, patch
Details | Diff | Splinter Review
newer patch.
12.94 KB, patch
Details | Diff | Splinter Review
block test output (gzip'd text file)
40.94 KB, application/x-gzip
Details
newest patch (what i ran the regression tests against)
13.29 KB, patch
Details | Diff | Splinter Review
newer patch
16.35 KB, patch
Details | Diff | Splinter Review
[This is the reopened version of bug 1994.] The alt text of an IMG element is alternative text, that is, text that should completely replace the image, if the image cannot be shown. In other words, if the image could not be drawn, then it should not be replaced by a box containing the alt text, instead, the alt text should be drawn inline. The quoted uri has more on this, including some tests. Lynx does this correctly. We used to do this correctly. libpr0n broke us.
Blocks: 5764, html4.01, 8131, 8979, moz-broken, 15545, 23691, 24125, 24126, 24778, 24815, 26282, 31451, 34981
Depends on: 5886, 6052
Keywords: mozilla1.0, nsbeta1, regression, testcase
QA Contact: tpreston → ian
Attached patch cleaner patch — — Splinter Review
Status: NEW → ASSIGNED
Keywords: patch
Priority: -- → P2
Target Milestone: --- → mozilla0.9
looking for reviews for this patch
+ } else if (!mSizeConstrained) { + // see if the image is ready in this notification as well, and if the size is not needed + // check if we have a constrained size: if so, no need to reflow since we cannot change our size + if (mParent) { + mState |= NS_FRAME_IS_DIRTY; + mParent->ReflowDirtyChild(presShell, (nsIFrame*) this); + } else { + NS_ASSERTION(0, "No parent to pass the reflow request up to."); + } + } The comments don't make sense -- the second one-line comment relaly applies before the if (!mSizeConstrained). Maybe split things up like so: + } else { + // see if the image is ready in this notification as well, and if the size is not needed + // check if we have a constrained size: if so, no need to reflow since we cannot change our size + if (!mSizeConstrained) { + etc. Rather than asserting 0, do this: + NS_ASSERTION(mParent, "No parent to pass the reflow request up to."); + if (mParent) { + mState |= NS_FRAME_IS_DIRTY; + mParent->ReflowDirtyChild(presShell, (nsIFrame*) this); + } The indentation of that mParent->ReflowDirtyChild(...) line was off, too. The changes look ok to me, FWIW. I'll sr= ASAP given a real layout peer r=. /be
Whiteboard: [imglib]
attinasi is probably the best guy to look at nsImageFrame changes.
OK, checking it out now.
With Brendan's changes the code looks fine, but I tested it and it does not seem to work for me. On the URL it looks like we are still treating some of the images as blocks, and using a hacked-up test I made I don't see any alt text at all (ns6 shows it inline). The hacked-up test is: <html> <body> <img src="http://www.attinasi.org/what.jpg" alt="this is the alt text"> </body> </html> Is there a test that this makes work that I should check?
did you try the url in the bug? It might be related to you needing some other changes I have in my tree.. I will try and work it out to figure out if that is the case.
Yes, I tried the URL. There are some sections in there where each line is suppossed to look the same, and they do not. Even the first 'quick reference' test at the top of the page at the URL looks like it has two blocks with the ALT text, and they are suppossed to be two inlines. NS6.01 renders the tests at the URL correctly.
hhmmmm... some of them don't go inline for me.. i think they are <input type=image>.. might be something else i have to do for those. most of them should work though.
Attached patch newer patch. — — Splinter Review
Cc'ing kmcclusk, hoping to get to the bottom of the mystery raised by this XXX comment: + /* XXX Why do we subtract 1 here? The rect is (for example): (0, 0, 600, 1).. + Why do we have to make y -1? + */ + // The y coordinate of aRect is passed as a scanline where the first scanline is given // a value of 1. We need to convert this to the nsFrames coordinate space by subtracting // 1. /be
The fact that we used to use a 1x1 image for images with no intrinsic size information, and now we're using 0x0 scares me a little bit. I don't *think* it should be a problem, but... + } else { + // see if the image is ready in this notification as well, and if the size is not needed + // check if we have a constrained size: if so, no need to reflow since we cannot change our size + if (!mSizeConstrained) { + NS_ASSERTION(mParent, "No parent to pass the reflow request up to."); + if (mParent) { + mState |= NS_FRAME_IS_DIRTY; + mParent->ReflowDirtyChild(presShell, (nsIFrame*) this); + } + } + } In OnStopDecode(), why do you need to re-dirty the image? Won't you have done that when you got the image bits? + nsRect r(*aDirtyRect); + float p2t; aPresContext->GetPixelsToTwips(&p2t); - nsRect r(*aDirtyRect); - r *= p2t; // convert to twips + r.x = NSIntPixelsToTwips(r.x, p2t); + r.y = NSIntPixelsToTwips(r.y, p2t); + r.width = NSIntPixelsToTwips(r.width, p2t); + r.height = NSIntPixelsToTwips(r.height, p2t); Is |operator*=()| broken? Or is it just too much C++ fanciness? (I don't really care, just curious.) r=waterson
Of course, as with any layout change, you must run the layout regression tests before you check this in, biyotch. ;-)
i think they used to be 0x0.. i was doing something stupid somewhere with 0x0, but it works ok now. (i probably had a div by 0 somewhere before that i since fixed) we shouldn't need the additional reflow.. i'll try removing it when my build finishes. operator*=() works fine.. just too c++ish for me.. i just wanted to do what is done everywhere else in layout "just to be safe"
Ok. I'll be excited to hear how those layout regression tests come out.
ok, i posted the block layout regression test output. i think it shows that a lot of things are smaller by 1px (off by 15 twips), which would make sense since images that fail to load with this patch without alt text have a 0x0 frame instead of a 1x1 frame. also, images that fail to load now with alt text will turn into text nodes and be inserted. i don't see anything that looks really bad, but i don't really know what the output data is telling me. comments would be welcome.
the only test that might have "bad" behavior is 55874. the images has a width and height specified, but since the "url" fails, and there is no alt text, we do the CantRenderReplace on it and it becomes a weirdly sized frame with the border around it.
Attached patch newer patch — — Splinter Review
added back support for internal-gopher images ;) to fix one of the regression tests
Patch 30910 looks good, although I do not understand the gopher stuff - [s]r=attinasi
There already, just moved, but doesn't: + if (mListener) + NS_REINTERPRET_CAST(nsImageListener*, mListener.get())->SetFrame(nsnull); // set the frame to null so we don't send messages to a dead object. suggest that you should add a SetFrame or similar method to imgDecoderObserver (maybe when you rename that to imgDecoderListener :-). I bet USE_IMG2 is rusty. Why not combine ifdefs here? The comment doesn't usefully apply to any #else part or old imglib code. +#ifndef USE_IMG2 nsCOMPtr<nsIURI> baseURL; GetBaseURI(getter_AddRefs(baseURL)); +#endif + // Set the image loader's source URL and base URL #ifdef USE_IMG2 How about using mListener = do_QueryInterface(listener); here: + listener->QueryInterface(NS_GET_IID(imgIDecoderObserver), getter_AddRefs(mListener)); Nit: use NS_STATIC_CAST here: + mParent->ReflowDirtyChild(presShell, (nsIFrame*) this); Uber-nit: How about a space after // ? My eyes hurt. + //After these DOM events are fired its possible that this frame may be deleted. As a result + //the code should not attempt to access any of the frames internal data after this point. Ok, enough nits. I'm actually ok with the code, except how do we get these XXX comments' questions answered, soon? + // XXX do we need to make sure that the reflow from the OnStartContainer has been // processed before we start calling invalidate if (!aRect) return NS_ERROR_NULL_POINTER; - nsRect r(aRect->x, aRect->y, aRect->width, aRect->height); + /* XXX Why do we subtract 1 here? The rect is (for example): (0, 0, 600, 1).. + Why do we have to make y -1? + */ Kevin, anyone? /be
USE_IMG2 isn't rusty. it still works. SetFrame on imgDecoderObserver doesn't make any sense do_QueryInterface on a non-interface pointer doesn't work. fixed yer comment ubernits
Pav reminded me that I meant CallQueryInterface, not do_QueryInterface. r/sr=brendan@mozilla.org, but please police those XXX comments, get questions answered. /be
fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
*** Bug 75821 has been marked as a duplicate of this bug. ***
I suggest marking this bug to INVALID ASAP !!! Not only is this bug immensly stupid (since it suggests compleatly breaking the layout of millions of excisting pages), but it will also in the future force the use of Objects (which has NO requirement of an alt attribute) or <div style="background: url();"> for the placement of images on pages, since it causes a full page reflow for every image that fails to load when height and width is specified. Also, where will that put us as far as giving support to people with disabilities I ask you. Right back to square one. But what is even more important is that this has absolutely no grounds in the specs. Nowhere in the HTML 4.01 SPEC does it say that alt="" should REPLACE the ENTITY <IMG>! What it sais is (§13.8) "Several non-textual elements (IMG, AREA, APPLET, and INPUT) let authors specify alternate text to serve as content when the element cannot be rendered normally." Read carefully, it sais "serve as content when..." NOT "serve instead of the element when..." !!! IOW it should replace the CONTENT of the <IMG> ENTITY (ie take the place of the src=""), NOT replace the <IMG> ENTITY in it self! The width, height, title, style (CSS) and any other attribute of the <IMG> should thus NOT be affected in any way by the alt attribute and a possible load failiour of the src="" (just replace the src=""). (Of course, if the height and/or width are not specified, the "containing box" should IMO shrink to just fit the alt text. However this is something compleatly different then what this bug is currently suggesting) Please set to INVALID, yesterday if possible ... / Stefan Huszics
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
"Please set to INVALID, yesterday if possible ..." Since it's "fixed" I obviously mean change it back to how it was to begin with before you broke it =)
remarkign fixed.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: