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

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Jean-Francois Ducarroz]

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!

Comment 1

[Jean-Francois Ducarroz]

Attachment: small xul file that reproduce the problem

Comment 2

[Jean-Francois Ducarroz]

fix typo into summary

Comment 3

[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 [e-mail], if you disagree change it back to
claudius and cc esther

Comment 4

[Peter Trudelle]

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.

Comment 5

[Peter Trudelle]

reassigning to shaver. Mike, this is dependent on the inline editing you are
working on.

Comment 6

[Alec Flett]

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?

Comment 7

[Jean-Francois Ducarroz]

Yes, I will give a try...

Comment 8

[Jean-Francois Ducarroz]

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?

Comment 9

[Alec Flett]

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.

Comment 10

[Alec Flett]

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.

Comment 11

[Phil Peterson]

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

Comment 12

[Phil Peterson]

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?

Comment 13

[buster]

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.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 15

[Alec Flett]

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.

Comment 16

[Jean-Francois Ducarroz]

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.

Comment 17

[buster]

I'll look into it.

Comment 18

[Jean-Francois Ducarroz]

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.

Comment 19

[Alec Flett]

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.

Comment 20

[Alec Flett]

Attachment: HTML and JS that demonstrates the problem.

Comment 21

[Alec Flett]

ok, that attachment shows exactly what's going on.. just load the HTML in viewer
or apprunner, and you'll see what to do.

Comment 22

[Alec Flett]

updating the summary

Comment 23

[buster]

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.

Comment 24

[Alec Flett]

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?

Comment 25

[Jean-Francois Ducarroz]

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

Comment 26

[buster]

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.

Comment 27

[Nisheeth Ranjan]

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.

Comment 28

[Nisheeth Ranjan]

Accpeting bug...

Comment 29

[rubydoo123]

removing link to Editor PDT+ tracking bug

Comment 30

[Jean-Francois Ducarroz]

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

Comment 31

[Nisheeth Ranjan]

I'm going to try to fix this next week: 11/15 - 11/19.

Comment 32

[Nisheeth Ranjan]

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

Comment 33

[Jean-Francois Ducarroz]

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

Comment 34

[Jean-Francois Ducarroz]

nisheeth, did you plan to solve the problem for all forms elements? because I have the same problem with select
elements (bug 18685)

Comment 35

[Jean-Francois Ducarroz]

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

Comment 36

[Nisheeth Ranjan]

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.

Comment 37

[Pierre Saslawsky]

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

Comment 38

[Nisheeth Ranjan]

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.

Comment 39

[Alec Flett]

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.

Comment 40

[Nisheeth Ranjan]

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

Comment 41

[Jean-Francois Ducarroz]

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

Comment 42

[Nisheeth Ranjan]

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.

Comment 43

[Nisheeth Ranjan]

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.

Comment 44

[David Hyatt]

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.

Comment 45

[Chris Waterson]

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?

Comment 46

[Chris Waterson]

Can I have one global counter, or does the count have to start at "0" for each
document? (Trying to avoid a member variable.)

Comment 47

[Chris Waterson]

Also, do XUL elements need to support this?

Comment 48

[Chris Waterson]

Attachment: proposed fix for XUL

Comment 49

[Chris Waterson]

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.

Comment 50

[Jean-Francois Ducarroz]

FYI: "s'il vous plait" or "S.V.P"

Comment 51

[Chris Waterson]

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.

Comment 52

[Jean-Francois Ducarroz]

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

Comment 53

[Chris Waterson]

Attachment: better fix

Comment 54

[Chris Waterson]

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.

Comment 55

[buster]

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!

Comment 56

[Chris Waterson]

all yours, buster.

Comment 57

[Chris Waterson]

(I should add that I checked in the "better fix".)

Comment 58

[Nisheeth Ranjan]

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

Comment 59

[buster]

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.

Comment 60

[buster]

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.

Comment 61

[Chris Waterson]

sounds like a frame level problem that has to do with tree widget destroying
and re-creating frames.

Comment 62

[Alec Flett]

nisheeth can do that better than I can
(and around it goes!)

Comment 63

[Chris Waterson]

per hyatt, this is his bug. he has a dup somewhere.

Comment 64

[Jean-Francois Ducarroz]

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

Comment 65

[Jean-Francois Ducarroz]

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

Comment 66

[Peter Trudelle]

removed extraneous [blocker] tag

Comment 67

[esther]

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.