Bugzilla

Assignee

Comment 1

•

24 years ago

Attached patch patch to fix bug (has some extra stuff in it... ignore :)) (deleted) — Details — Splinter Review

Assignee

Comment 2

•

24 years ago

Attached patch cleaner patch (deleted) — Details — Splinter Review

Assignee

Updated

•

24 years ago

Status: NEW → ASSIGNED

Keywords: patch

Priority: -- → P2

Target Milestone: --- → mozilla0.9

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

Terri Preston

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

Attached patch newer patch. (deleted) — Details — Splinter Review

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

Attached file block test output (gzip'd text file) (deleted) — Details

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

Attached patch newest patch (what i ran the regression tests against) (deleted) — Details — Splinter Review

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

Attached patch newer patch (deleted) — Details — Splinter Review

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. ***

Stefan Huszics

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

Stefan Huszics

Updated

•

23 years ago

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Stefan Huszics

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 =)