Closed Bug 16709 Opened 20 years ago Closed 20 years ago
[DOGFOOD]dataloss because ender stores state in frames, not DOM
1.08 KB, text/plain
1.57 KB, text/html
2.88 KB, patch
|Details | Diff | Splinter Review|
3.71 KB, patch
|Details | Diff | Splinter Review|
If you enter several recipient (2 or 3) in the addressing Widget of the message compose window (press enter at the end of each one to get a new line) and then scroll down and then up to come back to the first recipient, the content of each the edit fields are gone. It's not just a display matter, their are really empty! I have created a small XUL file that reproduce the problem: 1) Open the xul file addresswidget.xul (that I will attach latter to this bug report) into the browser 2) type something in the first input field. 3) type something in the second input field. 4) scroll up 5) both field are now empty!
Summary: [Blocker] MsgCompose Adressing Widget loose data when scroll up! → [DOGFOOD] [Blocker] MsgCompose Adressing Widget loose data when scroll up!
Summary: [DOGFOOD] [Blocker] MsgCompose Adressing Widget loose data when scroll up! → [DOGFOOD] [Blocker] MsgCompose Addressing Widget looses data when scroll up!
fix typo into summary
Note this is on all platforms, there are other bugs for the Addressing pane flakiness bug 15001 & bug 15002. This bug sound like it's part of 15001. Changing QA Contact to firstname.lastname@example.org, if you disagree change it back to claudius and cc esther
Assignee: trudelle → hyatt
Priority: P3 → P1
Target Milestone: M11
reassigning to hyatt as p1 for m11. possible dup? possible workaround: use a table instead of a tree, the tree optimizations discard the contents of the input fields, and anything else that isn't reflected in the content model.
reassigning to shaver. Mike, this is dependent on the inline editing you are working on.
hyatt and I were talking about this, here's the explanation as I understand it: ender keeps state in the Frame for the widget, and the tree widget destroys frames. One possible solution is to have a special "mode" for the tree widget (perhaps the default) so that Frames are stored when in this mode... Another possibility, what shaver is I think attempting, is to use titledbuttons instead of the ender widget, and then redesign the titledbuttons to have an "edit" mode... so that the "edited" content is stored in the titledbutton's DOM element as an attribute or something. Personally, it really seems like it would be much more worth our time to work around this by storing the ender widget contents in the content model, at least for dogfood. I think the first solution is alot of work for very little gain. I think the second solution would be great, but I still much more work than trying to work around it for dogfood. Jean-Francois, could you store the value of the ender widget in the DOM during the onchange= or onblur= event?
Yes, I will give a try...
When I define a value for the input field directly into the xul file, Ender will use this value as the default value even when I scroll. But if I set the value of the field myself from JS (inputField.value = "something else"), Ender still reset the content of the input field to the value defined in the xul when you scroll. Do I have another way to force the default value to be changed to something else from JS?
Oh, crap. I see what you're saying. Shaver, whats the ETA on the titledbuttons rearchitecture? That would at least solve this for a while.
JF, how about doing the titlebutton work ourselves? Here's what you'd do: - instead of an ender widget, use a titledbutton. - to "activate" a text field, you would set the "display" style of the titledbutton to "none" and insert a <input type="text"> field just before or after the titledbutton - in the onblur= handler for this text field, you'd save the value of the widget in the titledbutton, and then destroy the ender widget.
*** Bug 15001 has been marked as a duplicate of this bug. ***
I think we need to find a way to get traction on this issue in the next couple of days. Alecf, ducarroz and shaver (?) can you guys huddle on this?
hyatt and I had a discussion about this tuesday. At that time, we discussed several options, the most promising of which were: 1) the option alecf posted a few minutes ago 2) using nsIStatefulFrame API on nsGfxTextControlFrame to set/get edit state when the tree control destroys and recreates frames during scrolling I'm available to consult on either option or more brainstorming if necessary, and of course add any reasonable features to the text control to support you guys.
What I'm working on right now (slowly, I fear) is splitting <titledbutton> into two new frame types, one for croppable-text-with-accesskey and one for image. After that, croppable-text-with-accesskey (working title: <textlabel>) is the place to put all the awesome editing work. Last I heard at the porkjockeys meeting I tele-attended, though, people were going to look at ways to use text fields in the tree widget by making the frame-destroy-when-scrolled-away behaviour optional. Hyatt?
Steve - I just asked this in email, but shouldn't the text field be storing it's value in the DOM anyway? Does this mean if I have a form on a web page and I set it's "display" style to "none" and then back to "inline" (thus destroying and recreating the frame) that we'll loose the value of the widget? This seems wrong to me.
The same problem occurs when you collapse and then expand a toolbar with input field like the URL field in the browser or with any input fields in the message compose window.
I'll look into it.
More Info: With the browser's URL field, it's a little bit different. It seems that every time a page is loaded, the curent URL is correctly set in the DOM therefore it want be lost. But if i type "hello" into the input field (without pressing enter), then I collapse and extend back the toolbar, the input filed contains again the current URL.
Ok, ducarroz and I talked in person, and we came to the conclusion that there is no decent work around. I'm writing up a test case right now to make this easier to diagnose, as well as demonstrate the real bug. I'll attach it here in a few minutes. I'm reassigning to buster for right now because this is really an ender bug.
ok, that attachment shows exactly what's going on.. just load the HTML in viewer or apprunner, and you'll see what to do.
Summary: [DOGFOOD] [Blocker] MsgCompose Addressing Widget looses data when scroll up! → [DOGFOOD] [Blocker] dataloss because ender stors state in frames, not DOM.
updating the summary
I need to confer with Vidur and Kipp about the legality of storing runtime data in the content model. For example, what does it mean for multiple presentations of the same content? This certainly can't be addressed in M11.
My take on this is this: anything that is reflected into the DOM is not display-time data, (which is what I assume you mean by "runtime" data) should be stored in the content model, and should be reflected in all presentations. What happens if I go to print a document that has a text field? Won't there be a different PresContext, and thus a different frame associated with this node, and thus no value available. This means that if I type something into a form, and then go to print the form, that none of the data I entered into the form will print?
*** Bug 17832 has been marked as a duplicate of this bug. ***
Summary: [DOGFOOD] [Blocker] dataloss because ender stors state in frames, not DOM. → [DOGFOOD] [Blocker] dataloss because ender stores state in frames, not DOM.
I think this is a symptom of a larger problem, that not all cases of frame destruction in layout make use of nsIStatefulFrame. That problem must get solved correctly for a whole bunch of reasons, 2 of which are described above. Vidur's recent work to make page loads incremental has uncovered a few more reasons why this is needed as well. So, as long as that work needs to get done anyway, then some of the stop-gap measures mentioned above sound like throw-away workaround code to me. Seems we should solve this right, in the bowels of the layout system. cc'd a bunch of layout guys who were all involved in a discussion about this today.
I can take over this bug. The frame constructor needs to capture/restore frame state for cases where it deletes and then reconstructs frame hierarchies. Re-assigning this to myself.
removing link to Editor PDT+ tracking bug
*** Bug 18437 has been marked as a duplicate of this bug. ***
I'm going to try to fix this next week: 11/15 - 11/19.
Updating. Coalescing reflow work and other PDT+ bugs on my plate mean that this will only be fixed in the week after thanksgiving: 11/29 - 12/3. Updating status whiteboard accordingly
*** Bug 18685 has been marked as a duplicate of this bug. ***
nisheeth, did you plan to solve the problem for all forms elements? because I have the same problem with select elements (bug 18685)
*** Bug 19345 has been marked as a duplicate of this bug. ***
Jean-Francois, select frames already save/restore their state for session history back/forwards so they will probably be able to do it for this new case also.
*** Bug 16212 has been marked as a duplicate of this bug. ***
OK, I have code in my tree that fixes the test case that Alec attached. There are three different functions in the CSS frame constructors that do frame destruction where we want to capture frame state: RecreateFramesForContent(), ReconstructDocElementHierarchy(), and RemoveMappingsForFrameSubtree(). I've fixed up the RecreateFramesForContent() function so that it saves the state of the frame subtree before destroying it. I've also changed a bunch of other frame construction functions so that they restore state on frames after creating and initializing them. Since Alec's test case ends up calling RecreateFramesForContent(), everything is happy for that case. What remains to be done is: - save frame state in ReconstructDocElementHierarchy() - teach the tree row group frame that uses RemoveMappingsForFrameSubtree() to save frame state. I'll work on these two items tomorrow. Once that is done, the problems in the mail compose addressing widgets should get resolved too.
wow, this is great! Let me know if you need help in the tree widget. It looks like there's about 5 places in the rowgroupframe that destroy child frames using RemoveMappingsForFrameSubtree.... I'd like to factor out the whole save-state thing into one function instead of distributing it to these 5 places.
*** Bug 19508 has been marked as a duplicate of this bug. ***
*** Bug 18437 has been marked as a duplicate of this bug. ***
I have fixed the two other items that were remaining as of my earlier update on 11/30. Now, the tree row group frame saves/restores state for its child frames. The mail compose's addressing widgets no longer lose email addresses if you scroll. I'll check this in once I get a code review.
Whiteboard: [PDT+] Should be fixed by 12/3 → [PDT+] Getting fix code reviewed
I've checked in my changes, but, there is one more thing that needs to happen before HTML objects inside XUL/XML contexts can save/restore state. I had tried a change that made this work but broke restoration of frame state during session history back/forward. The current status is that cases that use HTML content inside an HTML document (like Alec's test case) save/restore their frame state properly. But, HTML content inside XML documents, XUL documents, and XUL overlays do not save/restore their frame state. The way to fix this is described below. HTML content objects get created within HTML documents as well as within XML/XUL documents and XUL overlays. A monotonically increasing content ID needs to be assigned to these content objects such that the set of content objects on a document each have a unique ID. These IDS are used as part of the key into a hashtable that stores each content object's frame state. Currently, content IDs are only being assigned to HTML content objects in an HTML document. Once the other places that create HTML content objects also start assigning content IDs, this bug will be fixed. I can make the changes necessary for HTML inside XML, but, as per a talk with hyatt, I think waterson is the right person for doing the changes for HTML inside XUL documents and XUL overlays. Since the more immediate problem of the mail compose addressing widget will be fixed by the "HTML inside XUL" changes, I'm re-assigning this bug to waterson. I'm creating bug 20872 to remind me to make these changes for HTML inside XML.
The ID counters should live on master documents, so e.g., navigator.xul makes hTML content 0,1,2 and then global overlay makes 3,4,5 but in messenger.xul we make HTML content 0,1 and then global overlay makes 2,3,4 In other words, by making it live on the master doc we ensure that there isn't any mistaken sharing of state among overlays that are used in different master docs.
So if I understand this correctly, HTML element construction inside a XUL document now goes something like this: 1. Create HTML node "foo" 2. Ask XUL document for next serial number 3. Call foo->SetContentID(serial number) Is this correct?
Can I have one global counter, or does the count have to start at "0" for each document? (Trying to avoid a member variable.)
Also, do XUL elements need to support this?
proposed fix attached; hyatt, code review sil vous plais? I made numbers global, not per document. Hope that's ok (don't see why it wouldn't be). Hacked nsXULElement to implement [Get|Set]ContentID(), too.
FYI: "s'il vous plait" or "S.V.P"
Unfortunately, I can't verify whether this fix works or not: the XUL test case doesn't allow click-to-focus in the entry fields (another bug?), and the addressing widget is currently busted s.t. "enter" does not add a new addressing line.
I've just checked in a fix for the addressing Widget problem that didn't add a new row. Pull mailnews/compose/resources/content/addressAutoComplete.js
Okay, a better fix. This one uses NS_CONTENT_ID_COUNTER_BASE, and tries to ensure that content IDs are consistent from load-to-load in a XUL document. Of course, this'll fall all over the floor when people start to modify overlays. Oh well. Anyway, with this patch, I am able to see edit fields recover their state in the addressbook widget WHEN YOU CLICK IN THEM. In other words, I can scroll edit fields off the screen, and then back on, and their content is restored as soon as I click into the field. So let's check these fixes in, and then get an editor person to figure out why the content doesn't paint until a click is received.
when this is all checked in, if the only remaining problem is that the text in the control doesn't display until the text control gets focus, assign it to me. great work, guys!
all yours, buster.
(I should add that I checked in the "better fix".)
*** Bug 19093 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Whiteboard: [PDT+] Getting fix code reviewed → [PDT+] 12/15
There is no way I'll be able to get this fixed by tonight's deadline, since I'm in Mountain View today. Let's leave it at M12 for now, and if jar and chofmann give me approval I'll check it in by 12/15.
The problem is the text control frame is never getting an inital reflow, only 2 resize reflows. Text control frames, and probably lots of other frame types, depend on getting a reflow with (eReflowReason_Initial == aReflowState.reason) after construction. I tested this by hiding and showing the URL bar in the browser, and setting a breakpoint in nsGfxTextControlFrame::Reflow(). Assiging back to waterson.
sounds like a frame level problem that has to do with tree widget destroying and re-creating frames.
nisheeth can do that better than I can (and around it goes!)
per hyatt, this is his bug. he has a dup somewhere.
*** Bug 21398 has been marked as a duplicate of this bug. ***
*** Bug 21398 has been marked as a duplicate of this bug. ***
Summary: [DOGFOOD] [Blocker] dataloss because ender stores state in frames, not DOM. → [DOGFOOD]dataloss because ender stores state in frames, not DOM.
removed extraneous [blocker] tag
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Using build 1999121309m12 on win and build 1999121308m12 on linux this is partilaly fixed using the original scenario. The names don't disappear when scrolling but if you scroll in order to get to the 4th addressing line (currently you can't get to it after typing in the 3rd address by pressing <Enter>) and add another name only the last 2 names acutually get the message. I have checked all the other duplicates and they refer to displaying of the names in the addressing pane not actually sending and receiving. I will verify this one an open a new bug.
You need to log in before you can comment on or make changes to this bug.