if you enter data into textboxes or textareas and then reload, or go forward/back to the form, the text is missing. this occurs with linux trunk build 2002110904, but not 2002110721. a testcase for this is attachment 105538 [details] (bug 179020)

brendan asked for this feature change :-)

I hope that's sarcasm :) This is real triaging catfood...

this is a regression from bug 124990. ==> bryner

more info on reproducing this: if you use the keyboard to tab into the text boxes, it works fine until you shift-reload. To get the bug to occur, you need to click-to-focus the textbox when you enter the text into the form. moving this out of history since keyboard works ==> Form controls

ccing jkeiser....

This bug is set as a blocker for bug 179020 - shouldn't it just be a dup?

no, the regression in bug 179020 is much older and requires pasting the text. this bug only requires mouse-focus.

At least on Bugzilla, I can reproduce this even with tabbing to focus.

It looks like what's happening is that the editor key listener (nsTextEditorKeyListener::KeyPress in nsEditorEventListeners.cpp) is calling preventDefault on keypress events during the first stage of event dispatch. Then when the event goes to the text control frame during the system event stage of dispatch, it doesn't set the "value changed" bit because preventDefault was called. So, the frame doesn't think it has any state to save. Unfortunately, I'm not familiar with any of this code. So, maybe someone can help me out here: - Why does the editor call preventDefault() on the key event? - Why does the editor need to listen for key press events in both event groups? Would changing it to only call preventDefault if it's the system event group pass break anything? - Why does the text control frame skip setting the value-changed bit if preventDefault was called? - Is it correct behvaior for the preventDefault state to carry over from the first dispatch to the second? (I'd suspect so, since the default action would happen after _both_ event groups had been processed, right?)

Actually, the PreventDefault was added by joki's patch, and I think it's wrong.

This fixes the problem at hand by not checking PreventDefault(). What I can't figure out is why it was added in the first place.

I see some places in textarea that are likely not calling PreventDefault as well (specifically enter and escape), this is a note to self to investigate, not related at all to this bug. bryner, it seems best to me that the editor only be done on the system event loop, but I haven't investigated that yet. Editor folks, it seems to me that a better solution to this problem overall would be to listen to the textarea for changes and set this flag then. Would nsITransactionListener::DidDo() / Undo / Redo be the right place?

I believe there indeed was an RFE that asked for the form state to be reset on Reload (or at least Shift-Reload), which is IMHO reasonable (sometimes you do want to reset the page and Shift-Reload seems like a natural thing to do). However, the data should definitely be preserved on "Back".

I'm not sure if the patch jkeiser attached is really intended to fix this bug or not. I'm not sure which case of "PreventDefault()" being called is referred to in comment 10 and comment 11. In comment 9, bryner asks about nsTextEditorKeyListener::KeyPress calling PreventDefault. This is done to prevent things like pressing spacebar in a textarea and having the contentArea also handle that event. There are also other cases having to do with platform-specific keybindings as well as caret navigational keyevents like arrow presses, pageup/down, home, end, delete, etc. It is not clear to me why this code should need to be changed at all. If the editor is handling the event, shouldn't it always set the flag so others know it was handled? I don't understand why we'd want to only do this some times. How would that affect embedders? In comment 9, bryner asks: >- Why does the text control frame skip setting the value-changed bit if >preventDefault was called? I'd also like to understand this. This seems like the real problem imo. In comment 12, jkeiser comments on the issue of other missing "PreventDefault" cases. I have a bug on that but it doesn't have a complete patch attached at the moment. The bug is 172174. I never finished porting over the patch from the chimera branch to the trunk. jkeiser--if you'd like to grab it, please feel free!

Comment #11 does indeed fix this bug. I believe that even if we switch both editor and text control frame to do their thing on the same event pass, this problem will still occur, because text control element is a child of the control and therefore gets key events earlier in the bubble cycle. Regardless, we should stop using the event to guess whether the editor called PreventDefault and let the editor *tell* us if it handled the key. Perhaps it's possible that someone else called PreventDefault() during the capture stage, in which case we'd erroneously think the text editor did it. Re bug 172174, you have made me aware of it so I have been adding PreventDefault() as I go along. One day it will be everywhere :)

So, what happens if there is a keypress event that doesn't correspond to a typed character in the editor (e.g. a menu item shortcut). In that case we _don't_ want value changed state to be set, right? However, it looks like the value changed bit is just an optimization to avoid trying to save the state, so nothing "incorrect" will happen if we set it a bit overzealously. I think the _best_ approach would be some sort of notification directly from editor that the text in the field has changed. That would also take care of bug 179020. brade, does nsIEditActionListener sound like the right thing to use?

Right. Not only that, but text inputs currently miss click actions like Paste and consequently will not restore if you just paste into a control. This patch: (1) uses the TransactionListener (do/undo/redo) to check whether the editor has changed state (and new, improved--it returns ValueChanged to its pristine state when the undo level goes back to zero!) (2) uses the TransactionListener to send "input" events so that we don't have to have two interfaces for that (3) removes the KeyListener since we no longer check for that (4) removes a boolean that was not really being used

Comment on attachment 106685 [details] [diff] [review] Patch v2.0 >+nsTextInputListener::DidDo(nsITransactionManager *aManager, >+ nsITransaction *aTransaction, nsresult aDoResult) > { >- // we only need to update if the undo count is now 1 >+ mFrame->InternalContentChanged(); >+ mFrame->SetValueChanged(PR_TRUE); >+ >+ // If this is the first do, we need to update undo/redo items, but otherwise >+ // we don't need to update squat > PRInt32 undoCount; > aManager->GetNumberOfUndoItems(&undoCount); >- if (undoCount == 1) >- { >- if (mFirstDoOfFirstUndo) >- UpdateTextInputCommands(NS_LITERAL_STRING("undo")); >- >- mFirstDoOfFirstUndo = PR_FALSE; >+ if (undoCount == 1) { >+ UpdateTextInputCommands(NS_LITERAL_STRING("undo")); > } >- >+ >+ mFrame->InternalContentChanged(); >+ > return NS_OK; Why do you need to call mFrame->InternalContentChanged() twice?

I don't :) That was a result of an earlier indecision as to when to fire the event--I ultimately decided "as late as possible" so that the state was all consistent when people ran in JS.

OK, this uses EditAction() instead of TransactionManager(), which gives us even more bloat reduction. There is some new logic in there that checks whether we really need to change the Undo and Redo menu, so that we don't call out into JS on every keypress in chrome. I also cleaned up a few places where the code was doing excessive returns and using outdated return syntax, and removed some "virtual" keywords from some methods in this class, which has no subclasses ;)

this works out input, showing when input and change fire, and has a button to set the value with JS to verify that that behavior does not change.

Comment on attachment 106727 [details] [diff] [review] Patch v2.1 ==== Copy/Paste error? This should use |numRedoItems|: + manager->GetNumberOfRedoItems(&numUndoItems); ==== This certainly is an interestng way to convert a number to a bool. :-) I'm wondering if it would be clearer to others if you just made it |!=0| so that someone doesn't mistake it as a typo in the future. :-) Also if you converted your num*Item locals to bools earlier, you can reduce your 4 |if| expressions above to 2 xor expressions. + mHadUndoItems = !!numUndoItems; + mHadRedoItems = !!numRedoItems; ==== So what was all that code you removed in |KeyPress()| for?

Fix nits - I am used to !! but in this case it does look better with != 0. Kin: the KeyPress() stuff there was *solely* a heuristic to guess whether the key event caused a change in the textarea. The SetValueChanged() in EditAction() fully replaces it.

Taking.

Really taking

Comment on attachment 106749 [details] [diff] [review] Patch v2.1.1 >@@ -2930,15 +2739,24 @@ > event.message = NS_FORM_INPUT; > event.flags = NS_EVENT_FLAG_INIT; > >- // Have the content handle the event, propagating it according to normal DOM rules. >+ // Have the content handle the event, propagating it according to normal >+ // DOM rules. > nsWeakPtr &shell = mTextSelImpl->GetPresShell(); > nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(shell); >- if (!presShell) >- return NS_ERROR_FAILURE; >+ NS_ASSERTION(presShell, "No pres shell"); >+ if (!presShell) { >+ return; >+ } >+ Really small nit, but... if you're going to null check presShell anyway, maybe change it to NS_WARNING and move it inside the if block. > nsCOMPtr<nsIPresContext> context; >- if (NS_SUCCEEDED(presShell->GetPresContext(getter_AddRefs(context))) && context) >- return presShell->HandleEventWithTarget(&event, nsnull, mContent, NS_EVENT_FLAG_INIT, &status); >- return NS_ERROR_FAILURE; >+ presShell->GetPresContext(getter_AddRefs(context)); >+ NS_ASSERTION(context, "No pres context"); >+ if (!context) { >+ return; >+ } >+ Same thing here. r=bryner with those changes.

Comment on attachment 106749 [details] [diff] [review] Patch v2.1.1 sr=kin@netscape.com

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

Fix checked in.

Fix didn't appear in the 20 November nightly build.

It works for me. Could you be more specific?

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

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

I generally don't have a problem with forms losing their data, but while submitting a bug lately, I forgot to choose a component and got a big, ugly, red error. When I went back, my carefully crafted submission was gone, and I had to retype it all. I haven't been able to reproduce it.