116230 - Text fields used for logins are behaving erratic on text input and removal.

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[André Dahlqvist]

[Build-ID: 2001-12-20-08]

Steps to reproduce:

1) Go to http://www.slashdot.org
2) Type text into the login and password fields on the right and then erase it.

Result: Notice that on the first keypress one character is written, but after
that two characters are written on each keypress. For an example, if you type
"And" the 'A' will immediately show up but the "nd" part will not show up until
on the third keypress.

This seams to only happen on input fields used for logins, because it works
nicely in bugzilla. Another testcase is http://www.gnuheter.com. The same thing
happens there if you type something into the login boxes.

Also notice the "strange" behaviour when you erase the characters.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Over to editor.  Wasn't the checkin for bug 91423 supposed to fix this?

Comment 2

[K Chayka]

win32 build 2001122208, win98se

I ran into exactly the same thing at http://www.mapblast.com/myblast/index.mb

After experimenting, I narrowed it down to a comination of 3 things:
1. A text input field in a table cell
2. A <br> between the label and the input box
3. <font> tags around it all

Removing any 1 of the above 3 conditions restores behaviors to normal.  Both
slashdot and mapblast have all 3 conditions.

I'm not sure when this started happening, but the bug doesn't exist in
the 12/10 build.

Comment 3

[K Chayka]

Attachment: html page for reproducing the problem

Comment 4

[Chris Waterson]

Hrm, the patch in bug 91423 was supposed to fix stuff like this. Let me take a
look...

Comment 5

[Chris Waterson]

This was definitely caused by the checkin for bug 91423. The problem is that we
don't want to retarget a reflow to the primary frame when ``hard'' breaks are
present, for example, <br> frames (and possibly `white-space: pre'). When a hard
break is present, the unconstrained reflow doesn't necessarily recreate all of
its continuations, as I'd assumed.

It might be possible to change this logic to retarget the reflow to the first
frame following a hard break, or the primary frame, whichever we encounter first
when walking backwards. Then again, that might not work in more complicated
situations, for example, could a case exist where the unconstrained reflow would
destroy the continuation immediately after a hard break? (Perhaps when the hard
break appears in a continuation?)

Comment 6

[rbs]

I tried the following change to your patch from bug 91423:

-    if (aState.mNextRCFrame) {
+    if (aState.mNextRCFrame &&
+        aState.mCurrentLine->GetBreakType() == NS_STYLE_CLEAR_NONE) {

It suprisingly fixed attachment 62684 [details] too, but this could mean that modifying 
the reflow path may be having a side-effect since the <BR> is located in the 
container higher up in the path, and the new condition is simply stopping the 
path from changing ahead of time.

Perhaps, the real problem might be that |aState.mNextRCFrame| and the reflow 
path are out of sync.

BTW, another edge case that I meant to ask. What about the other code (elsewhere
in the tree) that are doing:

   |next| = rc->GetNext()
   [...]do a reflow that cascades to the block code, which then mangles the path
   do something with |next|, ->boom?

Comment 7

[Chris Waterson]

Hmm. I'm not sure I see the relationship between aLine and aState.mNextRCFrame.
(nsBlockFrame::ReflowLine will be called for each dirty line in the block.)

Don't we really care about whether or not the continuation flow that winds up at
aState.mNextRCFrame runs through a line that is NS_STYLE_CLEAR_LINE? (In which
case, the first continuation after the NS_STYLE_CLEAR_LINE line will live on
despite the unconstrained reflow.)

> Perhaps, the real problem might be that |aState.mNextRCFrame| and the reflow
> path are out of sync.

I'm not sure I understand that. aState.mNextRCFrame ought to have been picked
from the reflow path in nsBlockFrame::Reflow.

> BTW, another edge case that I meant to ask. What about the other code 
> (elsewhere in the tree) that are doing:

IOW, you are worried about an activation peeking at the next frame along the
reflow path and remembering that frame before we descend into
nsBlockFrame::ReflowLine? (Any activations during ReflowLine will, of course, be
looking at the ``retargeted'' path.) I'll need to look, I guess, but I assumed
that fixing up aState.mNextRCFrame would be sufficient.

Comment 8

[Chris Waterson]

Attachment: proposed fix

Here's how I'd propose to fix this problem. Instead of walking back to the
primary frame when we need to ``retarget'', we walk back through the
continuations *in parallel* with the line list, stopping if we detect either
that we've reached the primary frame *or* we've hit a line that is
NS_STYLE_CLEAR_LINE.

I'm still doing my homework on the inline reflow to make sure this is
sufficient, but I thought I'd throw this out there for comment.

Comment 9

[Chris Waterson]

Attachment: torture tests

Inline incremental reflow torture tests.

Comment 10

[rbs]

>IOW, you are worried about an activation peeking at the next frame along the
>reflow path and remembering that frame before we descend into
>nsBlockFrame::ReflowLine?

That's right. In fact, my entire post about the out-of-sync possibility is based 
on that point too, e.g., a scenario such as:

   . rc->GetNext(aState.mNextRCFrame);
   . prepare the incremental reflow based on aState.mNextRCFrame, this might set
     aState.mCurrentLine for example
   . then mangling/retargeting the path changes aState.mNextRCFrame and might
     have some other effects (e.g., the data from the previous preparation
     become irrelevant).

But since it is hard and time-consuming to check things out fully, I have no 
concrete details to confirm or deny the scenario. But I am a bit nervous of 
mangling the path and was wondering if there were no other alternatives that 
will force all (e.g., on a local basis?) the inline continuations to be reflowed 
(or recreated) since that's what your analyses in bug 91423 suggest.

Comment 11

[Chris Waterson]

IIRC, the problem with just marking the continuations dirty is that the reflow
will get optimized away. The inline code will convert the reflow reason to
``resize''; the box frame detects that no size change has occurred, and
therefore refuses to reflow the text field's anonymous <div>.

The point you raise is a good one: it can't hurt to try to construct a case that
will cause problems...

Comment 12

[rbs]

What if the inline code is changed so as not to convert the reflow reason in the
particular unconstrained reflow case?

Comment 13

[Chris Waterson]

At first, I thought the reflow reason logic was insane (see, e.g., bug 73348). I
now think I may see some method to the madness. Specifically, both
nsBlockReflowContext::ReflowBlock and nsLineLayout::ReflowFrame have similar
code that does something like this:

  Start by assuming a reflow reason of `resize', so that a child
  can optimize the reflow away if its size hasn't changed.

  IF the frame hasn't yet been reflowed
  THEN
    Set the reflow reason to `initial' so that the child frame
    knows it needs to do its initial reflow.
  ELSE IF the frame lies along the incremental reflow path
  THEN
    Propagate the `incremental' reflow reason so that the child
    frame knows to pop the next frame from the reflow path.
  ELSE IF the reflow reason is `style change'
  THEN
    Propagate the `style change' reason untouched so the child
    frame doesn't attempt to optimize away the reflow.
  ELSE IF the reflow reason is `dirty'
  THEN
    IF we are actually dirty
    THEN
      Propagate the `dirty' reason so we don't attempt to
      optimize away the reflow if our size hasn't changed.
    ENDIF
  ELSE IF the reflow reason is `incremental'
  THEN
    Either we are the target of the incremental reflow, or
    we somehow got ``stumbled across'' while processing an
    incremental reflow targeted at another frame.

    IF the former (i.e., we are the target of the incremental
       reflow), AND the reflow is a `style change' reflow,
    THEN
       Propagate the `style change' reflow reason so that our
       entire subtree will reflow itself.
    ENDIF (*)
  ENDIF

(*) nsLineLayout has an additional clause here:

    ELSE IF the incremental reflow is a `dirty' reflow, AND
            we are actually dirty,
    THEN
      Propagate the `dirty' reflow reason so our children
      don't optimize away the reflow.

So, in theory, we may be able to implement your suggestion with the existing
logic. Specifically, we could punt and dirty the _entire_ inline frame subtree,
starting with the target continuation frame's primary frame, and recursing
through all of its continuations and their subtrees. IOW, in
nsBlockFrame::ReflowLine,

  IF we have an incremental reflow targeted at a continuation
     AND are about to do an unconstrained reflow
  THEN
    LET frame := the target frame's primary frame.
    WHILE frame != target
    DO
      RecursiveMarkFrameSubtreeDirty(frame).
      frame <= next-in-flow(frame).
    DONE
  ENDIF

This is clearly a ``shotgun approach'' that would lead us to doing more reflow
that the ``reflow path fixup'' approach.

But thinking out loud a bit...we could probably optimize this such that we'd
only mark frames dirty that lie along the reflow path ``between'' each target
continuation frame and its primary frame (or better yet, between its target
continuation and the first frame after a ``hard break'').

But if _that_ solution will work, oughtn't the ``reflow path fixup'' solution
work, too? In fact, aren't the ``reflow path fixup'' and ``dirty subtree''
solutions isomorphic? Specifically, the ``dirty subtree'' approach runs the risk
of walking ``too far back''; i.e., to a frame in a line prior to the reflow
state's current line. And that will happen in exactly the same cases where the
``reflow path fixup'' moves nsBlockReflowState::mNextRCFrame to a frame in a
line prior to nsBlockReflowState::mCurrentLines.

Comment 14

[rbs]

Indeed, it seems that the two approaches may boil down to the same thing -- save 
that there is no mangling of the reflow path. Regarding marking the subtree 
dirty, I was wondering if this can't be done gradually (not recursively, but 
each segment at a time), and with the additional change:

  ELSE IF the reflow reason is `dirty'
  THEN
    IF we are actually dirty
+   OR we are a continuation and are about to do an unconstrained reflow
    THEN
      Propagate the `dirty' reason so we don't attempt to
      optimize away the reflow if our size hasn't changed.
    ENDIF

But as usual with this dreaded layout, it is unclear whether something work 
without trying it out :-(

Comment 15

[Chris Waterson]

...and something that fixes the test case is likely to break something else you
hadn't thought of. :-)

I think that we agree that tinkering with the yet-to-be-visited portion of the
reflow path is no big deal, right? I.e., it's mutating mNextRCFrame that could
be the problem. Specifically, we run the risk of having an inconsistency that
may have unknown effects later on. (Unless, of course, we can prove that it 1)
won't cause an inconsistency, or 2) the inconsistency would be insignificant.)

Anyway, I'll experiment with your proposal and report back anon.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Chris Waterson]

Okay rbs, so the problem with the solution that you suggest is that the inline
/frames/ along the reflow path aren't dirty, even though the /lines/ that they
live in are.

Recall from bug 91423 that we mark the frames dirty when we schedule the reflow.
Specifically, nsBlockFrame::ReflowDirtyChild is marking the frame for the
anonymous <div> in the text field as dirty. The dirty bit only propagates up one
level to the the ScrollPortFrame that immediately wraps the <div>'s Area frame.
Later, during the process of performing the reflow, we pluck a frame from the
reflow path, and nsBlockFrame::PrepareChildIncrementalReflow marks the line
containing that frame as dirty.

This is where the special-case code for handling an incremental reflow comes
into play; i.e.,

  IF the frame lies along the incremental reflow path
  THEN
    ...

I suppose at this point we could mark the frame's continuations as dirty...

(Sorry this took so long to turn around. Holidays conveniently got in the way! ;-))

Comment 18

[Chris Waterson]

...but of course, we'll never reach ``this point''. I.e.,

  *aNextRCFrame == aFrame

will _never_ be true, because we'll have destroyed the continuation in the
unconstrained reflow. Boioioing!

Comment 19

[Chris Waterson]

Attachment: move path fixup to nsBlockFrame::PrepareChildIncrementalReflow

rbs, how about this fix? It's as above (i.e., we monkey with the reflow path);
however, we do the monkeying during nsBlockFrame::PrepareChildIncremenalReflow,
well before we begin iterating through the line list.

Comment 20

[rbs]

Upon applying the patch, I didn't observe a particular wierdness. I added the 
following old & good printfs to have a little insight into what might be 
happening in the torture tests during typing. You might find useful to 
experiment with them as well.

void
nsBlockFrame::RetargetInlineIncrementalReflow(nsBlockReflowState &aState,
                                              line_iterator &aLine,
                                              nsIFrame *aPrevInFlow)
{
//XXXrbs - entry
  nsIFrame* oldFrame = aState.mNextRCFrame;
  nsAutoVoidArray oldPath;
  oldPath.InsertElementsAt(*aState.mReflowState.reflowCommand->GetPath(), 0);

  [... your code here ...]

//XXXrbs - exit
  nsIFrame* newFrame = aState.mNextRCFrame;
  nsVoidArray* newPath = aState.mReflowState.reflowCommand->GetPath();
  PRBool isSamePath = PR_TRUE;
  for (int k = oldPath.Count()-1; k >= 0 ; k--) {
    if (newPath->IndexOf(oldPath[k]) < 0) {
      isSamePath = PR_FALSE;
      break;
    }
  }
  if (newFrame != oldFrame && !isSamePath) {
    printf("RetargetInlineIncrementalReflow: frame & path changed\n");
  }
  else if (newFrame != oldFrame) {
    printf("RetargetInlineIncrementalReflow: frame changed\n");
  }
  else if (!isSamePath) {
    printf("RetargetInlineIncrementalReflow: path changed\n");
  }
  else {
    printf("RetargetInlineIncrementalReflow: nothing changed!\n");
  }
}

Comment 21

[Chris Waterson]

> Upon applying the patch, I didn't observe a particular wierdness.

I'm not sure what you mean by this statement?

> I added the following old & good printfs to have a little insight into what
> might be happening in the torture tests during typing. You might find useful
> to experiment with them as well.

I.e., in order to test code coverage?

Comment 22

[rbs]

> > Upon applying the patch, I didn't observe a particular wierdness.
>
>I'm not sure what you mean by this statement?

That is code-reading and code-testing were okay at this end.

> I.e., in order to test code coverage?

Yes.

Comment 23

[rbs]

Comment on attachment 63319 [details] [diff] [review]
move path fixup to nsBlockFrame::PrepareChildIncrementalReflow

r=rbs

Comment 24

[Chris Waterson]

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

Comment 25

[Chris Waterson]

I went ahead and checked this in without super-review as the dups are starting
to pile up. attinasi, dbaron: could you please do a post-hoc review on this? Thanks!

Comment 26

[Marc Attinasi]

Comment on attachment 63319 [details] [diff] [review]
move path fixup to nsBlockFrame::PrepareChildIncrementalReflow

sr=attinasi - good call on the checkin too.

Comment 27

[Michael Dunn]

since Bug 91423 is in 094. Does this need 094 check-in? Bugscape 11618

Comment 28

[André Dahlqvist]

Verified fixed with 2002-01-10-21 on Linux.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

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