Overview Description: The HTML4 spec (section 17.13.2) says form controls with style="display: none;" should be successful controls, but Mozilla does not submit them to the server when the form is submitted. Steps to Reproduce: 1) Load the test case. 2) Click on the two form submission buttons, one after another, and watch the URL to see how one form submits the text field name=value pair and the other one (with style="display: none;") does not submit the pair. Actual Results: name=value pair not submitted Expected Results: name=value pair submitted Build Date & Platform Bug Found: Linux 2000-03-27-11 Additional Builds and Platforms Tested On: none Additional Information: attaching test case with quotation from relevant section of HTML4 spec

reassigning

The bug is valid and the report is definitive. Adding 'testcase' keyword. This occurs on 2000-04-09-08 Win98 build as well - changing OS to All.

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

This bug could cause dataloss in form submission. Nominating for beta3.

Marking nsbeta3-. Added html4, relnote keywords

Moving bug marked nsbeta3- to Future milestone.

This also happens on Mac (build 2000-08-14-13), so I'm changing Platform to All. Nav 4.x has this bug and so does Opera 4.02. However, in IE 5.5 (Win32) it works perfectly - adding '4xp' keyword to reflect the fact that a competitor is ahead of us here.

Updating QA contact.

Adding rtm keyword.

Marking rtm-. There is not enough time to fix this problem before rtm, and acoording to Antti Näyhä comment's Nav 4.x and Opera also contain this bug.

Why is it OK to ship a browser with a known data loss bug just because an old browser (NN4) and a rather insignifigant browser (opera) have it as well? ********* THIS BUG CAUSES DATA LOSS ************ According to the bug reporting tools and guidlines, that makes the priority CRITICAL. I am updating the severity accordingly. I would like to ask for a re-evaluation for RTM of this bug based on severity. David OK, bugzilla won't let me change severity. Drat :) Still, I would like to encourage a re-think on this bug.

I'll renominate for you, but I have to tell you the chances are slim. The main reason for rtm-'ing this bug is not to be backwards compatabile with Nav, but because there are not enough resources (time) to fix this before final lock down for the product. Since there are existing browsers with this problem, and no known real-world testcases, the potential for dataloss is hypothetical, not actual, and fairly low probability.

Here's a real world example of data loss I have built two web applications that have run into this bug. I send XML to the browser and store it in a textarea that I would like to have display:none. I then use a javascript XML parser to fiddle with the XML- adding subtracting and other neat stuff. I then place the resultant XML back into the textarea and submit the form. The server grabs the XML out of the text area and processes it with its XML processor. Having to use the hack to get around this bug can (on some browsers) leave a small artifact of the element I'm using as my datastore. I can't use display:none because then I don't get my XML sent back to the server. David

Removing '4xp' keyword since the keyword description doesn't mention competitor products anymore.

Adding dataloss keyword, setting severity to major since this is an obstacle for developers. Nominating for mozila 0.9.

We hit this problem as well. Here's a simple scenario. We have a multi-page wizard implemented as multiple DIVs which show/hide one after the other. When the user submits the last page, the form does not post all of the gathered information.

Fixing whiteboard to [rtm need info] since [rtm] has no meaning. Is this possibly an rtm- bug by now? Who could help with this bug?

Marking rtm-. Not enough time to fix for rtm.

I have a real world example as well, similar to the wizard as stated in another comment. A user fills out a form that is multiple tabs/pages where only one tab is shown at a time. When the user submits, only the form elements in the tab that is displayed are sent. This is a very critical bug in my eyes. Especially as more and more people are using DHTML.

So what's going on with this bug? Is there any chance of it getting fixed in a patch in the near future?

This is a very serious bug. Anychance of getting it fixed?

The same problem also with other element's !! look at bug 52334 'Problems with contentDocument and 'display:none' in IFRAMES.'

I have issues with this bug. I have reproduced it accidentally in 4 different web applications used by many major corporations, who really want to be able to use Netscape, but until this bug is fixed i have to direct them to a more stable browser that will not lose their critical data. To Tell you a little more about the bug I have isolated it to where I can make the <input type= "text"> data moves fine if I make the <input type="hidden"> and on the back button click I lose my hidden information that is critical to keep track of where data should go.

I agree, this is serious. As a side note, Heath, I think you may be actually running into bug 55988, which is different from this bug.

This problem is also covered by bug 57269, it seems.

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

Setting milestone to mozilla0.9.1. Can someone past the URL's of sites that are failing. If this problem is widespread enough we will consider changing the milestone to mozilla0.9

Not to be sarcastic, but you've got to be kidding. We've spent man-months (I'm not kidding) rewritting perfectly good forms, to make Netscape 6 work. Also, hidden iframes are the most basic way to communicate to your server in the background. We very badly want to be a great service for non-IE users. But make the pain high enough and I may give up.

Kevin, let's think outside the box for a moment here,OK? You are a web developer who wants to build a decent interface for your application. This interface involves maybe some XML data, a tabbed interface wizard and other information that you want to hide from the user but send to the server (state information for example). How would you do that? You would use elements with display:none in almost all cases. Why? *BECAUSE THAT'S WHY THEY'RE THERE* At this time, Netscape/Mozilla is forcing its developers to hack and work around this stupid bug (I've probably spent 2 man months working around it myself) while at the same time listening to the hype that "Mozilla is a platform". Bull****. This bug is a blocker and is really getting on my nerves. How can you justify *not* fixing it? You aren't going to find many top 100 sites with this problem because either 1) they are designed to work with 3.x browsers and are really read-only 2) A developer (like me) has spent the lengthy amount of time to work around the problem 3) A developer has decided that Netscape can simply go to hell and only lets browsers that are stable and work as expected in (IE for example.....). Come on Folks. Developers are your front line in your battle for relevancy. Please don't drive us away by making your platform a pain in the **** to work on... David

David, you're absolutly right. Score:5, Insightful :) I've developed an intranet application and had to exclude mozilla based browsers due to potential dataloss/datacorruption. Working around this bug has been a to big pain in the ass.

Same goes for us. We have pretty much decided to wait until this bug gets fixed to even bother going after Netscape customers. Right now we have generally been telling any customers that do use Netscape to switch to IE because Netscape 6 is flawed. And I really don't like telling people to use MS products. I would much rather say the opposite. Just my 2 cents.

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

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

Eric, is there anything we can do to help getting this bug fixed sooner? Or are there any silent dependencies this bug is waiting for? If so, can you please make them explicit? If not, where would you start looking in the source code? I found http://lxr.mozilla.org/seamonkey/source/layout/html/forms , but maybe you can suggest a more specific starting point? Kevin, it looks like you haven't been on the cc list. Many people think you are underestimating the severity of this bug. Is there any chance to retarget this one back to mozilla 0.9? Or can you commit that this is a hard mozilla 0.9.1 stopper? Otherwise please consider marking this bug helpwanted and giving a short description how to help.

I should add that when I nominated this bug for mozilla0.9 that milestone was supposed to be the _first_ one following the NS6 release. In the meantime, at least two milestones (0.8 and 0.8.1) have been inserted before 0.9, and I was just too lazy to update the nomination.

Bug 60893 "display:none affects JavaScript access to form fields" may be related. If these bugs have the same root cause, then we have a problem: > ------- Additional Comments From Kevin McCluskey 2001-03-01 17:32 > > Setting milestone to future. This will be addressed when the XBL > implementation of the form elements lands. Maybe bug 57209 "Rewrite HTML form controls using XBL" is what you are referring to here, Kevin? That bug is currently targeted at mozilla1.1, whenever that will be. However, I don't think this bug can wait that long. So, someone please clarify the relationship between this bug and bug 60893 and bug 57209.

The 'layout' frames, which are in layout/html/forms, are where all the form submit and form element state logic is currently implemented. The problem is that when a form element has style="display:none", there are no layout frames created for that element. Thus, when we gather up the form control state for submission and query all the frames, any element that has style="display:none" will be skipped. On the other hand, the 'content' for the element will always be present, even if the element has style="display:none". The content is located at content/html/content. As you can see, to get this bug fixed, any code relating to form submission and form state should be moved from layout/html/forms to content/html/content. I've started with this process, moving the logic in nsFormFrame.cpp to nsHTMLFormElement.cpp (in my local tree) though it is going to be a rather large change and will take some time to finish. I *hope* to have this done before 0.9 leaves, but I probably won't check it in until early 0.9.1 because of the risk of regressions. If you have some experience with building mozilla, it might be possible to help this land sooner if you could test out patches that I post against regressions. Or, if you are comfortable with the code, let me know and we can coordinate efforts to get this done in parallel! :)

Eric, I've build mozilla a couple of times so I'm game to helping you out if you need me. I may take a while to get up to speed, but I have a vested interest in getting this to work :) I may need a little hand holding via AOLIM to get me started, but after that, I'll help as much as I can. Please contact me at djoham@criadvantage.com when you are ready. One quid pro quo however :) In doing your fixes, please make sure you address bug 55987 as well which is another hokey display: none problem - this time with IFRAMES. Also, there is another bug somewhere about changing state of an item with display:none and loosing the new state when the display is set back. PLEASE PLEASE PLEASE tell me your fixes will help these bugs as well. It sounds like they will... Best regards, David

Hmm, well the iframes problem (bug 55987) is another issue altogether! The problem there is that for framesets / iframes, when they have style="display:none", the 'layout' frame is also not created, which in that case means that the document is not created, and if there is a src=, the page is not even loaded from the server! The fix there might be similar - move the document creation / page loading logic from html/document/src/nsFrameFrame.cpp to content/html/content/src/nsHTML[I]FrameElement.cpp, but there is a much greater chance of "issues" (performance, layout quirks) with that fix... I'd be happy seeing that tested a *lot* before it became part of the builds! > Also, there is another bug somewhere about changing state of an item > with display:none and loosing the new state when the display is set back. This will be fixed if I get this bug (34297) fixed right. :)

Thanks for your detailed response, Eric. I'm happy to see that - you already have a plan and - you already have started working on this, so the current target milestone is realistic. After taking a look at nsFormFrame.cpp I think this is probably a number too big for me :) But when you have a patch, I will probably be able to produce a linux build with it, and try a few forms.

Oi, have to push this one more milestone - running out of time. I will continue to work on this and attach a patch asap...

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

It sounds like this is sufficiently risky that it's not going to make it into NS 6.1. That's too bad, because it means it'll be another N months before I can tell people (customers and others) that they really should be using Netscape 6. I think this bug is the last one that makes Netscape unusable with my product; when it's fixed, I'll be delighted to be able to recommend Netscape, not only as an alternative to IE, but also so we can claim support for platforms other than Windows. So, assuming the fix won't be in before Netscape branches for 6.1, I'd suggest that this get fixed ASAP thereafter. It's risky, so it makes sense to put in the patch early in the next cycle so it has plenty of time to get tested before the next milestone (as Eric has already indicated). I'm trying to make the case that this bug is too crippling to delay any longer, and it really should get fixed in 0.9.3 - and certainly *before* Mozilla 1.0. Like djoham, I'm game to attempt to build with any candidate patches for testing.

Rockin! I just blew away the text of one of my bugs because of this bug! Any traction on it, 16 months later?

Missed 0.9.3.

I'm investigating this bug because it is killing me on all my new sites, where I like to use forms that expand and contract, or multi-page forms that are really all one page with a lot of javascript. It looks as though the main problem is that nsFormFrame::ProcessAsURLEncoded and nsFormFrame::ProcessAsMultipart work by walking the descendant frames of the form control frame. This isn't right because form controls without frames won't get submitted. I think the answer could be as simple as walking the DOM descendants of the form node. But, I need to know something. If a form control has display: none (or visibiilty: hidden, i suppose), is it true that it has no control frame whatsoever, or simply that its control frame is not a child of the primary frame? Also, what method is used to traverse from a DOM node to a control frame? The risk here seems manageable because the two methods in question are only called from one place: nsFormFrame::OnSubmit.

Jeff et al: some more info. The DOM itself does not appear to hold this information (unless for some reason it doesn't want to give it to me from JavaScript). I have written a testcase, modified from Myk's, that pops up an alert onSubmit that shows the value of the form.foo. It turns out the HTMLInputElement does exist in the form, even though display=none, but *the value is not set*. Does setting display:none explicitly remove the values of form elements? Or does display:none prevent retrieval of the value? Or maybe (this is my bet) the value is retrieved directly from the HTML control, and if the control isn't there, the value can't be retrieved.

Even more info, from a more detailed testcase: 1. The DOM value of the control is not set at startup if it is not displayed. 2. Unhiding the control sets the DOM value. 3. Re-hiding the control leaves the DOM value around. 4. Setting the DOM value before unhiding the control gives the control the DOM value as the initial value. I am assuming that re-hiding the control removes the control again, so I surmise that the DOM is not actually asking the control for the value when you access the DOM, but the other way 'round. Based on this, it appears that creating the control sets the initial DOM value based on the HTMLElement if it somehow detects that it hasn't been set before. The problem, then, would be twofold: Problem 1. VALUE attributes on inputs with display:none do not get set in the DOM until the control is created. Problem 2. Form::OnSubmit is walking the controls instead of the DOM. This bug covers problem #2, but both problems need to be solved to make the testcases work. Here's my analysis of the current problem (the way I came up with #2 up there). Preface: I'm new to Mozilla source, and anything I say could be wrong. I have looked in one file to see how things work, and run a testcases to see if I am right. Please correct me if I'm wrong anywhere. It looks like OnSubmit calls ProcessURLEncoded to grab the form data here: http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsFormFrame.cpp#751 ProcessURLEncoded appears to be walking the actual controls, and the information is invisible from there (note that it grabs nsIFormControlFrames, which I assume are actual controls): http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsFormFrame.cpp#1228 If someone will give me some info on how to walk the DOM in C++, I'd be happy to give a try at a patch for #2. I can't say how long it'll take me, but it's a nice, fairly simple (hopefully) intro to Mozilla coding. (Don't let me step on your toes if you're working on this, Jeff.) In fact, if someone can give me pointers as to how to fix #1, I might be able to help there too. It seems like (from how I would *guess* Mozilla works) the HTML parser is working fine and creating HTMLElements properly (and corresponding DOM stuff), but it's not setting default values of form elements when it does so. Should it be? Who *should* be?

After performing more exhaustive tests, I am more convinced my analysis is right. We are walking the controls instead of the DOM in OnSubmit, and the DOM value is not getting initialized until the control is unhidden/created. I also found out something interesting: if you even *fetch* the DOM value before you unhide the control, the control will not initialize the value when it is unhidden. I suspect that the fetch is "initializing" the value.

I think there is general agreement that the best way to fix this bug is to store the form values in the content, not in the layout frame.

It looks to me like we *are* storing the values in the content--the DOM--and we are just not walking the DOM in OnSubmit. I think this is what you were saying in an earlier comment, I was just out to prove it :) You can tell this because if you leave the control hidden, but set its value in the DOM, and then submit, the submit does not take the value. Heck, even if you unhide the control, and then hide it, the DOM value still exists but submitting has the same result--nothing. Only if the control is unhidden when the form submits does submission work correctly. The only case where we're not storing the values in the DOM is when there is a VALUE= in the form element and the form control is never created. Am I understanding correctly? And are you working on this patch, or do you have pointers to some place where I can learn how to work with Mozilla DOM?

OK. I've been traversing through the code and I think that we can solve this problem with the suggested fix. Other functions need it too, though. Specifically, OnReset needs it, and *all* radio button stuff needs it (if I understand the radio buttons functions' purpose). Radio button stuff might get hairy but I think it's necessary. We can't be searching for form data by looking at the visible controls on the form. Period. The Next Logical Step is to ask: do any other elements do this same thing? Elements that look at the values of other elements should *never* look at frames, only content. The only time they should look at frames is when they are trying to do layout. Another interesting question is what happens when the entire form is display=none? JavaScript can still submit the form. But if the frame is not there, will submit work at all? I definitely think this is a question for Another Time, Another Bug, but I figured I'd vent it so I didn't lose it. It seems to me we are not separating content and presentation as well as we could. As a side note, as far as I can tell I need to do GetContent() on the nsFormFrame and this will get me the DOM node for the form. I can then traverse to children. Is this correct, gurus? And Jeff, if you want to make the patch, pipe up. I'm still learning :)

Progress report. I am working on this patch actively now (weekends and some weeknights). I have catalogued all the code related to this and it looks like we don't need to move any data (member vars, etc.) from nsFormFrame into HTMLFormElement, and we can get the values we need from the HTML elements, so it should be happy. My proposed plan is this: 1. Change nsFormFrame::OnSubmit to walk the elements[] array of the HTMLFormElement. (From my testing this array has the elements even if display=none.) 2. Move OnSubmit (and attendant functions) into nsHTMLFormElement.cpp. Change corresponding base classes. (See below for reasoning.) 3. Move GetNamesValues() and GetMaxNumValues() from each of the control frames (like nsGfxTextControlFrame2) to the corresponding HTMLElement (like nsHTMLInputElement). Make nsIFormControl contain these functions. Things this leaves out that will need to be done for an *absolutely complete* solution to the problem: 1. Adding the form elements to the elements[] array initially *even if the frame controls are not created*. (I think this is the cause of the VALUE= problem.) 2. Verifying that form control frames are not storing the actual data in the frame instead of the content (DOM node). dbaron on IRC pointed out that this may not fix everything--some controls are not storing their values in the DOM, apparently :( But still, it will make display=none work on some elements, and it won't break anything existing (it will continue to work with all controls that have frames and will now work with some controls without them), so it is low-risk. Brings us closer to a fix and we can move on to fix those controls and the VALUE= problem for a complete fix. dbaron also brought up something about broken HTML possibly creating multiple forms out of one form /form pair (if I understood right). If this is happening, it's a problem, but I don't think this fix will bring us any more afoul of that than we already are. Why I'm moving OnSubmit() into nsHTMLFormElement: I want to fix another potential problem at the same time as this. Specifically, I want to make sure JavaScript can still submit the form if the form frame is not there (form display:none). If OnSubmit sits on the form frame, then of course it can't possibly be called :) This should be low-impact because the only place it is called is in nsHTMLFormElement anyway. (IsIndex has its own OnSubmit method, but it is totally unrelated.)

Hmm. BIDI is storing the submit direction and such in an object variable currently, instead of passing it around, even though it reinitializes the object variable every time submit() is called. I would like to see if I can't pass the charset and such around to whoever needs it. I am also moving Reset into the fix because it's pretty much up the same alley. Looks like I am going to have to move Reset() and IsSuccessful() from control frames to content elements as well--and I am changing IsSuccessful() to ShouldSubmit() unless I hear a reason why I shouldn't :) The purpose of that function is not apparent from its name.

> It looks to me like we *are* storing the values in the content--the DOM--and > we are just not walking the DOM in OnSubmit. Yes and no. Where the value is represented by an attribute in the content (e.g., 'checked' for a radiobutton), we store the value in the content model. On the other hand, sometimes the value is not represented by an attribute in the content model. (e.g., 'GetValue' for a text input returning the current text control value, but the 'value' attribute representing the solely the 'default' value in the content model) In these cases, the current value is stored in the frame itself, and the 'presentation state' when the frame is not around. The right approach is to walk the DOM on the submit, and grab values from the frame if it is around, presentation state otherwise; and in the cases where the value happens to be represented by a content attribute (or the frame and presstate are both not there), then just grab it directly from the content. > And are you working on this patch, or do you have pointers to some place where > I can learn how to work with Mozilla DOM? I have a portion of this patch in various stages in my tree. (Most recently, it was abandoned to finish some critical work for N 6.1, so it may be out of date) Pointers: The w3c specs are invaluable, especially when it comes to the quirks of implementing form controls. Our implementation of these lies largely in mozilla/content/html/content/ and mozilla/layout/html/forms. (I'm certain that is useless info now, but for posterity's sake...) > Other functions need it too, though. Specifically, OnReset needs it, and > *all* radio button stuff needs it Exactly. My first pass for the patch in my tree removed *all* of the code from mozilla/layout/html/forms/src/nsFormFrame.cpp - the task is mainly just to move it all into content, and replace nsFormFrame with a simple block frame in nsCSSFrameConstructor... > The Next Logical Step is to ask: do any other elements do this same thing? > Elements that look at the values of other elements should *never* look at > frames, only content. The only time they should look at frames is when they > are trying to do layout. Yes and no, as above, when the current state is not represented by an attribute, one will need to consult the frame to get the 'current' state, or if the frame is not around, check the presentation state. (If neither is around, falling back to the content.) > Another interesting question is what happens when the entire form is > display=none? JavaScript can still submit the form. But if the frame is not > there, will submit work at all? I definitely think this is a question for If a form element (nsHTMLFormElement) is around, but is display:none, there will be no corresponding frame (nsFormFrame), and since all the submit logic is currently in the frame, it will not submit at all. The logic could be moved to the content to solve this problem, but would ideally go in an independent service, like a 'form submitter service', as it is currently duplicated both here and in the plugins code. Your suggestion to leave it in nsFormFrame is fine as an interim solution to this problem. > dbaron also brought up something about broken HTML possibly creating multiple > forms out of one form /form pair (if I understood right). If this is > happening, it's a problem, but I don't think this fix will bring us any more > afoul of that than we already are. Right, in this case, we would have multiple form elements (content) as well, and each would manage it's controls separately, so it should not be a problem. > ...--and I am changing IsSuccessful() to ShouldSubmit() unless I hear a reason The reason it is called IsSuccessful is because the html spec on form submission refers to 'successful' form controls: http://www.w3.org/TR/html4/interact/forms.html#h-17.13.2 I'd prefer IsSuccessful, as that would eliminate any ambiguity...

Thanks Eric! > Where the value is represented by an attribute in the content > (e.g., 'checked' for a radiobutton), we store the value in the content model. > On the other hand, sometimes the value is not represented by an attribute in the > content model. (e.g., 'GetValue' for a text input returning the current text > control value, but the 'value' attribute representing the solely the 'default' > value in the content model) In these cases, the current value is stored in the > frame itself, and the 'presentation state' when the frame is not around. > > The right approach is to walk the DOM on the submit, and grab values from the > frame if it is around, presentation state otherwise; and in the cases where the > value happens to be represented by a content attribute (or the frame and > presstate are both not there), then just grab it directly from the content. Are you suggesting we should check whether the frame exists in OnSubmit()? In my opinion, OnSubmit() should *always* use whatever value <element>.value would return in the DOM. If that value is out of sync with the control, that's a priority to fix anyway. I think GetValue() shields us from worrying about frames in OnSubmit(). For example, calling GetValue() on nsHTMLInputElement (your example) will go to the frame if it exists. Then we get to the second stage of the fix: have the default value populate initially in the DOM for display=none controls, and have frames check the DOM for their initial values and *never* the value attribute (which they must be doing currently). These fixes will fix everything we need, I think. The third stage is to make the controls use the content for their storage. It seems like The Right Thing To Do, and dbaron indicated that doing this would fix a lot of other stuff. > I have a portion of this patch in various stages in my tree. (Most recently, it > was abandoned to finish some critical work for N 6.1, so it may be out of date) How solid is it? What I have done so far is move Submit and Reset (and all the functions they need to exist) from nsFormFrame to nsHTMLFormElement. I have not yet compiled because I need to modify the form controls to support the four helper functions. > Exactly. My first pass for the patch in my tree removed *all* of the code from > mozilla/layout/html/forms/src/nsFormFrame.cpp - the task is mainly just to move > it all into content, and replace nsFormFrame with a simple block frame in > nsCSSFrameConstructor... I love this idea. But I am not sure how to go about it. Do you mean remove nsFormFrame.cpp altogether, and just create some sort of block frame in nsCSSFrameConstructor? (Like possibly even do NS_NewBlockFrame(aPresShell, &newFrame))? > If a form element (nsHTMLFormElement) is around, but is display:none, there will > be no corresponding frame (nsFormFrame), and since all the submit logic is > currently in the frame, it will not submit at all. The logic could be moved to > the content to solve this problem, but would ideally go in an independent > service, like a 'form submitter service', as it is currently duplicated both > here and in the plugins code. Your suggestion to leave it in nsFormFrame is > fine as an interim solution to this problem. Turns out I've been moving it into nsHTMLFormElement, but I've been thinking about moving it into a separate .cpp anyway, since there are so many functions that it clutters the code. Creating a service, however, is beyond my current knowledge, and I haven't found docs on it in the website. I'll look into it before long. This is duplicated in plugins code? So the bug is going to happen there too? Seems like we pretty much need to create a service to fix this everywhere. When does submit happen in plugins code? > > ...--and I am changing IsSuccessful() to ShouldSubmit() unless I hear a reason > > I'd prefer IsSuccessful, as that would eliminate any ambiguity... OK, that was a good reason. I think I'll just add a comment as to why above the function definition for poor unfortunate souls who don't know the odd terminology in the spec :)

> I think GetValue() shields us from worrying about frames in OnSubmit(). Now that I think about it, you're absolutely right in that the OnSubmit method should not touch the frames directly. :) Bad wording on my part, sorry! What we need to ensure is that the logic to correctly generate the form submit data for each form control is retained (the equivalent of the GetNamesValues() methods in the corresponding frames). What I meant to point out was the GetValue() will not return what you want to submit to the server in all cases (which I'm sure you've already run into). To enumerate: <button>, <textarea>, and <input type=text,password,button,submit,file> return results of GetValue() <input type=checkbox,radio> if NOT checked, don't append to submission data if checked if value attribute set, return value attribute if value attribute NOT set, return 'on' <select> iterate through options if this option is selected if value attribute set, return value attribute if value attribute NOT set, return text, whitespace compressed > Then we get to the second stage of the fix: have the default value populate > initially in the DOM for display=none controls, and have frames check the DOM > for their initial values and *never* the value attribute (which they must be > doing currently). These fixes will fix everything we need, I think. I see, you mean, call SetValue/Checked/Selected() to set the presentation state right away so that GetValue/Checked/Selected() returns the right thing, then have the frames call GetValue/Checked/Selected() to determine their initial state. Sounds right! > The third stage is to make the controls use the content for their storage. It > seems like The Right Thing To Do, and dbaron indicated that doing this would > fix a lot of other stuff. From what I understand, this would not be the correct way to implement the form controls. The DOM API's were ment to be just that, and adding attributes that are not in the spec to the content model to represent state would violate the spec... I think that you can achieve what you are after by making use of 'presentation state' to store the values for the controls while the frames are not around. ??? > > I have a portion of this patch in various stages in my tree. > How solid is it? You'd probably be best off sticking with what you have done so far. My patch mainly centered around creating a 'form submit service' and ripped everything apart to the point where nothing worked, then I ran out of time to put it together again. It's probably best to take the 'baby steps' approach here! > > ...removed *all* of the code from ...nsFormFrame.cpp... > I love this idea. But I am not sure how to go about it. Do you mean remove > nsFormFrame.cpp altogether, and just create some sort of block frame in > nsCSSFrameConstructor? (Like possibly even do NS_NewBlockFrame(aPresShell, > &newFrame))? Yep, exactly! Just remove the code block from nsCSSFrameConstructor::ConstructFrameByTag that starts with: else if (nsHTMLAtoms::form == aTag) { ... } and we will fall back to ConstructFrameByDisplayType, and create a block frame automatically! :) > Turns out I've been moving it into nsHTMLFormElement, but I've been thinking > about moving it into a separate .cpp anyway, since there are so many functions > that it clutters the code. Creating a service, however, is beyond my current > knowledge, and I haven't found docs on it in the website. I'll look into it > before long. ... > This is duplicated in plugins code? So the bug is going to happen there too? > Seems like we pretty much need to create a service to fix this everywhere. > When does submit happen in plugins code? Leaving it in nsHTMLFormElement is probably fine for now (baby steps). Duplication of this code occurs to varying degrees in: nsIsIndexFrame::OnSubmit() and nsPluginHostImpl::NewPluginURLStream. They don't take exactly the same data, or process it exactly the same way, and they are both much less frequently used code paths than general form submission, so I wouldn't worry about it right away. Thanks again for tackling this work, your efforts are definitely appreciated!

Fully agreed on everything except: > > The third stage is to make the controls use the content for their storage. It > > seems like The Right Thing To Do, and dbaron indicated that doing this would > > fix a lot of other stuff. > From what I understand, this would not be the correct way to implement the form > controls. The DOM API's were ment to be just that, and adding attributes that > are not in the spec to the content model to represent state would violate the > spec... I think that you can achieve what you are after by making use of > 'presentation state' to store the values for the controls while the frames are > not around. ??? OK. That looks like that's what the DOM is doing, and that's fine. But ... it seems like there should be only *one* place the value is stored. If it's in presentation state, DOM should never check the frames to get the value. And frames should get/set values to/from the presentation state. Redundancy should be avoided whenever possible, as it creates bugs. What is the purpose of the presentation state, BTW? Is there one presentation state per control? I see that InputElement is using it to store values when the frames aren't around. > Duplication of this code occurs to varying degrees in: > nsIsIndexFrame::OnSubmit() and nsPluginHostImpl::NewPluginURLStream. I'll put these on the rapidly growing todo list :)

Oh yes, for those who are making wizards with Mozilla, a decent workaround is to use visibility:collapse instead of display:none and visibility:inherit instead of display:inherit or display:block. It produces strange artifacts, but at least it generally works and you can get on with life :)

Unfortunately, if I recall correctly, visibility:collapse doesn't work in IE so you have to branch your code. It also doesn't reflow the page in the same way as display:none so you have to be very careful about how you lay out your page. My current workaround is to use <span> tags and absolute positioning. Makes Mozilla slower than all get out (you can type maybe one letter a second) but it works in almost all browsers (Konqueror included, with opera being the notible exception) I would also like to express my gratitude to John for taking this project on. Thank you for your time... David

visibility: collapse won't work on anything but tables. It means the same thing as visibility: hidden if used on something other than a table row or column. It will generate a fully transparent layout box, which is not what you want. See section 11.2 in CSS2.

> ...it seems like there should be only *one* place the value is stored. If > it's in presentation state, DOM should never check the frames to get the > value. And frames should get/set values to/from the presentation state. > Redundancy should be avoided whenever possible, as it creates bugs. Sounds like a good idea to me: though this would take a bit of work, and might make things a teensy bit less performant, it would clean things up. (And perhaps, further in the future, Session History could even then just get presentation state instead of having to call SaveState and RestoreState on the frames themselves... :) > What is the purpose of the presentation state, BTW? Is there one presentation > state per control? I see that InputElement is using it to store values when > the frames aren't around. Currently, it's purpose is exactly that (to store values when the frames aren't around), so that when a form control is set to display:none then back, it will not use it's value. Yes, there should be one per control.

Agreed, and I didn't even know about the benefit to Session History :) I'll have time to work on this on Sunday (and hopefully tomorrow). I don't expect to get it working this weekend though, just coded / compiled and the tests started. As much as I believe in the Plan, I anticipate unanticipated problems :)

OK. Guru Reality Check while I code. I need the proper frame classes for each type of element. Below are my current *assumptions* about this. First, though, some questions. QUESTIONS: - What's the difference between nsGfxButtonControlFrame and nsHTMLButtonControlFrame? - Where are the actual frames created? (I can't find an external reference to the frames or even elements in LXR.) - Why are fieldset, label and legend form controls? (They can't have values as far as I can tell.) - What're nsComboboxControlFrame and nsListControlFrame for? Do I at least have nsGfxListControlFrame pegged? <input type=hidden> - frame: ? <input type=text> - frame: nsGfxTextControlFrame2 <input type=password> - frame: nsGfxTextControlFrame2 <input type=button> - frame: nsHTMLButtonControlFrame (?) <input type=reset> - frame: nsHTMLButtonControlFrame (?) <input type=submit> - frame: nsGfxButtonControlFrame (?) <input type=checkbox> - frame: nsGfxCheckboxControlFrame <input type=radio> - frame: nsGfxRadioControlFrame <input type=file> - frame: nsFileControlFrame <input type=image> - frame: nsImageControlFrame <button> - frame: nsHTMLButtonControlFrame <select> - frame: nsGfxListControlFrame (?) <textarea> - frame: nsGfxFrameControl2 <fieldset> - frame: nsFieldSetFrame <label> - frame: nsLabelFrame <legend> - frame: nsLegendFrame I am also *assuming* that each of the aforementioned elements has the corresponding nsHTML<TagName>Element class as its content class. There appear to be classes for all of them, at least.

Reassigning.

I've found out where the frames are constructed (the answer was staring me in the face). It turns out that input type=hidden has a frame! (It uses the button frame.) I am thinking we can get rid of this "feature" when we make this fix. Oh, also, nsGfxListControlFrame isn't used anywhere that I can tell. I think I will try and remove it and see what happens. *toothy grin* Now to figure out whether select dropdowns store their value in nsListControlFrame or nsComboboxControlFrame.

There is a *lot* of crap in the text control about initial value. Ugh. This should *so* not be the domain of the text control. It has to tell, for example, whether it's a textarea so it can grab the value from the inside of the textarea. The code will be a great deal simpler once these changes are made. Anyway, for now I'm not removing it from there ... I'm just going to add these lines to the new Reset() in InputElement: if(type == NS_FORM_INPUT_TEXT || type == NS_FORM_INPUT_PASSWORD) { nsAWritableString initVal; GetDefaultValue(initVal); SetValue(initVal); }

I have a patch now, but it needs a small amount of work. Mozilla is running and submitting forms using DOM only (no physical controls), with a few quirks (I rewrote a few things like Reset() because the current implementations didn't make sense anymore in the frame context). Once I work out the quirks I will send it out. Hopefully I will have fixed the initial value problem by then too (I am hoping I can just call Reset() on the DOM element) but don't get your hopes up there yet.

I am adding a method to nsIContent: DoneCreating(). This change could potentially affect vendors, but I think it is worthwhile. The interface basically gets called whenever the parser finishes adding attributes and content to an interface. Right now only forms are using it, but potentially any element that wanted to initialize could use it. Netscape, bug owners, lend me your ears. This is an API change.

Hmm.. that sounds a bit dangerous IMHO. What happens if a script changes the DOM after DoneCreating() is called? The same problem will arise for any client of that function which could lead to even more "dynamic-html" bugs then we already have... What did you have in mind doing in DoneCreating for form elements?

If a script changes the DOM after DoneCreating() is called, it sets the values normally. For most elements, all I do in DoneCreating() is Reset() the control. I just want an initial value when we're done. For Select, there is some special logic, but that logic preexisted the DoneCreating() function (it was called DoneAddingContent(). An alternative way to do this would be to have the value not initialize until GetValue() is called--I think this would work, but it would entail making the form controls call the DOM for the value instead of the presentation state, which for some reason (I'm still not entirely sure why) Pollmann didn't want. I am all for this solution if we can't find a reason not to do it. It seems like the cleanest possible way to do it.

Arr. The select code SUCKS and requires a decent amount of rework before this will work. I may revamp the whole thing. I would post the patch right now, but this is the major flaw in it; <select> just doesn't submit at all (or reset for that matter) because the data isn't sitting where it's supposed to. I talked with Jonas and I agree, it's best if we don't add that interface to nsIContent because it could create bugs by people relying on it too much. People should be able to handle attributes dynamically as they come. So should form controls. To that end we will not reset the value until the first GetValue()--at which point, if it's not initialized, it will *be* initialized from GetDefaultValue(). I think this pretty much throws out the idea of doing this in stages; which is too bad because the current patch (unified diff) is ~ 4000 lines, not including two new files :) We could still do it in stages but at this point we'd be doing a decent amount of redundant work.

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

reassigning to evaughan.

marking nsenterprise-.

Quick update for those interested. The first patch is nearly working. We're going to put in <select> and form submission changes first, get that reviewed and into the tree, and then move on to <input> and <button> in a second patch. This should make the reviewers happier since they won't have to deal with it all at once. At evaughan's suggestion, I'm accepting this bug.

oops, really reassigning.

What about an entry on http://komodo.mozilla.org/planning/branches.cgi ? This undertaking seems big enough to me.

We have *just* missed this milestone for the first patch. I have a complete patch in jst's hands for review. It's 360K, so I'll refrain from posting it here and wasting people's time until it's gone through that gauntlet at least once. Setting milestone to 0.9.7. I am almost certain the select and form fix will be in at 0.9.6, but I can't guarantee we'll have file, button, text and radio all fixed by that point. I don't even really know the scope of the work involved, except that I think each of them individually will be easier and smaller than fixing <select>. This was the hardest part.

I have added a partially-reviewed-by-jst patch for those who'd like to try it. Patch is up to date with CVS as of about an hour ago. It is fully functional (no known regressions), and fixes many things, but it is *not* necessarily done with respect to programming style and jst may catch other things as well. It is a work in progress. Oh, BTW, I posted the patch (and this) using my patched build on Linux. I have been surfing Bugzilla and she seems to be fine. If you decide you are adventurous and want to test, there are some select regression tests at http://www.johnkeiser.com/mozilla/select and a nice big gigantic form submit test is at http://www.johnkeiser.com/cgi-bin/mozform.pl. Additions to regression tests are welcome--even encouraged.

We have a branch now, and some binary optimized builds with the patch will be available as soon as I can get leaf to put them up on mozilla.org. To get the branch, compile and test, you can do: cvs -d :pserver:anonymous@cvs-mirror.mozilla.org:/cvsroot co -r FORMREWRITE_20011008_BRANCH mozilla/client.mk cd mozilla gmake -f client.mk

Test builds! Get your test builds here! Optimized and hot off the presses for your bug-hunting enjoyment! Comes in both Linux and Win32 flavors! http://ftp.mozilla.org/pub/mozilla/nightly/experimental/newforms/ See the URLs I mentioned in my 2001-10-04 00:35 comment for tests, as well as many of the bugs this bug blocks. And of course, use our very own Bugzilla, one of the main baddies when it comes to select and forms.

Looks like the Linux build didn't transfer right. Check back tomorrow morning / afternoon. The real build is 7.7M. Win32 build looks like it's the right size though.

I wanted to point out, this is just the *first patch*. It makes form submit and reset work when the form or the elements are display: none, and it makes select initialize properly when it is display: none. The effects on the testcase in this bug is, if you click Unhide, then Hide, then Submit, it will submit. The problem, as noted before, is that many controls do not initialize until they show up visually. This is fixed for <select> in this patch, but out of respect for the reviewers I did not fix everything else. If one does a wizard interface where the user has to visit every tab, this patch will fix the problem completely. If the user can skip tabs, however, controls will not be initialized properly. If we can get this patch tested and into CVS, subsequent patches will be small and pretty easy to get in. This is the "big one" where we really need testing.

Y'all will probably be happy to know that I have input type=text/file/password (and consequently the testcase in this bug) fully working in my tree. When I have textarea working, we'll put it up to the branch. What this means is, when this first patch is reviewed and checked in, input type=text and textarea will go in along with it :) This leaves button, checkbox and radio. Radio will *definitely* not happen until after this patch is in the tree.

The test above Speed Test (which actually comes from a different bug) it dynamically adds a lot of items to the selects. This test is noticably slower and we spent a lot of time speeding it up. Also, when the items in the second select are replaced the last item ends up being selected instead of the first. Test it by selecting "Parisienne" in the first list.

In the test above select_js.html there are two separate issue: 1) It does not scroll to display the newly selected item (in all cases) test this by pressing "Add 100Items", then "Select Item #2" 2) The "Edge Case" doesn't work correctly. After pressing "Edge Case" in the third select (the multiple selection) Items #5 and #9 are both suppose to be selected

In the test above select_js3.html there is one issue: The first test btn replaces the first three items in the combo. When I press the Test #1 button multiple times with a trunk build I always see the same result. With this patch I the second time I press the button, the items in the list reverse order.

As to the last item in the list being selected, I guess we need to have a discussion there ... from discussions I have had with people, when we add a selected item into a single select (as you are doing), that item should be immediately selected. If you added a selected option into a select, isn't that what you'd expect? Doing it on a timer (as we are doing now) works unpredictably and keeps the options and selectedIndex from being accessible to JavaScript immediately (the subject of many of the blocked bugs above). I'll look into the speed issue. Note that the speed optimizations made before are still there (and more): the bug 97345 testcase actually goes slightly *faster* than before (~25s versus 30s to add 10,000 options dynamically). So I haven't totally ignored performance :) (JST had to talk me out of a few optimizations I wanted to make, in fact ...) This problem has to do with setting the item default selected before adding to the select, I believe. (That's the only difference between testcases). Note that this operation *should* take slightly longer than before because we were hiding that operation in a timer before. That is the subject of at least 5 or 6 of the bugs above (selected index / options /length not immediately available for JavaScript access), so it seems important enough. It should not take /this/ much longer on such a small select though, so I'll check it out. I will look into these (and the other issues) in a couple of days. Busy time right now. Thanks greatly!

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

OK, all issues fixed. Attaching patch. Specifically: 1. Speed Issue: this was due to FlushPendingNotifications being called more often. It's fixed by only flushing content, and not flushing notifications at all when the option is selected. Everything seems to work, rods is looking into it. 2. Does not scroll to display selected item. This was only happening on init, now it happens *whenever* you set selectedIndex with JavaScript. Fixed. 3. Edge Case. For some reason, setting selectedIndex was clearing all options--a mini-regression within the patch. Fixed. 4. Items Added In Reverse Order. This was rather nasty. It made replace work badly and stemmed from GetOptionIndex initializing the option index to 0. Now we don't initialize it at all. (I checked, none of the callers use the value if it fails.) Fixed.

Please note that bug 97345 testcase is now a *lot* faster on the trunk than on the branch (before the branch was slightly faster than the trunk). jesup ran a jprof on the testcase tonight and he tells me that this is a result of an nsLineBox change by dbaron in bug 86947, so we'll get that extra performance boost for free :)

Patch "Patch to Branch - fixes rod's issues" checked in on the branch.

General comments: I don't love the removal of all checking of the return value of GetPrimaryFrame(). It happens to be ok (since the pointer is generally checked), but it's confusing and I think bad practice to have the result be ignored (since it's now always NS_OK) and only test the value of an Out parameter. Someone else might someday make that return value real again and not notice (or not want to) fix all callers. Still, I wouldn't hold this up for that. Comments on Void/SupportsArray code, and also <option> adding: In WillAddOptions(), ChildAt() is used to determine if we're past the end of the array or not. Generally, I don't like that, even if it's supported for nsISupportsArrays (it soon will not be for nsVoidArrays, and ChildAt is based on that - though ChildAt will grow (for now) some bounds-checking). I prefer either a test against the Count (ChildCount in this case), or (if we always know if it's an Append or not) a separate append method. Please change to compare against ChildCount() (it's fast). The rewrite of the <option> adding/etc code that I redid in 97345 seems good. This rewrite is much more general (and much larger1); my change was a very specific tweak for performance in the normal case. The whole option code is better engineered now. It also looks like <optgroup> is more supported now, which is good. Style: In layout/html/forms/src/nsGfxTextControlFrame2.cpp, there are two bracing styles used: if (foo) { } and if (foo) { } By far, most of the code in that file uses the first; code that doesn't was probably inserted by people who didn't look to see what style was in use. One should NOT partially change the style of a file; one should follow the style in the file unless you change it SO much it makes sense to assert style over the entire file. So please remove the spurious conversions to type 2, and make the rest of the code you added to that file follow type 1. (This comment might apply to other files, but it certainly does here.) When in Rome... I LOVE the amount of code removal/factoring done in the redesign. Paranoia: +nsListControlFrame::ScrollToIndex(PRInt32 aIndex) +{ + if (aIndex == -1) { Change that to aIndex < 0 please Other than that, looks good, especially from an array/<option> POV. r=rjesup@wgate.com for the array and <option>/<optgroup> changes so long as the comments above are addressed (other than the GetPrimaryFrame issue).

Randell, if someone cares about the return value from GetPrimaryFrame() they could check it for the purpose of passing it along to the caller if there really was an *error* somewhere (but then again GetPrimaryFrame() will never return an error for any *valid* reasons as it's written today, nor with this change), but checking for the error code to determine if there was a frame or not is just plain wrong. The absense of a frame is a very normal condition, it should *not* be flagged with an error code. I know we violate this all over the place, but it's still wrong. If all you care about is if we could find a frame for a element or not, then check the pointer and ignore the return value (unless you want to pass it along to the caller, which wasn't really done anywhere for any good reasons). I *hate* methods that return error codes to indicate something other than an error (think of error codes as exceptions, would you throw an exception if there's no frame for an element, NO). I'm even thinking about making GetPrimaryFrame() return void since the return value is meaningless anyway, and it's an internal helper so [XP]COM clean-ness doesn't matter. This way people wouldn't be fooled into believeing that GetPrimaryFrame() is broken and returns an error when there's no frame. Thanks for going through the changes!

Removed the extraneous bracing change. Now we are checking ChildCount() instead of the result of ChildAt() too. Thanks!

This doesn't build for me on Linux. Clobbered but still get: nsFormSubmitter.cpp: In function `nsresult nsFormSubmitter::OnSubmit (nsIForm *, nsIPresContext *, nsIContent *)': nsFormSubmitter.cpp:212: `ctrlsModAtSubmit' undeclared (first use this function) nsFormSubmitter.cpp:212: (Each undeclared identifier is reported only once for each function it appears in.) nsFormSubmitter.cpp:212: `textDirAtSubmit' undeclared (first use this function) gmake[5]: *** [nsFormSubmitter.o] Error 1 gmake[5]: Leaving directory `/home/dark/DISK/mozilla/content/html/content/src' gmake[4]: *** [install] Error 2 etc. I'm building with --disable-bidi

I'll look into it tomorrow evening, but try building from CVS. It may actually be a bug though. The patch is committed and tomorrow's daily should reflect it. I will update bugs tomorrow.

I'm seeing the same bustage as R.K.Aa building the current trunk, I've got --disable-bidi as well.

Filed the --disable-bidi build bustage as bug 108174 - for anyone who's interested ;-)

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

OK, this particular bug is fixed (controls with display: none submit). Moving initial value issues for radio and checkbox into new bugs (bug 108307 and bug 108308). There is also an ugly issue with JavaScript modifying controls *in between* showing and hiding that needs to be addressed (bug 108309). Please report any other issues you have with form controls and display: none as separate bugs--this bug is getting rather large and spammish.

Now that you've fixed this bug, how about doing the same thing for IFRAMEs with "display: none"? :-) Don't know if we have a bug for it but I think that currently IFRAMEs with "display: none" don't load their documents...

The problem with IFrames is probably either bug 55987 or bug 52334. If not, please do a search and post another bug if you can't find it. That being said, I would like to personally give a standing ovation of thanks to John Keiser for all the work he has done to resolve this bug. Thank you. Perhaps a friend of the tree award is in order? David

IFrame bug is bug 52334.

verifying on build 2002-01-04-03-trunk windows 98 and 2002-01-04-08-trunk Linux RedHat

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

I'm sure you know this but any child controls are also excluded from the form control collection

Mark, I'm not quite sure what comment 117 means (or how it relates to this bug). Please file a bug on it, with a testcase attached to the bug, and cc me on it.