form controls with style="display: none;" unsuccessful in Mozilla

VERIFIED FIXED in mozilla0.9.7

Status

()

Core
HTML: Form Submission
P3
critical
VERIFIED FIXED
17 years ago
3 years ago

People

(Reporter: myk, Assigned: John Keiser (jkeiser))

Tracking

(Depends on: 1 bug, Blocks: 1 bug, 4 keywords)

Trunk
mozilla0.9.7
dataloss, html4, relnote, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P1]relnote-devel form controls w/ "display: none" should still submit name/value pairs, URL)

Attachments

(9 attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Comment 1

17 years ago
Created attachment 7184 [details]
test case and quote from HTML spec

Comment 2

17 years ago
reassigning
Assignee: rods → pollmann

Comment 3

17 years ago
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.
Keywords: testcase
OS: Linux → All
Whiteboard: [TESTCASE] form controls w/ "display: none" should still submit name/value pairs

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M17

Comment 4

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

Comment 5

17 years ago
This bug could cause dataloss in form submission.  Nominating for beta3.
Keywords: correctness, nsbeta3
Marking nsbeta3-. Added html4, relnote keywords
Keywords: html4, relnote
Whiteboard: [TESTCASE] form controls w/ "display: none" should still submit name/value pairs → [nsbeta3-][TESTCASE] form controls w/ "display: none" should still submit name/value pairs

Comment 7

17 years ago
Moving bug marked nsbeta3- to Future milestone.
Target Milestone: M17 → Future

Comment 8

17 years ago
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.
Keywords: 4xp
Hardware: PC → All

Updated

17 years ago
Blocks: 7954

Comment 9

17 years ago
Updating QA contact.
QA Contact: ckritzer → vladimire

Comment 10

17 years ago
Adding rtm keyword.
Keywords: rtm
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.
Whiteboard: [nsbeta3-][TESTCASE] form controls w/ "display: none" should still submit name/value pairs → [rtm-][nsbeta3-][TESTCASE] form controls w/ "display: none" should still submit name/value pairs

Comment 12

17 years ago
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.

Comment 13

17 years ago
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.
Whiteboard: [rtm-][nsbeta3-][TESTCASE] form controls w/ "display: none" should still submit name/value pairs → [rtm][nsbeta3-][TESTCASE] form controls w/ "display: none" should still submit name/value pairs

Comment 14

17 years ago
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

Comment 15

17 years ago
Removing '4xp' keyword since the keyword description doesn't mention
competitor products anymore.
Keywords: 4xp

Updated

17 years ago
Whiteboard: [rtm][nsbeta3-][TESTCASE] form controls w/ "display: none" should still submit name/value pairs → [rtm][nsbeta3-][TESTCASE] relnote-devel form controls w/ "display: none" should still submit name/value pairs
Adding dataloss keyword, setting severity to major since this is an obstacle for
developers. Nominating for mozila 0.9.
Severity: normal → major
Keywords: dataloss, mozilla0.9

Comment 17

17 years ago
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.

Comment 18

17 years ago
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?
Whiteboard: [rtm][nsbeta3-][TESTCASE] relnote-devel form controls w/ "display: none" should still submit name/value pairs → [rtm need info][nsbeta3-][TESTCASE] relnote-devel form controls w/ "display: none" should still submit name/value pairs
Marking rtm-. Not enough time to fix for rtm.
Whiteboard: [rtm need info][nsbeta3-][TESTCASE] relnote-devel form controls w/ "display: none" should still submit name/value pairs → [rtm-][nsbeta3-][TESTCASE] relnote-devel form controls w/ "display: none" should still submit name/value pairs

Comment 20

17 years ago
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.  

Comment 21

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

Comment 22

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

Comment 23

17 years ago
The same problem also with other element's !!

look at bug 52334
'Problems with contentDocument and 'display:none' in IFRAMES.'

Comment 24

17 years ago
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. 

Comment 25

17 years ago
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.
Severity: major → critical
Target Milestone: Future → mozilla0.9
This problem is also covered by bug 57269, it seems.

Updated

17 years ago
Keywords: nsbeta1

Comment 27

17 years ago
*** 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
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 29

17 years ago
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.

Comment 30

17 years ago
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

Comment 31

17 years ago
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.

Comment 32

17 years ago
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.

Comment 33

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

Comment 34

16 years ago
*** 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.
Keywords: nsbeta3, rtm
Whiteboard: [rtm-][nsbeta3-][TESTCASE] relnote-devel form controls w/ "display: none" should still submit name/value pairs → relnote-devel form controls w/ "display: none" should still submit name/value pairs
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.

Comment 38

16 years ago
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!  :)

Comment 39

16 years ago
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

Comment 40

16 years ago
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.

Comment 42

16 years ago
Oi, have to push this one more milestone - running out of time.  I will continue
to work on this and attach a patch asap...
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Whiteboard: relnote-devel form controls w/ "display: none" should still submit name/value pairs → [Hixie-P3] relnote-devel form controls w/ "display: none" should still submit name/value pairs
*** Bug 74362 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 44

16 years ago
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.

Updated

16 years ago
Keywords: mozilla0.9.3

Updated

16 years ago
Keywords: nsenterprise

Comment 45

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

Comment 46

16 years ago
Missed 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 47

16 years ago
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.
Whiteboard: [Hixie-P3] relnote-devel form controls w/ "display: none" should still submit name/value pairs → [Hixie-P1]relnote-devel form controls w/ "display: none" should still submit name/value pairs
(Assignee)

Comment 48

16 years ago
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.
(Assignee)

Comment 49

16 years ago
Created attachment 45095 [details]
Show the DOM value of the non-displayed element
(Assignee)

Comment 50

16 years ago
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?
(Assignee)

Comment 51

16 years ago
Created attachment 45123 [details]
Better Set of Tests
(Assignee)

Comment 52

16 years ago
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.

Comment 53

16 years ago
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.
(Assignee)

Comment 54

16 years ago
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?
(Assignee)

Comment 55

16 years ago
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 :)
(Assignee)

Comment 56

16 years ago
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.)
(Assignee)

Comment 57

16 years ago
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.

Comment 58

16 years ago
> 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...
(Assignee)

Comment 59

16 years ago
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 :)

Comment 60

16 years ago
> 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!
(Assignee)

Comment 61

16 years ago
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 :)
(Assignee)

Comment 62

16 years ago
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 :)

Comment 63

16 years ago
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

Comment 64

16 years ago
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.

Comment 65

16 years ago
> ...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.
(Assignee)

Comment 66

16 years ago
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 :)
(Assignee)

Comment 67

16 years ago
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.

Comment 68

16 years ago
Reassigning.
Assignee: pollmann → kmcclusk
Status: ASSIGNED → NEW
Keywords: nsenterprise → nsenterprise+
(Assignee)

Comment 69

16 years ago
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.
(Assignee)

Comment 70

16 years ago
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);
}
(Assignee)

Comment 71

16 years ago
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.
(Assignee)

Comment 72

16 years ago
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?
(Assignee)

Comment 74

16 years ago
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.
(Assignee)

Comment 75

16 years ago
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.

Updated

16 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Updated

16 years ago
Blocks: 60893
*** Bug 99200 has been marked as a duplicate of this bug. ***
reassigning to evaughan.
Assignee: kmcclusk → evaughan

Comment 78

16 years ago
marking nsenterprise-.
Keywords: nsenterprise+ → nsenterprise-
(Assignee)

Comment 79

16 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 80

16 years ago
oops, really reassigning.
Assignee: evaughan → jkeiser
Status: ASSIGNED → NEW
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

16 years ago
Blocks: 100511
(Assignee)

Updated

16 years ago
Blocks: 100988
(Assignee)

Updated

16 years ago
Blocks: 97004
(Assignee)

Updated

16 years ago
Blocks: 100181
(Assignee)

Updated

16 years ago
Blocks: 64120
(Assignee)

Updated

16 years ago
Blocks: 38825
(Assignee)

Updated

16 years ago
Depends on: 52975
(Assignee)

Updated

16 years ago
Blocks: 58855
(Assignee)

Updated

16 years ago
Depends on: 60137
(Assignee)

Updated

16 years ago
Blocks: 100800
(Assignee)

Updated

16 years ago
No longer depends on: 60137
(Assignee)

Updated

16 years ago
Blocks: 60137
(Assignee)

Updated

16 years ago
No longer depends on: 52975
(Assignee)

Updated

16 years ago
Blocks: 88076
(Assignee)

Updated

16 years ago
Blocks: 65188
(Assignee)

Updated

16 years ago
Blocks: 69839
(Assignee)

Updated

16 years ago
Blocks: 94432
What about an entry on http://komodo.mozilla.org/planning/branches.cgi ? This
undertaking seems big enough to me. 
(Assignee)

Updated

16 years ago
Blocks: 100810
(Assignee)

Updated

16 years ago
Blocks: 70482
(Assignee)

Updated

16 years ago
Blocks: 9136
(Assignee)

Updated

16 years ago
Blocks: 101114
(Assignee)

Updated

16 years ago
Blocks: 40983
(Assignee)

Updated

16 years ago
Blocks: 101720
(Assignee)

Updated

16 years ago
Blocks: 101993
(Assignee)

Updated

16 years ago
No longer blocks: 100810
(Assignee)

Comment 82

16 years ago
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.
Blocks: 100810
Target Milestone: mozilla0.9.5 → mozilla0.9.7
(Assignee)

Comment 83

16 years ago
Created attachment 52020 [details] [diff] [review]
(Work In Progress) Submit/Select Patch #1
(Assignee)

Comment 84

16 years ago
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.
(Assignee)

Updated

16 years ago
Blocks: 92466
Blocks: 103645
No longer blocks: 103645

Updated

16 years ago
Blocks: 104166
(Assignee)

Updated

16 years ago
Keywords: mozilla0.9.3 → nsbeta1
(Assignee)

Comment 85

16 years ago
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
(Assignee)

Updated

16 years ago
Blocks: 104904
(Assignee)

Comment 86

16 years ago
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.
(Assignee)

Comment 87

16 years ago
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.
(Assignee)

Comment 88

16 years ago
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.
(Assignee)

Comment 89

16 years ago
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.

Updated

16 years ago
Blocks: 105405
(Assignee)

Updated

16 years ago
Blocks: 105685
Blocks: 103709
(Assignee)

Updated

16 years ago
Blocks: 104996
(Assignee)

Updated

16 years ago
Blocks: 105945
(Assignee)

Updated

16 years ago
Blocks: 106085

Comment 90

16 years ago
Created attachment 54660 [details]
Speed test

Comment 91

16 years ago
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.

Comment 92

16 years ago
Created attachment 54662 [details]
select_js.html

Comment 93

16 years ago
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

Comment 94

16 years ago
Created attachment 54664 [details]
select_js3.html

Comment 95

16 years ago
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.
(Assignee)

Comment 96

16 years ago
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. ***
(Assignee)

Updated

16 years ago
Blocks: 91658
Blocks: 106524
(Assignee)

Updated

16 years ago
Blocks: 106735
(Assignee)

Comment 98

16 years ago
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.
(Assignee)

Comment 99

16 years ago
Created attachment 55887 [details] [diff] [review]
Patch to Branch - fixes rod's issues
(Assignee)

Comment 100

16 years ago
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.
(Assignee)

Comment 102

16 years ago
Created attachment 56059 [details] [diff] [review]
Patch to trunk
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!
(Assignee)

Comment 105

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

Comment 106

16 years ago
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
(Assignee)

Comment 107

16 years ago
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.

Comment 108

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

Comment 109

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

Comment 110

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

Updated

16 years ago
Blocks: 108232
(Assignee)

Comment 111

16 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
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... 

Comment 113

16 years ago
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.

Comment 115

16 years ago
verifying on build 2002-01-04-03-trunk windows 98
and 2002-01-04-08-trunk Linux RedHat
Status: RESOLVED → VERIFIED
*** Bug 121522 has been marked as a duplicate of this bug. ***

Comment 117

13 years ago
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.

Updated

10 years ago
Depends on: 184025
You need to log in before you can comment on or make changes to this bug.