143959 - Prompts only work once per session

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: everything starting with linux trunk 2002-05-10-21.  Works fine in linux
       trunk 2002-05-10-07

STEPS TO REPRODUCE:

1) Load the URL in the "URL" field of this bug
2) Click either OK or Cancel on the first prompt
3) Try to type in the text entry field on the second prompt

EXPECTED RESULTS:  Be able to type

ACTUAL RESULTS: no characters appear in the textbox

NOTES:
I have verified that reverting to a clean tree from friday morning makes the
problem go away.  Applying the patch for bug 129115 the causes the problem to
reappear.

This affects at least the following prompt dialogs: prompt() from JS, Basic
Authentication for HTTP, login to mailserver for mailnews.  Once a basic prompt
has been posed for any of these purposes, no prompt dialog can be typed in.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, as a note the prompt _can_ be dismissed and will send the right info.  It's
just that it never reflows to show the text you have typed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

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

Is there a chance we might not be setting the reflow root frame state bit on the
textbox?

Comment 4

[kinmoz]

From doing a quick debug, it looks like there's a missing AppendReflowCommand() 
happening in the prompt dialog.

I see this sequence when typing in an empty textfield in this bug page:

  CancelReflowCommandInternal
  AppendReflowCommand
  CancelReflowCommandInternal
  AppendReflowCommand

In the prompt case, I only see:

  CancelReflowCommandInternal
  AppendReflowCommand
  CancelReflowCommandInternal

So by the time the editor forces a flush, there's nothing in the reflow queue.

Comment 5

[kinmoz]

To answer dbaron's question ... the REFLOW_ROOT bit is being set.

Comment 6

[Chris Waterson]

Looking.

Comment 7

[Chris Waterson]

Attachment: temporary fix

This patch disables the dirty reflow coalescing that block frames do. (It's
really not that necessary given the way the reflow tree works, but we might as
well figure out why it broke.)

Comment 8

[Stephen P. Morse]

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

Comment 9

[Chris Waterson]

Attachment 83378 [details] [diff] checked in on the trunk to get the tree open. Lowering
severity; continuing to investigate.

Comment 10

[Frederic Bezies]

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

Comment 11

[Jacob Kjome]

I think this bug affects bookmarks when you file a bookmark and create a new
folder.  If you delete all of the default text of "New Folder" and then type,
you don't see anything actually print out in the text box.  However, if you
click "Ok", the folder does get created with the name of whatever you typed in.

Jake

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

rjesup says he never even hit submit so we have no idea why Bugzilla changed
some entries. Changing them back (I hope).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Jesup likely ran into this morning's other blocker -- bug 143917

Comment 14

[Chris Waterson]

Attachment: a real fix.

Here's the real fix. It reverts the previous change to nsBlockFrame.cpp and
fixes the problem in nsBoxLayoutState.cpp.

The change to nsBoxLayoutState.cpp is really a one-liner (although I've blown
it out of proportion by adding comments and renaming a variable for clarity's
sake). nsBoxReflowState::Unwind needs to pass the _root box_ during the
recursion, not the current frame's box.

Unwind works by recursively descending through the reflow tree to each of the
target frames. Along the way, it clears the NS_FRAME_HAS_DIRTY_CHILDREN bit for
each of the frames along the path. When if finally _does_ reach a target, it
resets the NS_FRAME_HAS_DIRTY_CHILDREN bit on the root box's frame, and then
calls MarkDirty on the target box.

MarkDirty walks back up through the box's ancestor chain until it reaches a
frame with the NS_FRAME_HAS_DIRTY_CHILDREN bit set. For each ancestor, it
re-sets the NS_FRAME_HAS_DIRTY_CHILDREN bit and does some initialization magic
to prepare the box for an impending relayout. If MarkDirty walks off the top of
the box hierarchy, then it will schedule a reflow command at the root box's
frame. Hence, the need to pass aRootBox down to the target frame, so that we
can properly initialize the relayout without scheduling another reflow command!


I suspect that we're doing more work than we need to do here, but would rather
fix this and then file another bug to investigate and optimize.

Comment 15

[kinmoz]

Comment on attachment 83415 [details] [diff] [review]
a real fix.

sr=[e-mail]

Comment 16

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

Comment on attachment 83415 [details] [diff] [review]
a real fix.

r=dbaron, except:
 1. Why don't we want to get rid of that |#if 0| block for good?
 2. I really don't understand why Unwind needs to do all that work.  What
happens if we don't clear the dirty bits?

Comment 17

[Matthias Versen [:Matti]]

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

Comment 18

[Chris Waterson]

Changes checked in. I've filed bug 144306 on the first issue dbaron raised, and
bug 144308 on the second.

Comment 19

[Chris Waterson]

Damn. Too much champagne and donuts this afternoon. I just noticed that the
`subject' line in mail doesn't update properly with these changes. Re-opening so
I can track _that_ problem down.

Comment 20

[Chris Waterson]

Attachment: third time's the charm?

Allright, it turns out that there are some cases where not all of the boxes are
marked with NS_FRAME_HAS_DIRTY_CHILDREN when a reflow is targeted at both the
root box and a box beneath the root.

Which means that my code to leave the NS_FRAME_HAS_DIRTY_CHILDREN bit untouched
after we hit a reflow target breaks, because we end up in a situation like
this:

  x <-- A
   o
    o
     x
      x
       x <-- B

Where `x' means the NS_FRAME_HAS_DIRTY_CHILDREN bit is set, `o' means it isn't,
and `<--' means that the frame is the target of a reflow. In this situation,
MarkDirty on frame `B' ends up being essentially a no-op, because the NS_FHDC
bit is already set.

This patch eliminates the `aClearDirtyBits' check, so we _always_ clear dirty
bits on the path to the target. This means that the hierarchy will look like
this when Unwind arrives at `B':

  x <-- A
   o
    o
     o
      o
       o <-- B

So, when we MarkDirty, _all_ of the ancestors back to the root will be marked
with NS_FHDC.

(I think that I was confused when I added this code: I was probably patching
around the mistage that I've corrected in patch 83415; namely, failure to pass
aRootBox down through the recursion.)

Comment 21

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

Comment on attachment 83462 [details] [diff] [review]
third time's the charm?

r=dbaron

Comment 22

[Frederic Bezies]

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

Comment 23

[kinmoz]

Comment on attachment 83462 [details] [diff] [review]
third time's the charm?

sr=[e-mail]

Comment 24

[Chris Waterson]

Fixed, knock on wood.

Comment 25

[Tracy Walker [:tracy]]

verified on windows commercial build 2002-05-15-04-trunk

Comment 26

[Frederic Bezies]

It is dead ! Yes ! Hate this one ! Very good trunk build :-)

Using 2002051504 - WinXP 

Comment 27

[[e-mail] (gone - use [e-mail] instead)]

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