No editable iframe with these steps to follow

VERIFIED FIXED in mozilla1.9beta1

Status

()

Core
Editor
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: cpearce)

Tracking

({regression, testcase})

Trunk
mozilla1.9beta1
x86
Windows XP
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCc], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
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.
Requesting blocking for the regression
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RwCc]
(Assignee)

Comment 2

10 years ago
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).

Assignee: nobody → chris
(Assignee)

Comment 3

10 years ago
Created attachment 286767 [details]
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.
(Assignee)

Comment 4

10 years ago
Created attachment 286770 [details] [diff] [review]
Patch v1 - don't call MakeWindowEditable() when we don't have a presShell

Daniel, do you have time to review this?
Attachment #286770 - Flags: review?(daniel)
(Reporter)

Comment 5

10 years ago
Chris, does this also fix bug 263392?
(Assignee)

Comment 6

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

Comment 7

10 years ago
Created attachment 286903 [details] [diff] [review]
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.
Attachment #286770 - Attachment is obsolete: true
Attachment #286903 - Flags: review?(daniel)
Attachment #286770 - Flags: review?(daniel)
(Assignee)

Updated

10 years ago
Duplicate of this bug: 263392

Comment 9

10 years ago
Comment on attachment 286903 [details] [diff] [review]
Patch v2

r=brade but please remove the extra blank line added at the end.l
Attachment #286903 - Flags: review+
(Assignee)

Comment 10

10 years ago
Created attachment 287005 [details] [diff] [review]
Patch v3 - v2 with brade's requested change

Patch with brade's suggested change, carrying forward r=brade.
Attachment #286903 - Attachment is obsolete: true
Attachment #287005 - Flags: superreview?(roc)
Attachment #287005 - Flags: review+
Attachment #286903 - Flags: review?(daniel)
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!
Attachment #287005 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 12

10 years ago
Created attachment 287009 [details] [diff] [review]
Patch v4

Patch v3 with Roc's suggestions. r=brade sr=roc.
Attachment #287005 - Attachment is obsolete: true
Attachment #287009 - Flags: superreview+
Attachment #287009 - Flags: review+
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!
Attachment #287009 - Flags: approvalM9?
Comment on attachment 287009 [details] [diff] [review]
Patch v4

Approving to land for beta.
Attachment #287009 - Flags: approvalM9? → approvalM9+
Checked in.

Chris, could you create a mochitest for this? Should be fairly easy.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Comment 16

10 years ago
Created attachment 287043 [details] [diff] [review]
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.
Attachment #287043 - Flags: review?(roc)
(Assignee)

Comment 17

10 years ago
Created attachment 287047 [details] [diff] [review]
Mochitest v2 - s/spaces/tabs/ in Makefile

Same as first mochitest patch, but spaces replaced with tabs in Makefile.
Attachment #287043 - Attachment is obsolete: true
Attachment #287047 - Flags: review?(roc)
Attachment #287043 - Flags: review?(roc)
Comment on attachment 287047 [details] [diff] [review]
Mochitest v2 - s/spaces/tabs/ in Makefile

excellent
Attachment #287047 - Flags: review?(roc) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9 M9
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?
Depends on: 402284

Comment 21

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

Comment 22

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

10 years ago
(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. 

(Reporter)

Comment 24

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110405 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
(Reporter)

Updated

10 years ago
Flags: in-testsuite? → in-testsuite+
Depends on: 439965
You need to log in before you can comment on or make changes to this bug.