108175 - INPUT loses value

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Axel Hecht]

textfields have their value property set, but they don't display it,
and they clear the field on submit.
In bugzilla, that clears QA, URL, Summary, all the useful information :-(
Probably regression by jkeiser.

Comment 1

[Håkan Waara]

cc twalker on this smoketest blocker.

Comment 2

[R.K.Aa.]

i wonder if this was fixed already. 03:28 in bonsai:

Fix problem in the formrewrite changes, the new code ends up doing a flush on
the document from within DemoteContainer() in the sink which ends up notifying
the document about some peices of content more than once. This causes duplicated
content on some pages. r=[e-mail]. sr=myself.

Comment 3

[Axel Hecht]

that was causing the bugzilla form to appear twice in the rendering.
The build I tested this on was built with that checkin.

Comment 4

[R.K.Aa.]

I saw the bug before the checkin, pulled it, rebuilt: bug gone. (Linux)

Comment 5

[John Keiser (jkeiser)]

Ah, yes, that is precisely what was fixed last night.  I'll check a nightly
shortly.  Posting this with a build made shortly after the aforementioned fix.

Comment 6

[Axel Hecht]

Can't verify anymore. No idea what happened. I was pretty sure that I had this
in my build, and rebuilding didn't really change much.
Maybe I have to settle like btek.
Oh, I can't scroll in the select in bugzilla. :-(
Resolving this one, sorry for the noise

Comment 7

[John Keiser (jkeiser)]

In this bug?  There's not enough entries in this bug to scroll ... the scrollbar
looks the same as it did before, too (I have always wondered why we show a
scrollbar with room to scroll when there actually isn't room to scroll).  On bug
34297, for example, there are enough entries to scroll, and it does.

Comment 8

[John Keiser (jkeiser)]

rkaa noted that the scrollbar bug is bug 45731.

Comment 9

[Axel Hecht]

so then I'm fine. And just paranoid. I saw a scrollbar that didn't do anything,
nice to see that this is "works as designed".
Thanx

Comment 10

[Bradley Baetz (:bbaetz)]

restoring lost values.Did this get into any nighyl build? If so, we need to
block them from bz.

Comment 11

[R.K.Aa.]

oops.. seems there's a remnant?
The double-page is fixed. But i just accidentally happened to delete a summary
when adding comment in a bug, using a CVS build WITH the patch from here.

Comment 12

[R.K.Aa.]

Reopening.

With a build containing the "fix" I accidentally deleted QA and summary in bug
108175 when adding comment. The CVS build i then used also had the fix for
108175 and was built --disable-bidi
Can someone on a current bidi-enabled CVS build please test if INPUT remains intact.

Comment 13

[R.K.Aa.]

repulled - rebuilt - now what..

Comment 14

[R.K.Aa.]

weird. When reloading the bug in the CVS build, fields for QA, Summary and
Keywords were gone.

When looking at the bug with 2001102910 those fields are back.
When checking bug summary, i seem to have changed resolution to FIXED.
Yet it still reads "REOPENED" in the bug itself.

Comment 15

[R.K.Aa.]

DOH..NOT set to fixed... and NOT modified.. yet the fields appear blank in the
CVS build. 

Comment 16

[rods (gone)]

reassigning to [e-mail]

Comment 17

[John Keiser (jkeiser)]

I'm afraid I can't get to it until this evening (I'm at work), but I will do
everything humanly possible to get this found and fixed before nightly builds. 
I have a few ideas.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Today's builds have been blocked from modifying data on b.m.o.

Comment 19

[Tracy Walker [:tracy]]

is this Sun Solaris only?  if not, please update platform/OS.

Comment 20

[kysmith0 (gone)]

Downgrading to critical since JKaiser is on it. 

Comment 21

[kysmith0 (gone)]

This is causing data corruption in bugzilla so it's once again a blocker. 

Comment 22

[Boris Zbarsky [:bzbarsky]]

I think I just saw this... but with the 2001-11-01-08 build.  This would be
before jkeiser's checkin.

Comment 23

[Johnny Stenback (:jst)]

Boris, you "think" you saw this? If yuo did, please confirm. I'm about to check
in a workaround for this problem.

Comment 24

[Johnny Stenback (:jst)]

Attachment: Proposed workaround, will make us broken in a way we were broken before the form changes landed.

Comment 25

[John Keiser (jkeiser)]

Comment on attachment 56320 [details] [diff] [review]
Proposed workaround, will make us broken in a way we were broken before the form changes landed.

r=jkeiser

Comment 26

[Johnny Stenback (:jst)]

Workaround checked in. Downgrading bug to critical since we need a real fix.

Comment 27

[John Keiser (jkeiser)]

Attachment: A Proper Fix

Comment 28

[John Keiser (jkeiser)]

OK, what's really happening is the frame is being destroyed so soon after it's
created that it never had a chance to grab its initial value.  The way text
elements work is, the text value is not grabbed until it absolutely has to
be--on first reflow.  (Comments purport to explain why, but I have not yet
grasped their full import.)  Well, when the frame is destroyed we have the it
tell the content its value so that it stays around (in case we create another
frame like we are doing this time).

In the case of DemoteContainer() the frame is often destroyed before the first
reflow even happens, so the frame's string is "" and it has never been
initialized to the initial value.  But, foolishly assuming that it already has
the value, the frame tells the content "here's my value, go ahead and run with
it!"  The text control blithely sets the value (as any good setter will do), and
the initial value is forever lost.  The next frame that comes up takes the blank
value.

I guess DemoteContainer is good for something--stress testing frames :)

My solution is simply not to call SetInternalValue() on the content unless (a)
there is cached state (i.e. somebody set the value and the first reflow hasn't
happened) or (b) the initial value has been grabbed (this is what mUseEditor
denotes).

Comment 29

[Boris Zbarsky [:bzbarsky]]

I said "think" because I saw the dependencies of a bug I commented on wiped
out... I could not reproduce it, though.  Of course I also could not reproduce
it with the builds that are known to have this bug... it seems to be a timing
problem from jkeiser's description.  :(

Comment 30

[John Keiser (jkeiser)]

Attachment: Even More Proper Fix

Comment 31

[John Keiser (jkeiser)]

OK, so I talked to sicking and he noted that this stems from a larger
problem--not specifying who owns the value of the control and when.  (If I had
said right out that content owned the value until initialization then this fix
would have been obvious from the start.)

So this patch does two things: first, it removes mCachedState from InputElement,
so that we only store the value 2 places (content/editor) instead of three
(content/layout/editor).  And second, it lays hard and fast rules for who owns
the value, when:

1. Content owns the value (it is canonical) when there is no frame.
2. Content owns the value before initial reflow of the frame (when mUseEditor
gets set).
3. Editor owns the value after initial reflow of the frame.

The flow of program control to decide where to get the value now goes like this:

CONTENT.GetValue():
1. If no frame exists, get value from content (CONTENT.GetValueInternal).
2. If no frame exists and no value exists, get default value.
3. If frame exists, call FRAME.GetProperty(value)

FRAME.GetProperty(value)
1. Call GetTextControlFrameState()
2. If mUseEditor is true, get value from editor.
3. If mUseEditor is false, get value from content (call CONTENT.GetValueInternal)

Control initially belongs to content.  When we switch between the two we first
get the value from one and then set it in the other.  We switch between the two
on initial reflow (content->editor) and on frame destroy (editor->content).

Comment 32

[Johnny Stenback (:jst)]

The latest patch in this bug was checked in, AFAIK this bug is FIXED.

Comment 33

[John Keiser (jkeiser)]

Looks good to me.

Comment 34

[Madhur Bhatia]

--

Comment 35

[Madhur Bhatia]

verified fixed on 
win2k buildID: 2001-11-19-06trunk
linux 7.1 buildID: 2001-11-20-08trunk