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)
Tracking
()
RESOLVED
FIXED
mozilla0.9
People
(Reporter: ian, Assigned: pavlov)
References
(Blocks 2 open bugs, )
Details
(Keywords: regression, testcase, Whiteboard: [imglib])
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/x-gzip
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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.
Reporter | ||
Updated•24 years ago
|
Assignee | ||
Comment 1•24 years ago
|
||
Assignee | ||
Comment 2•24 years ago
|
||
Assignee | ||
Updated•24 years ago
|
Assignee | ||
Comment 3•24 years ago
|
||
looking for reviews for this patch
Comment 4•24 years ago
|
||
+ } 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
Updated•24 years ago
|
Whiteboard: [imglib]
Comment 5•24 years ago
|
||
attinasi is probably the best guy to look at nsImageFrame changes.
Comment 6•24 years ago
|
||
OK, checking it out now.
Comment 7•24 years ago
|
||
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?
Assignee | ||
Comment 8•24 years ago
|
||
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.
Comment 9•24 years ago
|
||
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.
Assignee | ||
Comment 10•24 years ago
|
||
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.
Assignee | ||
Comment 11•24 years ago
|
||
Comment 12•24 years ago
|
||
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
Comment 13•24 years ago
|
||
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
Comment 14•24 years ago
|
||
Of course, as with any layout change, you must run the layout regression tests
before you check this in, biyotch. ;-)
Assignee | ||
Comment 15•24 years ago
|
||
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"
Comment 16•24 years ago
|
||
Ok. I'll be excited to hear how those layout regression tests come out.
Assignee | ||
Comment 17•24 years ago
|
||
Assignee | ||
Comment 18•24 years ago
|
||
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.
Assignee | ||
Comment 19•24 years ago
|
||
Assignee | ||
Comment 20•24 years ago
|
||
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.
Assignee | ||
Comment 21•24 years ago
|
||
Assignee | ||
Comment 22•24 years ago
|
||
added back support for internal-gopher images ;) to fix one of the regression
tests
Comment 23•24 years ago
|
||
Patch 30910 looks good, although I do not understand the gopher stuff -
[s]r=attinasi
Comment 24•24 years ago
|
||
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
Assignee | ||
Comment 25•24 years ago
|
||
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
Comment 26•24 years ago
|
||
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
Assignee | ||
Comment 27•24 years ago
|
||
fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 28•24 years ago
|
||
*** Bug 75821 has been marked as a duplicate of this bug. ***
Comment 29•23 years ago
|
||
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
Updated•23 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 30•23 years ago
|
||
"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 =)
Assignee | ||
Comment 31•23 years ago
|
||
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.
Description
•