389350 - No editable iframe with these steps to follow

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

See testcase, which is from bug 263392.

To reproduce:
- Click on the designMode on button
- Click on the set iframe to display block button
- Click on the designMode on button again.

Expected result:
The document in the iframe should be editable.

Actual result:
The document in the iframe is not editable.

These particular steps regressed with the landing of the patch for bug 237964.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Requesting blocking for the regression

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem is that when we set the iframe to be design mode while its display:none, we call SetDesignMode() which calls nsEditingSession::SetupEditorOnWindow() which fails.

Call stack: 

>	composer.dll!nsEditingSession::SetupEditorOnWindow(nsIDOMWindow * aWindow=0x04d7c3a0)  Line 300	C++
 	composer.dll!nsEditingSession::MakeWindowEditable(nsIDOMWindow * aWindow=0x04d7c3a0, const char * aEditorType=0x02f3b094, int aDoAfterUriLoad=0, int aMakeWholeDocumentEditable=0, int aInteractive=1)  Line 223 + 0x12 bytes	C++
 	gklayout.dll!nsHTMLDocument::EditingStateChanged()  Line 3988 + 0x31 bytes	C++
 	gklayout.dll!nsHTMLDocument::SetDesignMode(const nsAString_internal & aDesignMode={...})  Line 4123 + 0xe bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x0000001d, unsigned int methodIndex=1, unsigned int paramCount=1235076, nsXPTCVariant * params=0x0215bf0d)  Line 102	C++

This leaves the mEditingState in the state eSettingUp, which blocks the event listeners being set on the window when its reset when we set the display:block later on.

The call fails because the nsHtmlDocument's window doesn't have a presShell at the time, and at nsEditingSession.cpp:464 we bail out if we don't have one. There's also a comment in there saying "we really shouldn't need a presShell here!".

There are two solutions:

1. Change nsHTMLDocument::EditingStateChanged() so that it doesn't call MakeWindowEditable() when  the window doesn't have a presShell.
2. Move selection out of the presShell into the document, which as a side effect means that the selection on display:none wouldn't require presShell. This sounds like a bit of work.

I already have a patch for 1, and it works (because we come back and call MakeWindowEditable() again when display:block is set later on) and it works.

I'll run some tests to see if we need to do 2.

(Taking bug).

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Test case - comparison of IE/FF selection manipulation with style={none,block}

This compares the behaviour of IE and Minefield in how selection manipulation is handled with display:none and display:block.

This shows that IE7 cannot manipulate selection when working with a display:none iframe. To reproduce IE7's deficiency:

1. Start IE, load the file "testcase-compatibility.html" in this attachment.
2. Click "set iframe to display block" button. Note iframe becomes visible.
3. Click "Select item1", note that the text in the iframe is selected.
4. Click somewhere to clear the selection.
5. Click "set iframe to display none" button. Note iframe disappears.
6. Click "Select item1", note that we get JS error 800a025e.

It is interesting though, that in IE7 if you select the item, and make the iframe display:none and then display:block, then the selection does not get cleared (it does in Minefield).

So I propose that I submit my patch which prevents us from calling MakeWindowEditable() when we don't have a presShell, fixing this bug, and we can come back later to allow selection to be manipulated when the content is display:none.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - don't call MakeWindowEditable() when we don't have a presShell

Daniel, do you have time to review this?

Comment 5

[Martijn Wargers (dead)]

Chris, does this also fix bug 263392?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Almost, it does fix the "design mode not working" problem, and it appears that the flickering doesn't happen anymore anyway. The assertion still fails, so I'll modify the patch to prevent the bad code which causes it from getting run.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

This is similar to Patch v1, but it also prevents the editor from being initialized when we don't have a presShell, and so don't call makeWindowEditable(). makeWindowEditable() creates the editor, so if we don't call makeWindowEditable() but do try to setup the editor later in nsHTMLDocument::EditingStateChanged() (as patch v1 did) we'll fail and this throws an exception. This was identified in bug 263392, and this patch also fixes bug 263392.

Comment 9

[Kathleen :Brade]

Comment on attachment 286903 [details] [diff] [review]
Patch v2

r=brade but please remove the extra blank line added at the end.l

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - v2 with brade's requested change

Patch with brade's suggested change, carrying forward r=brade.

Comment 11

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

Comment on attachment 287005 [details] [diff] [review]
Patch v3 - v2 with brade's requested change

+  if (!docShell) {
+    return PR_FALSE;
+  }

Prevailing style here is to allow "if (...) return" to not use braces around the return.

+  if (presShell) {
+    return PR_TRUE;
+  } else {
+    return PR_FALSE;
+  }

Just "return presShell != nsnull;"

Otherwise great!

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

Patch v3 with Roc's suggestions. r=brade sr=roc.

Comment 13

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

Comment on attachment 287009 [details] [diff] [review]
Patch v4

Seeking M9 approval because the new GMail code that's being rolled out *right now* hits this bug, with the effect that clicking "Compose Mail" gives you a mail body that you can't edit!

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 287009 [details] [diff] [review]
Patch v4

Approving to land for beta.

Comment 15

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

Checked in.

Chris, could you create a mochitest for this? Should be fairly easy.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest

(In reply to comment #15)
> Checked in.
> 
> Chris, could you create a mochitest for this? Should be fairly easy.
> 

This mochitest creates a display:none iframe, sets it into design mode, sets it to display:block, then sends key pressess and ensures that the content was added.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest v2 - s/spaces/tabs/ in Makefile

Same as first mochitest patch, but spaces replaced with tabs in Makefile.

Comment 18

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

Comment on attachment 287047 [details] [diff] [review]
Mochitest v2 - s/spaces/tabs/ in Makefile

excellent

Comment 19

[Reed Loden [:reed]]

Checking in editor/composer/test/Makefile.in;
/cvsroot/mozilla/editor/composer/test/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/editor/composer/test/test_bug389350.html,v
done
Checking in editor/composer/test/test_bug389350.html;
/cvsroot/mozilla/editor/composer/test/test_bug389350.html,v  <--  test_bug389350.html
initial revision: 1.1
done

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 287009 [details] [diff] [review]
Patch v4

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================

>+  PRBool makeWindowEditable = (mEditingState == eOff) && HasPresShell(window);
>+  if (!makeWindowEditable) {
>+    // We should not make the window editable or setup its editor.
>+    // It's probably style=display:none.
>+    return NS_OK;
>+  }

I haven't looked in detail, but this early return looks wrong. There's more code in that function that needs to run if mEditingState != eOff. Why not just return early if !HasPresShell(window) and leave the rest of the function as is?

Comment 21

[Recall]

I would like this bug to be backed ou. It has caused WYSIWYG problems on vbulletin sites.

There is inconsistent right clicking in the form field now. You have to be exactly next to the cursor, where before it was anywhere within the box.

Test Site:

http://futuremark.yougamers.com/forum/forumdisplay.php?f=70

Steps to reproduce:

Navigate to here http://futuremark.yougamers.com/forum/forumdisplay.php?f=70
Register and make a post
Try and paste a link in the form field will only work while being right next to the cursor

Expected behavior

Right clicking anywhere in the form field should allow pasting.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Peter V's patch for bug 402284 fixes this, perhaps we should move that into 1.9?(In reply to comment #21)
> I would like this bug to be backed ou. It has caused WYSIWYG problems on
> vbulletin sites.

Peter V's patch for bug 402284 fixes this, perhaps we should move that into 1.9?

Comment 23

[Recall]

(In reply to comment #22)
> Peter V's patch for bug 402284 fixes this, perhaps we should move that into
> 1.9?(In reply to comment #21)
> > I would like this bug to be backed ou. It has caused WYSIWYG problems on
> > vbulletin sites.
> 
> Peter V's patch for bug 402284 fixes this, perhaps we should move that into
> 1.9?
> Thanks and yes it should defo be pushed considering the wide use of WYSIWYG forums and also vbulletin as a whole. 

Comment 24

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110405 Minefield/3.0a9pre