Closed
Bug 108175
Opened 23 years ago
Closed 23 years ago
INPUT loses value
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
VERIFIED
FIXED
People
(Reporter: axel, Assigned: john)
Details
(Keywords: regression, smoketest)
Attachments
(3 files)
955 bytes,
patch
|
john
:
review+
|
Details | Diff | Splinter Review |
1.48 KB,
patch
|
Details | Diff | Splinter Review | |
6.00 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•23 years ago
|
Keywords: regression,
smoketest
Comment 1•23 years ago
|
||
cc twalker on this smoketest blocker.
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=jkeiser@iname.com. sr=myself.
Reporter | ||
Comment 3•23 years ago
|
||
that was causing the bugzilla form to appear twice in the rendering. The build I tested this on was built with that checkin.
I saw the bug before the checkin, pulled it, rebuilt: bug gone. (Linux)
Assignee | ||
Comment 5•23 years ago
|
||
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.
Reporter | ||
Comment 6•23 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•23 years ago
|
||
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.
Reporter | ||
Comment 9•23 years ago
|
||
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•23 years ago
|
||
restoring lost values.Did this get into any nighyl build? If so, we need to block them from bz.
Comment 11•23 years ago
|
||
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•23 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 13•23 years ago
|
||
repulled - rebuilt - now what..
Comment 14•23 years ago
|
||
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•23 years ago
|
||
DOH..NOT set to fixed... and NOT modified.. yet the fields appear blank in the CVS build.
Comment 16•23 years ago
|
||
reassigning to jkeiser@iname.com
Assignee: rods → jkeiser
Status: REOPENED → NEW
Assignee | ||
Comment 17•23 years ago
|
||
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.
Status: NEW → ASSIGNED
Comment 18•23 years ago
|
||
Today's builds have been blocked from modifying data on b.m.o.
Comment 19•23 years ago
|
||
is this Sun Solaris only? if not, please update platform/OS.
Comment 20•23 years ago
|
||
Downgrading to critical since JKaiser is on it.
Severity: blocker → critical
Comment 21•23 years ago
|
||
This is causing data corruption in bugzilla so it's once again a blocker.
Severity: critical → blocker
Comment 22•23 years ago
|
||
I think I just saw this... but with the 2001-11-01-08 build. This would be before jkeiser's checkin.
Comment 23•23 years ago
|
||
Boris, you "think" you saw this? If yuo did, please confirm. I'm about to check in a workaround for this problem.
Comment 24•23 years ago
|
||
Assignee | ||
Comment 25•23 years ago
|
||
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
Attachment #56320 -
Flags: review+
Comment 26•23 years ago
|
||
Workaround checked in. Downgrading bug to critical since we need a real fix.
Severity: blocker → critical
Assignee | ||
Comment 27•23 years ago
|
||
Assignee | ||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
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. :(
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
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•23 years ago
|
||
The latest patch in this bug was checked in, AFAIK this bug is FIXED.
Assignee | ||
Comment 33•23 years ago
|
||
Looks good to me.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•23 years ago
|
OS: Solaris → All
Updated•23 years ago
|
Hardware: Sun → All
Comment 34•23 years ago
|
||
--
Comment 35•23 years ago
|
||
verified fixed on win2k buildID: 2001-11-19-06trunk linux 7.1 buildID: 2001-11-20-08trunk
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•