Closed Bug 100533 Opened 20 years ago Closed 16 years ago

[FIX]form.submit() inside a display: none iframe does nothing

Categories

(Core :: DOM: Core & HTML, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: Maniac, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.4+) Gecko/20010918
BuildID:    2001091803

submit() called from JavaScript for the form that resides in invisible block
(div style="display:none") does nothing: no actions, no messages in JS console.

Reproducible: Always
Steps to Reproduce:
1. Go to http://softwaremaniacs.org/formsubmit.htm
2. Look into the source (it's short and clear)
3. Try to submit visible form and invisible form

Actual Results:  When submitting visible form it does its action.
When submitting invisible form nothing happens.

Expected Results:  Invisible form should also do its action.
I believe this is a duplicate of bug 60893. While this bug deals with the form
itself and 60893 deals with form fields, the basic bug is the same: JS cannot
access form elements whose frames are not drawn.

Also see bug 34297 : 'form controls with style="display: none;" unsuccessful in
Mozilla'

I'm marking as dupe of bug 60893; please reopen if you disagree.

*** This bug has been marked as a duplicate of 60893 ***
Status: UNCONFIRMED → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
verifying
Status: RESOLVED → VERIFIED
To check the testcases:

1. Open attachment 2 [details] [diff] [review]/3. Wait for it to load. Click test. Check with Ctrl+Shift+I
that it did not load www.mozilla.org in the iframe.
2. Open attachment 3 [details] [diff] [review]/3. Wait for it to load. Click test. You'll see
www.mozilla.org load in the iframe.

Tested with build 2002112904
reopening per comment 6
Status: VERIFIED → UNCONFIRMED
Resolution: DUPLICATE → ---
I would like to point out that this new issue is completely different from the
old.  Forms that are display: none work perfectly fine.  This new issue,
however, is about forms in *iframes* that are display: none, which do not have
PresContexts.  I'd rather morph the bug in this case since it's a real issue.
Assignee: rods → alexsavulov
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: document.formname.submit(); doesn't submit an invisible form → form.submit() inside a display: none iframe does nothing
Target Milestone: --- → Future
Depends on: 180861
--> (no time)
Assignee: alexsavulov → form-submission
Putting this onto my radar, I have some idea of the fix for this.  Won't happen
for a while though.  Others are welcome to fix in the meantime.
Assignee: form-submission → jkeiser
*** Bug 218835 has been marked as a duplicate of this bug. ***
This bug does not only affect the form.submit() but the form.click() and
possibly other form methods. You may want to update the summary to reflect this.
This bug can be worked around by reducing the iframe's size to 0 instead of
hidding it :

<iframe width="0" height="0" frameborder="0"></iframe>
OS: Windows NT → All
Hmm.. It looks like we just need the prescontext for the GetBidi() stuff. Is
that right?  Is that information really per-prescontext?
Seing that we don't really have a clear idea of what having more then one
prescontext really means i would say "no".
Um... Having more than one prescontext means we're printing, usually.
Sorry, i should have been more specific. I ment that we don't have a clear idea
of what more then one prescontext apart from printing and printpreview is. For
printing and print-preview i think we can safly assume that they will have the
same bidi-ness as the first (i.e. "main") prescontext so to me it looks like we
could move that info out of the context.

If we move it into the document it might be possible to submit forms in document
loaded through XMLHttpRequest which IMHO would be a good thing.
sounds like a plan to me...
I don't see any problem with moving the Set and GetBidi() into the document.
Simon, is it OK if multiple document viewers all touch the same bidi options?  I
would say "yes", but just checking....
I'm sorry, I don't understand the question, specifically the part about
"touching" bidi options.
nsIMarkupDocumentViewer exposes various methods to set the bidi options.  At the
moment, those methods call SetBidi on the prescontext.  Would it be OK if they
were to call them on the document instead?  I frankly can't think of a case when
we would have multiple "real" viewers for the same document...
I can't imagine where we'd have that either. I'd say it's ok to call bidi
methods on the document.

Mmm, nsIMarkupDocumentViewer, nsIDocumentViewer, nsIContentViewer,
nsIContentViewerFile, ... geez, think we have enough of those? Would it be good
to combine some of those (all) into one, or maybe two?
Group: security
Yeah, combining some of those would be nice.  Is there a reason you just marked
this security-sensitive, though?
Duh, no.
Group: security
I've the pb

http://www.magic.fr/webmail2-redirect.php

The link works with IE6 and Opera7.54
Depends on: 293171
Attached patch FixSplinter Review
I got a little carried away... I meant to do this as two patches, but it sorta all came together.

With this patch both this bug and the two bugs blocking it are gone.
Attachment #202199 - Flags: superreview?(jst)
Attachment #202199 - Flags: review?(jst)
Assignee: john → bzbarsky
Priority: -- → P2
Summary: form.submit() inside a display: none iframe does nothing → [FIX]form.submit() inside a display: none iframe does nothing
Target Milestone: Future → mozilla1.9alpha
Comment on attachment 202199 [details] [diff] [review]
Fix

 nsHTMLFormElement::Reset()
 {
   // Send the reset event
   nsresult rv = NS_OK;
   nsCOMPtr<nsPresContext> presContext = GetPresContext();
+  // XXXbz shouldn't need prescontext here!  Fix events!
   if (presContext) {

I was under the impression that we don't *need* a pres context to fire events any more... Though I didn't go double check that...

r+sr=jst
Attachment #202199 - Flags: superreview?(jst)
Attachment #202199 - Flags: superreview+
Attachment #202199 - Flags: review?(jst)
Attachment #202199 - Flags: review+
You can do it, but the event target will often end up wrong...  e.g. see bug 295340.
Fixed.
Status: NEW → RESOLVED
Closed: 20 years ago16 years ago
Resolution: --- → FIXED
Comment on attachment 202199 [details] [diff] [review]
Fix

>@@ -567,45 +573,51 @@ nsPresContext::GetUserPreferences()
...
>+  // Set on the document, not on ourselves so we don't do the various
>+  // extra SetBidi() work.
>+  GetDocument()->SetBidiOptions(bidiOptions);

I'm not sure about this part: why don't we need to reset visual mode and bidi enabled when the prefs change? 

Could mIsVisual move to the document also?
> I'm not sure about this part: why don't we need to reset visual mode and bidi
> enabled when the prefs change? 

Because we didn't use to and I was trying to minimize changes to code outside of what I was actually touching?  Other than that, no reason.

> Could mIsVisual move to the document also?

Possibly...  I don't really know what that member means.
(In reply to comment #32)
> > I'm not sure about this part: why don't we need to reset visual mode and bidi
> > enabled when the prefs change? 
> 
> Because we didn't use to and I was trying to minimize changes to code outside
> of what I was actually touching?  Other than that, no reason.

Sorry, with hindsight I realize that I was unconsciously comparing the new code to what I think the old code should have been, rather than what it actually was. I'll be making this change in bug 80352.

> 
> > Could mIsVisual move to the document also?
> 
> Possibly...  I don't really know what that member means.
> 

It means "are we rendering using 'visual ordering', i.e. displaying all text either right-to-left or left-to-right and not doing any Bidi reordering." It's really a shorthand for particular combinations of mBidi and mCharset, and since both of those have moved to the document, it doesn't seem to make sense to leave it on the prescontext.
Filed bug 316243 on moving mIsVisual
*** Bug 309165 has been marked as a duplicate of this bug. ***
*** Bug 327380 has been marked as a duplicate of this bug. ***
This actually fixed a regression that appeared between FF1.0 and 1.5. At Novell we have a Web application that creates a visibility:hidden IFRAME, loads it by setting .src, document.write's a form into it, and then calls submit(). In FF1.5 we are not getting around to creating a prescontext before the submit() so it fails. Apparently in 1.0 we created the prescontext earlier and the submit works. This patch makes things work again.

Unfortunately this patch changes interfaces and so it is not immediately clear we should land it on branches.
Changing interfaces is fine for FF2
The exact situation for the regression is this: (this is on branch)

-- application creates a hidden IFRAME element with a certain URI in 'src', and appends it to its DOM
  * we kick off a document load of that URI
  * nsDocLoader fires OnStateChanges all the way up the document tree
  * at or about the root, nsAccessibilityService is listening
  * nsAccessibilityService tries to get the DOM document off the nsIWebProgress for the load
  * this ends up QI'ing nsDocShell for its DOM document, which calls EnsureContentViewer
  * Since we haven't received anything for the new document yet, docshell panics and creates an about:blank content viewer
  * At this stage the IFRAME's nsSubDocumentFrame has not been created yet (we're still in BindToTree), so the docshell hasn't had InitWindow called on it. This means we have no widget to pass to the contentviewer's Init.
  * The contentviewer doesn't create a presentation, the prescontext remains null

-- application doesn't wait for onload. It just starts doing document.write() immediately.
  * This appears to cancel the load, which I assume is the desired behaviour in this pathological case
  * application writes into the about:blank content viewer/document.

-- application calls submit()
  * The contentviewer still has no prescontext, so without this patch, submission fails.

There are multiple issues here. The simplest one is that nsAccessibilityService shouldn't be asking for that document; it's getting the wrong one anyway, and causing us to do extra work. I believe this is still a problem on trunk. A bigger issue is that a QI should not be creating content viewers with who knows what side effects. I will file a bug on this.

If I neuter nsAccessibilityService, I encounter a new problem with this application. It's actually sticking the hidden IFRAME into a FRAMESET document. We don't create a frame for this, so it's just as if it was display:none. It seems that in FF1.0 this worked somehow, I'm not sure what changed there. This  problem obviously requires this patch, or a branch-safe version thereof; I may end up backporting this patch with magic _1_8_BRANCH interfaces.

(In reply to comment #38)
> Changing interfaces is fine for FF2

It is?

Even if that's true, I might be pushed to fix this for 1.8.0.x.
(In reply to comment #39)
> > Changing interfaces is fine for FF2
> 
> It is?

Sure, I don't think we're promising any more compat between 1.5 and 2 than between 2 and 3.

> Even if that's true, I might be pushed to fix this for 1.8.0.x.

Yes, it's not critical enough for that I believe
> Sure, I don't think we're promising any more compat between 1.5 and 2 than
> between 2 and 3.

We're promising Gecko API compat.  Gecko APIs are not subject to change on the 1.8 branch unless something really really weird happens.

roc, can we figure out when this regressed on trunk?  Fixing the original cause (in accessibility?) may be less invasive than this patch (though this patch is safe enough for 1.8.1, maybe).

For the rest, I'll try to look at this (in particular at whether what accessibility is doing merits a bug) in more detail when I get back.

Fixing accessibility is probably not hard. But I doubt that will be enough. I don't know how this could have worked in FF1.0 ... did we create frames for IFRAME children of FRAMESETs, maybe? I would hate to start doing that again!
If there's a publically accessible testcase (not necessarily minimal or anything) that shows the behavior difference between 1.0 and 1.5, we can probably find someone to find what change on trunk changed the behavior... Could be me when I get back, even.  ;)  That would let us know where we stand better, I think.  Is there a way of getting such a testcase up?
OK, so in Gecko 1.7 if you _dynamically_ add a kid to a frameset (so ContentInserted fires) we'll create a frame for the kid and then go on to completely forget it (leaking the frame and content node in the process).  That's probably why the testcase in question worked in Gecko 1.7.  :(  With a testcase, I could check on the correctness of this hypothesis.

I guess we should try to land this on the Gecko 1.8 branch; this ought to be safe enough except for the API changes (esp. the prescontext ones).
Boris, if you intend to checkin this on any branch then please also checkin
bug 344787, thanks.
Depends on: 344787
Flags: in-testsuite?
Attached file mochitest
RCS file: /cvsroot/mozilla/testing/mochitest/tests/test_bug100533.html,v
done
Checking in tests/test_bug100533.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug100533.html,v  <--  test_bug100533.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/mochitest/static/bug100533_iframe.html,v
done
Checking in static/bug100533_iframe.html;
/cvsroot/mozilla/testing/mochitest/static/bug100533_iframe.html,v  <--  bug100533_iframe.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/mochitest/static/bug100533_load.html,v
done
Checking in static/bug100533_load.html;
/cvsroot/mozilla/testing/mochitest/static/bug100533_load.html,v  <--  bug100533_load.html
initial revision: 1.1
done
Flags: in-testsuite? → in-testsuite+
*** Bug 362563 has been marked as a duplicate of this bug. ***
I'm seeing this bug on firefox 2.0 Was a fix for this put into firefox 2.0?
No.
(In reply to comment #50)
> No.
> 
Any ideas when it will go into firefox 2.0? Do you know what version of firefox 1 it went into? I appreciate the response :-)
At this point, this will not go into Firefox 2.anything.  It'll be fixed in Firefox 3.
(In reply to comment #52)
> At this point, this will not go into Firefox 2.anything.  It'll be fixed in
> Firefox 3.
> 

anybody got a workaround for this?
Instead of using display:none you can use visibility:hidden or just make the size really really small, like 1x1 pixels.
Blocks: 1306686
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.