79486 - [image input]input image showing up in wrong place

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Judson Valeski]

1. visit said URL.
2. notice the "Sign me up" button in the "Not a member yet?" section.
3. notice the input form fields in the "Already a member?" section.
4. There should be a "Sign in" button just below the "Forgot your password" link.

To verify, use 4.x to see the button.

Comment 1

[Doron Rosenberg (IBM)]

http://static.epinions.com/images/epi_images/signin.gif is the image, over to
imglib(probably a dupe)

Comment 2

[Terri Preston]

Actually, I see the sign in button, it's just in the upper left, over the 
epinions logo, layout issue? w2k build 2001050804

Comment 3

[Stuart Parmenter]

over to layout.  this is a dup of another bug... i recall something about a 
login button on netscape.com showing up in the wrong place.

Comment 4

[Hixie (not reading bugmail)]

The problem is that the <input type="image"> frame is having "issues" with being
at the end of an inline element and after a sibling block. New, well-formed test
case coming up. This doesn't occur with <img> elements. It also doesn't occur if
the image is broken (and replaced with its alt text).

Comment 5

[Hixie (not reading bugmail)]

Attachment: Simple and self explanatory testcase.

Comment 6

[rods (gone)]

since I am going to be gone and it sounds like a core layout issue...reassigning

Comment 7

[Marc Attinasi]

The problem is with the view created in the nsImageControlFrame::Init method.
When I remove the creation of the view, it is all good. I'll try to figure out
what is going wrong with the view's position...

Comment 8

[Marc Attinasi]

We get the same problem if we use the IMG and set its opacity so that a view is
created, btw. My guess is that image frames do not update their view positions
correctly when they are reflowed.

Comment 9

[Marc Attinasi]

*** Bug 75508 has been marked as a duplicate of this bug. ***

Comment 10

[Marc Attinasi]

*** Bug 84117 has been marked as a duplicate of this bug. ***

Comment 11

[Marc Attinasi]

*** Bug 82532 has been marked as a duplicate of this bug. ***

Comment 12

[Marc Attinasi]

Attachment: Testcase with normal image that is given a view (using opacity) - same problem

Comment 13

[Marc Attinasi]

The problem occurs when the intrinsic size is retrieved AFTER the initial
reflow. By interrupting in the debugger I can get the intrinsic size to be set
before the intial reflow of the image frame and then the view is positioned
correctly. Still looking, but the nsImageFrame method GetDesiredSize seems to be
missing some of the handling of the intrinsic size...

Comment 14

[Marc Attinasi]

Some more data: loading the testcase causes an incorrect frame tree. 
Specifically, the last line has an inline frame in it and this inline contains 
the image. If the testcase is loaded and a breakpoint is placed on the 
PostPlaceLine call in nsBlockFrame::PlaceLine then the last line has only the 
image (and the view is placed correctly and the page renders correctly).

I'll attach the two frame trees as a text file.

Comment 15

[Marc Attinasi]

Attachment: Frame dump compaing the correct frame tree to the incorrect frame tree: lines are constructed differently

Comment 16

[Rafael Ebron (:rebron)]

This affects http://dailynews.netscape.com/dailynews/main.tmpl
the vote button.  Added netcenter keyword.

Comment 17

[Chris Petersen]

Here is the same problem on our own site:

http://home.netscape.com/ex/shak/news/packages/microsoft/index.html

The vote button image floats outside of table. Really noticeable when resizing
the browser window too.

Comment 18

[Marc Attinasi]

*** Bug 75919 has been marked as a duplicate of this bug. ***

Comment 19

[Marc Attinasi]

BTW: the quick fix for the common manifestation of this (input image that is in
a form that is in a font) is easy: we just need to remove the View from the
input image: it does not really seem to be needed anyway. This could be a good
short-term fix for the branch, but the more fundamental problem of images with
views still needs to be fixed on the trunk. Also, the view should probably be
removed from the imput image anyway, it is a waste of a view as it provides no
benefits that I (or McClusky) can see.

Comment 20

[Jens Lautenbacher]

Hmm, so will this fix go in to the trunk soon? Or is there a "real" fix in the
works? Is the cause known by now at all? Sorry to bug you about this, but a lot
of our pages are affected by this bug, so I would be very happy when this thing
is  finally solved :-)

Comment 21

[Marc Attinasi]

I know what is causing the problem, but I do not have a real fix. My
recommendation is to remove the view from the input image frame since it is not
needed. That will fix most occurances of the problem, even though the underlying
problem will still be there. This could go on the branch if somebody wants to
try to convince the PDT to allow it, but they only want ABSOLUTELY STOP SHIP
bugs now, so it is unlikely to get their blessing.

Alternatively, you can just change the markup to fix the problem. The problem
only occurs when you have an input image inside of a block that is inside of an
inline. This is usually the following markup on netcenter (and elsewhere):

<font>
<form>
<input type="image">
<other controls maybe>
</form>
</font>

change it to:

<form>
<font>
<input type="image">
</font>
</form>

instead and the bug goes away (blocks in inlines are not really valid, and the
problem is in fixing it up internally).

Comment 22

[Tuukka Tolvanen (sp3000)]

*** Bug 91781 has been marked as a duplicate of this bug. ***

Comment 23

[Mats Palmgren (inactive)]

*** Bug 87856 has been marked as a duplicate of this bug. ***

Comment 24

[Mike Young]

*** Bug 92310 has been marked as a duplicate of this bug. ***

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

waterson was going to look into one of the dups, perhaps.  He suggested that it
might have something to do with not positioning a view correctly, which seems
like a likely theory.

Comment 26

[Chris Waterson]

Looking at this...

Comment 27

[Marc Attinasi]

... great! I look forward to seeing what you find after a lot of banging my head
against the screen on this one. CC'ing self.

Comment 28

[Chris Waterson]

Attachment: propagate frame state bits in MoveChildrenTo()

Comment 29

[Chris Waterson]

Usually, when a view is created for a frame, the frame is already in the frame
hierarchy. In a situation where a block frame ends up inside an inline, we
create ``anonymous'' frames to parent the block frames and the trailing inline
frames. The bug was that these anonymous frames weren't getting the
NS_FRAME_HAS_CHILD_WITH_VIEW flag properly set. I've altered the
|MoveChildrenTo()| helper function to do that.

This problem appeared to be sporadic because of a recent optimization that I put
in for the |ContentAppended()| case. If we tripped that optimization, then we'd
end up simply appending the new input frame to the anonymous block, and the
frame state bits would be updated correctly. If we _didn't_ trip that code, then
we stumble through |ConstructInline()|, which would fail to propagate the flag.

I've also tidied up the code where we propagate the NS_FRAME_HAS_CHILD_WITH_VIEW
flag up the frame tree in nsFrame, and fixed a crasher that occurs when
GECKO_FRAMECTOR_DEBUG_FLAGS=really-noisy-content-updates. (I botched the
|ContentInserted()| case where |parentFrame == 0|.)

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What does reparenting a style context mean, and when is MoveChildrenTo called? 
(Does it mean you change the inheritance chain?  Shouldn't that correspond to
the content model?)

Comment 31

[Marc Attinasi]

When I was debugging this I saw that delaying the first reflow (by breaking in
the debugger and then running again) caused the problem to go away. Was that
delay somehow triggering your optimization?

By the way: I am in total and complete awe of your speed in fixing this...

Comment 32

[Chris Waterson]

MoveChildrenTo() is a module-static function, and is only called by the {ib}
handling code. It's used in SplitToContainingBlock() and ConstructInline().

ReParentStyleContext() creates a new style context for the frame that is
parented by a different parent style context. It appears that it can be used to
change the inheritance chain; however, in this case it's not used that way,
since the anonymous elements still refer to the same content node.

I'm a bit nervous about having a frame that is parented by the anonymous block
frame ending up with its style context parented by the style context from the
inline. Mightn't we end up deriving computed values differently based on the
parent's display type? Or deriving computed values based on computed values from
our parent (which would be different due to display type)? Anyway, if you think
this is superfluous, then I'll remove it. (Since {ib} behavior is undefined, it
probably doesn't matter that much.)

Comment 33

[Chris Waterson]

marc: I noticed that I got different results depending on whether or not I let
output spew to the console. When I sent output to the console, the test case
displayed correctly; if I redirected to a file, it displayed incorrectly! As you
pointed out, two different frame models got created. I think what was happening
is this:

. In the ``correct'' case, a ContentInserted() was adding everything except
  the <input>. Then a subsequent ContentAppended() was adding the <input>.
  At the time the ContentAppended() arrived, the {ib} hierarchy had no trailing
  inline frames, so ContentAppended() just went ahead an attached the inline
  to the anonymous block and the normal logic kicked in to set the
  NS_FRAME_HAS_CHILD_WITH_VIEW logic.

. In the ``incorrect'' case, a single ContentInserted() was adding both the
  block and the <input> element. When this happened, we'd fall into
  ConstructInline(). This would put the inline frame in the ``trailing''
  anonymous inline frame, but would fail to set NS_FRAME_HAS_CHILD_WITH_VIEW
  on the anonymous inline after moving the <input>'s frame there.

Since I was able to see this difference just based on whether the console was
busy or not, I suspect that setting a breakpoint in reflow similarly perturbed
the order of event processing...
  

Comment 34

[Chris Waterson]

This clears the layout regression tests.

Comment 35

[Marc Attinasi]

I like it: [s]r=attinasi

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

r=dbaron, although I wonder (as we discussed) whether we really want to do
reparenting of style contexts here.  Maybe it should be |#if 0| for now?

Comment 37

[Chris Waterson]

Will do. Thanks.

Comment 38

[Chris Waterson]

Fix checked in, with style context reparenting #if 0'd out and commented in big
red lights.

Comment 39

[Chris Waterson]

*** Bug 92907 has been marked as a duplicate of this bug. ***

Comment 40

[chris hofmann]

lets get this on the 0.9.2 branch too..

Comment 41

[Chris Waterson]

Changes checked in on MOZILLA_0_9_2_BRANCH.

Comment 42

[Mats Palmgren (inactive)]

*** Bug 94190 has been marked as a duplicate of this bug. ***

Comment 43

[bsharma]

Verified on 2001-08-08-Trunk build on WinNT.

The above url and the attached url's load fine.