106489 - [PATCH]Text entered in box does not appear

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Henry Throop]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.5) Gecko/20011012
BuildID:    2001101202

Text entered in the 'Search' box on this website does not appear on the screen.
 It is actually entered, as can be seen by hitting the 'Go' button.  Works fine
on NS 4.75.

Reproducible: Always
Steps to Reproduce:
1.Go to http://www.alibris.com
2.Select the 'Search' box, and enter a search term
3.Hit 'Go'

Actual Results:  No text appears on the screen when typed.  However, the search
term entered does appear on the screen on the search results page after hitting
'Go'.

Expected Results:  The text should be displayed on the screen when entered.

Comment 1

[rubydoo123]

doing the reduced test case now

Comment 2

[rubydoo123]

this is one weird problem. I have the reduced test case. If anything is removed 
from the test case, then it works. The leading table and trailing table must be 
there. I don't get it!

Comment 3

[rubydoo123]

Attachment: reduced test case

Comment 4

[Kathleen :Brade]

This sounds like a duplicate... I know I've seen it before (even the testcase 
looks very familiar)...

Kin: any ideas?

I don't think this is linux-specific

Comment 5

[Kevin McCluskey (gone)]

Rod, could you take a look? 

Comment 6

[rods (gone)]

very strange, since it isn't a sizing issue over to kin

Comment 7

[kinmoz]

Moving this in to 0.9.7. A preliminary look at this in the debugger shows that
the floating table's TableOuterFrame origin isn't being set properly. That is,
it's y coordinate is zero during the incremental reflow when it should be
non-zero, so the TextWidget's view is not being positioned properly.

Still looking into where the problem is.

Comment 8

[kinmoz]

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

Comment 9

[kinmoz]

--> 0.9.8

Comment 10

[TucsonTester2]

confirming not platform specific. even though bug is already set to all, tested
using mac os x build 2001121805

Comment 11

[Syd Logan]

plussing

Comment 12

[kinmoz]

Attachment: Minimal Test Case (whitespaced stripped out to simplify frame/content tree)

Comment 13

[Amarendra Hanumanula]

 Adding "testcase" keyword.

Comment 14

[kinmoz]

--> mozilla1.0

Comment 15

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

Attachment: IRC log in which this problem was discussed

This is a log of an IRC discusssion kin and I had about this problem, and could
be useful for trying to remember what the problem was here.

Comment 16

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

Attachment: slightly more difficult testcase (as mentioned in IRC discussion)

Comment 17

[kinmoz]

Attachment: Patch Rev 1

Ok, so here's my first stab at fixing this particular problem.

The patch does the following:

  1. Moves GetRealFrame() and MoveChildrenTo() to a point in
     the file that is before anything that uses them *and* after
     the class nsFrameConstructorState is defined.

  2. Adds a method called AdjustOutOfFlowFrameParentPtrs()
     which is a method that recurses down through the frame
     hierarchy looking for placeholders within inline nodes.

  3. I modified MoveChildrenTo() to take an optional aState
     parameter, so that when it is present, would trigger
     a call to AdjustOutOfFlowFrameParentPtrs().

  4. Modified all calls to MoveChildrenTo() in the file
     to pass in a null, except when calling from ConstructInline().

To sum up the IRC discussion attatched to this bug, I found that
while loading the document, depending on the number of notifications
and when they happen the frame tree under an inline that contains
block elements is built differently, which I filed as bug 133989.

This meant that the path built by an nsHTMLReflowCommand could at
times be missing a parent to the target frame, which during reflow
could result in an IncrementalReflow being mapped to some other
type of reflow like a ResizeReflow which would somehow get ignored
because some of the frames didn't think there was any work that
needed to be done.

In any case this explains why I would see the problem with the
testcases *sometimes* and not other times.

This patch tries to correct or at least make the parent hierarchy
consistent, with the parent hierarchy that exists when things
do actually work.

Comments, reviews, and alternate approaches welcome.

dbaron - you'll probably notice that I simply reparent the out-of-flow
frame, and don't bother to move the out-of-flow that is in the aState
used in ConstructInline(). I'm a little fuzzy on what frame should
actually have the out-of-flow frame in their floater list in this
particular case.

Comment 18

[Kevin McCluskey (gone)]

adt2

Comment 19

[Chris Waterson]

Comment on attachment 76607 [details] [diff] [review]
Patch Rev 1

A comment about what this function does would be nice. :-)

>+static void
>+AdjustOutOfFlowFrameParentPtrs(nsIPresContext*          aPresContext,
>+                               nsIFrame*                aFrame,
>+                               nsFrameConstructorState* aState)
>+{
>+  nsIFrame *outOfFlowFrame = GetRealFrame(aFrame);
>+
>+  if (outOfFlowFrame && outOfFlowFrame != aFrame) {
>+
>+    // Get the display data for the outOfFlowFrame so we can
>+    // figure out if it is a floater or absolutely positioned element.
>+
>+    const nsStyleDisplay* display = nsnull;
>+    outOfFlowFrame->GetStyleData(eStyleStruct_Display,
>+                                (const nsStyleStruct*&)display);
>+ 
>+    if (!display) {
>+      NS_WARNING("outOfFlowFrame has no display data!");
>+      return;
>+    }
>+
>+    // Update the parent pointer for outOfFlowFrame if it's
>+    // containing block has changed as the result of reparenting,
>+    //
>+    // XXX_kin: I don't think we have to worry about
>+    // XXX_kin: NS_STYLE_POSITION_FIXED or NS_STYLE_POSITION_RELATIVE.
>+
>+    if (NS_STYLE_POSITION_ABSOLUTE == display->mPosition) {
>+      // XXX_kin: I think we'll need to add code here to handle the
>+      // XXX_kin: reparenting that can happen in ConstructInline()?
>+      // XXX_kin:
>+      // XXX_kin: The case I'm thinking about here is when the inline being
>+      // XXX_kin: constructed has a style="position: relative;" property
>+      // XXX_kin: on it, and ConstructInline() moves/reparents all child block
>+      // XXX_kin: frames and any inlines (including placeholders) that happen
>+      // XXX_kin: to be between these blocks, under the new inline-block it created.
>+      // XXX_kin: I think right now this case generates an assertion during
>+      // XXX_kin: reflow, and as a result things fail to render since I believe
>+      // XXX_kin: the placeholder is parented to the inline-block, and the
>+      // XXX_kin: outOfFlowFrame is in the original inline frame's absolute list,
>+      // XXX_kin: and is also parented to it.
>+    }
>+    else if (NS_STYLE_FLOAT_NONE != display->mFloats) {
>+      outOfFlowFrame->SetParent(aState->mFloatedItems.containingBlock);
>+    }
>+
>+    // XXX_kin: Hmmmm, do we have to worry about placeholders within
>+    // XXX_kin: out-of-flow frames? If so, we'll probably want to continue
>+    // XXX_kin: here instead of just returning.
>+

I think it might be more complicated, even. :-/

Specifically, we'd want to continue to descend into placeholders until we found
an
out-of-flow frame that established a new containing block for absolutely
positioned
elements. At that point, we could terminate the descent, because any absolutely
positioned frames below that frame would be properly parented.

But we can fix that another day.


>@@ -14198,7 +14290,7 @@
>                       nsnull, blockSC, nsnull, blockFrame);
> 
>   blockFrame->SetInitialChildList(aPresContext, nsnull, aBlockChildFrame);
>-  MoveChildrenTo(aPresContext, blockSC, blockFrame, aBlockChildFrame);
>+  MoveChildrenTo(aPresContext, blockSC, blockFrame, aBlockChildFrame, nsnull);
> 
>   // Create an anonymous inline frame that will parent
>   // aRightInlineChildFrame. The new frame won't have a parent yet:

Does this case potentially need to re-parent out-of-flow frames, too? (I.e.,
should
we be passing in a state instead of nsnull?)

r=/sr=waterson.

Comment 20

[kinmoz]

Attachment: Patch Rev 1.1

This patch adds the comment requested by waterson, as well as incorporates his
comments about future work to support recursive descent into placeholders.

To answer waterson's question:

> Does this case potentially need to re-parent out-of-flow frames, too? (I.e.,
> should we be passing in a state instead of nsnull?)

At the moment my patch only deals with floaters. Since we are reparenting the
items in list3 to another inline frame, that doesn't result in any containment
block change for floaters in the list, so passing in a state would still do
nothing with the current implementation. I probably will have to pass a state
once AdjustOutOfFlowFrameParentPtrs() supports absolute positioned frames.

Comment 21

[Marc Attinasi]

Comment on attachment 77936 [details] [diff] [review]
Patch Rev 1.1

r=attinasi

Comment 22

[kinmoz]

Answering waterson's question again, this time paying attention to the last arg 
in his example. ;-)

> Does this case potentially need to re-parent out-of-flow frames, too? (I.e.,
> should we be passing in a state instead of nsnull?)

I imagine it would if we are reparenting to a block frame, though I'm not 
exactly sure how to generate a test case that would trigger this particular 
piece of code. I thought it would be more prudent at this time to just leave it 
alone to avoid any potential regressions and perhaps revisit it when bug 133989 
is addressed.

Comment 23

[Jaime Rodriguez, Jr.]

Removing adt1.0.0. Pls land this on the trunk, and let it bake for a couple of
days. If there are no regressions, and QA approves, pls renominate.

Does this this bug manifest itself only on this site, or does it appear on any
top 100 sites?

Comment 24

[Kevin McCluskey (gone)]

adt: Although we have not identified other specific sites, the condition under
which text fields will fail is not uncommon. The text field just needs to be
inside a floater (align=) container.  Floaters are very common, so we would
expect to see a fairly large number sites that exhibit this flaw.

Comment 25

[kinmoz]

Fix checked in on TRUNK:

    mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp  revision 1.729

We'll let this bake a couple days on the TRUNK before I re-request adt and 
driver approval for checkin on the MOZILLA_1_0_0_BRANCH.

Comment 26

[Kevin McCluskey (gone)]

petersen: Could you verify the fix on the trunk? Thanks!

Comment 27

[Chris Petersen]

Verified on OS X trunk (2002-04-12-08) and Windows ME Trunk (2002-04-12-10)

Comment 28

[Kevin McCluskey (gone)]

Resolving as FIXED. Please add fixed1.0.0 keyword after the fix has been checked
into the 1.0.0 branch.

Comment 29

[Chris Petersen]

Based on my last comment, marking verified.

Comment 30

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for checkin into the 1.0 branch. Pls check this in
to the branch today. After it is checked in, pls add fixed1.0.0. Once QA has
verified it on the branch, then add verified1.0.0.

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 77936 [details] [diff] [review]
Patch Rev 1.1

a=[e-mail] for post-RC1 checkin

Comment 32

[kinmoz]

Fixed on the MOZILLA_1_0_0_BRANCH:

    mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp  revision 1.727.2.2

Comment 33

[Chris Petersen]

Verified in branch (0SX 2002-04-26-05) and (WinME 2002-04-25-06).

Comment 34

[Dale Feinstein]

I have experienced not being able to enter text into text fields with several
Web sites in 0.99. However, the problem seemed to have been resolved with 1.0
RC1.  Now, I am experiencing this problem again with 1.0 RC2.

Steps to reproduce this bug:
1.  Go to http://www.leedungarees.com/ .  You will need Flash to access this site.
2.  Click on the "Register to Win Jeans!" Flash link.
3.  A pop-up window will appear.  Try to enter text in the "Email" text field.

Comment 35

[kinmoz]

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