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

Status: RESOLVED FIXED

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

Description

[Ivan Sagalaev]

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.

Comment 1

[Jason Johnston]

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 ***

Comment 2

[Vladimir Ermakov]

verifying

Comment 3

[Cesar Eduardo Barros]

Attachment: Testcase (1/3): iframe contents

Comment 4

[Cesar Eduardo Barros]

Attachment: Testcase (2/3): with display: none

Comment 5

[Cesar Eduardo Barros]

Attachment: Testcase (3/3): with display: block

Comment 6

[Cesar Eduardo Barros]

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

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

reopening per comment 6

Comment 8

[John Keiser (jkeiser)]

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.

Comment 9

[Alexandru Savulov]

--> (no time)

Comment 10

[John Keiser (jkeiser)]

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.

Comment 11

[Ian]

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

Comment 12

[Ian]

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.

Comment 13

[Emmanuel Bourg]

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>

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.. It looks like we just need the prescontext for the GetBidi() stuff. Is
that right?  Is that information really per-prescontext?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Seing that we don't really have a clear idea of what having more then one
prescontext really means i would say "no".

Comment 16

[Boris Zbarsky [:bzbarsky]]

Um... Having more than one prescontext means we're printing, usually.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

sounds like a plan to me...

Comment 19

[Simon Montagu :smontagu]

I don't see any problem with moving the Set and GetBidi() into the document.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Simon, is it OK if multiple document viewers all touch the same bidi options?  I
would say "yes", but just checking....

Comment 21

[Simon Montagu :smontagu]

I'm sorry, I don't understand the question, specifically the part about
"touching" bidi options.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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...

Comment 23

[Johnny Stenback (:jst)]

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?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yeah, combining some of those would be nice.  Is there a reason you just marked
this security-sensitive, though?

Comment 25

[Johnny Stenback (:jst)]

Duh, no.

Comment 26

[WANG]

I've the pb

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

The link works with IE6 and Opera7.54

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

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.

Comment 28

[Johnny Stenback (:jst)]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

You can do it, but the event target will often end up wrong...  e.g. see bug 295340.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 31

[Simon Montagu :smontagu]

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?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 33

[Simon Montagu :smontagu]

(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.

Comment 34

[Simon Montagu :smontagu]

Filed bug 316243 on moving mIsVisual

Comment 35

[:Gavin Sharp [email: [e-mail]]]

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Changing interfaces is fine for FF2

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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

Comment 41

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 43

[Boris Zbarsky [:bzbarsky]]

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?

Comment 44

[Boris Zbarsky [:bzbarsky]]

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).

Comment 45

[Mats Palmgren (inactive)]

Boris, if you intend to checkin this on any branch then please also checkin
bug 344787, thanks.

Comment 46

[Robert Sayre]

Attachment: mochitest

Comment 47

[Robert Sayre]

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

Comment 48

[Phil Ringnalda (:philor)]

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

Comment 49

[Keith Treague]

I'm seeing this bug on firefox 2.0 Was a fix for this put into firefox 2.0?

Comment 50

[Boris Zbarsky [:bzbarsky]]

No.

Comment 51

[Keith Treague]

(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 :-)

Comment 52

[Boris Zbarsky [:bzbarsky]]

At this point, this will not go into Firefox 2.anything.  It'll be fixed in Firefox 3.

Comment 53

[Walid GadElKarim]

(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?

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Instead of using display:none you can use visibility:hidden or just make the size really really small, like 1x1 pixels.