Closed Bug 16709 Opened 20 years ago Closed 20 years ago

[DOGFOOD]dataloss because ender stores state in frames, not DOM.

Categories

(Core :: XUL, defect, P1, blocker)

All
Mac System 8.5
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: hyatt)

References

Details

(Whiteboard: [PDT+] 12/15)

Attachments

(4 files)

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
QA Contact: claudius → esther
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 esther@netscape.com, 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.
Blocks: 14742
Blocks: 16841
Whiteboard: [PDT+]
Assignee: hyatt → shaver
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. ***
Blocks: 11091, 12658, 16950, 17432
Target Milestone: M11 → M1
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.
Target Milestone: M1 → M11
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.
Assignee: shaver → buster
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
Status: NEW → ASSIGNED
Target Milestone: M11 → M12
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.
Assignee: buster → nisheeth
Status: ASSIGNED → NEW
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.
Blocks: 17907
Status: NEW → ASSIGNED
Accpeting bug...
No longer blocks: 12658
removing link to Editor PDT+ tracking bug
*** Bug 18437 has been marked as a duplicate of this bug. ***
Blocks: 18437
I'm going to try to fix this next week: 11/15 - 11/19.
Whiteboard: [PDT+] → [PDT+] Should be fixed by 12/3
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
Blocks: 20771
Assignee: nisheeth → waterson
Status: ASSIGNED → NEW
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.
Blocks: 17460
Status: NEW → ASSIGNED
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
Attached patch better fixSplinter Review
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!
Assignee: waterson → buster
Status: ASSIGNED → NEW
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.
Assignee: buster → waterson
Status: ASSIGNED → NEW
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.
Assignee: waterson → alecf
sounds like a frame level problem that has to do with tree widget destroying
and re-creating frames.
Assignee: alecf → nisheeth
nisheeth can do that better than I can
(and around it goes!)
Assignee: nisheeth → hyatt
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
Status: RESOLVED → VERIFIED
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.
No longer blocks: 16841
No longer blocks: 17432
No longer blocks: 17907
You need to log in before you can comment on or make changes to this bug.