Closed Bug 304188 Opened 19 years ago Closed 18 years ago

find-menu appears in editor element which has had makeEditable() called but designMode not set.

Categories

(Toolkit :: Find Toolbar, defect)

x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: kalle, Assigned: martijn.martijn)

Details

(Keywords: testcase)

Attachments

(4 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

When creating a XUL application with an <editor/> embedded, the editor pane will
bring up a Find-menu when the apostrophe or slash keys are pressed, even though
the user is currently having a caret and is able to edit the document on the page.
This only happens when editor.makeEditable() has been called, but the content
document's designMode property has NOT been set to "on".

Reproducible: Always

Steps to Reproduce:
1. create XUL document with <editor id="myEditor"/> embedded
2. set onload for document to: document.getElementById('myEditor').makeEditable();
3. load page and type text, then type apostrophe (') and/or slash (/).

Actual Results:  
Find menu pops up at bottom.

Expected Results:  
Uninterrupted editing.

Setting designMode to "on" will solve the issue.
Attached file Testcase
This testcase needs to be tested locally and be given the privilege it asks.
I'm seeing this also in the latest trunk build.
Status: UNCONFIRMED → NEW
Component: Keyboard Navigation → Find Toolbar / FastFind
Ever confirmed: true
Keywords: testcase
QA Contact: keyboard.navigation → fast.find
The bug also happens with typing any arbritrary character when I have the option
"Begin finding when you begin typing" checked.
Attached patch toolkit patch (obsolete) — Splinter Review
Ok, this fixes it for me. I think I can safely remove designMode check here.
I've checked it and it seems to work (although not a 'live' test since
designmode is not working in current trunk builds, known bug).
Attached patch typeaheadfind patch (obsolete) — Splinter Review
This is the typeaheadfind patch, which is used in SeaMonkey's build. I have no
idea if this works, because I've no SeaMonkey build lying around here. I only
know it compiles.
Comment on attachment 192283 [details] [diff] [review]
toolkit patch

Ok, designmode is working now again and I've verified I can safely remove it.
Attachment #192283 - Flags: review?(mconnor)
Comment on attachment 192286 [details] [diff] [review]
typeaheadfind patch

this one isn't necessary. Seamonky already works correct.
Attachment #192286 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee: nobody → martijn.martijn
Status: ASSIGNED → NEW
Comment on attachment 192283 [details] [diff] [review]
toolkit patch

>Index: toolkit/components/typeaheadfind/content/findBar.js

>+  if (win) {
>+    var navigation = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                     getInterface(Components.interfaces.nsIWebNavigation);
>+    var docShell = navigation.QueryInterface(Components.interfaces.nsIDocShell);
>+    var webNavigation = docShell.QueryInterface(Components.interfaces.nsIWebNavigation);
>+    var editingSession = webNavigation.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                    getInterface(Components.interfaces.nsIEditingSession);

This is wrong... you QI to nsIWebNavigation and nsIDocShell unnecessarily. This could just be:

editingSession = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                    .getInterface(Components.interfaces.nsIWebNavigation)
                    .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                    .getInterface(Components.interfaces.nsIEditingSession);
Attached patch patch2 (obsolete) — Splinter Review
Thanks Gavin!
This seems to work fine.
Attachment #192283 - Attachment is obsolete: true
Attachment #203808 - Flags: review?(mconnor)
Attachment #192283 - Flags: review?(mconnor)
A possible workaround is to suppress keypress events from bubbling, using something along the lines of:

 editor.contentWindow.addEventListener("keypress", function (event) {
                                           event.preventBubble();
                                       }, true);

where editor is the <editor> element.

But this may break other stuff which relies on them further up the tree. At least that's what I've gathered from http://xulplanet.com/forum/viewtopic.php?t=308&highlight=editor and irc.
preventBubble doesn't work anymore in current trunk builds:
http://developer.mozilla.org/en/docs/Gecko_1.9_Changes_affecting_websites
Instead, one should use stopPropagation() (that one also works on older builds).
(In reply to comment #11)

> preventBubble doesn't work anymore in current trunk builds:
> http://developer.mozilla.org/en/docs/Gecko_1.9_Changes_affecting_websites
> Instead, one should use stopPropagation() (that one also works on older
> builds).

Confirmed, this works as well (at least with FF 1.5.0.3). But nevertheless, it would be nice if the existing patch was integrated.

Attached patch Updated patch2Splinter Review
Ok, I updated the patch, and tested that it still worked correctly on the testcase.
Now I expect a certain review from someone ;)
Attachment #203808 - Attachment is obsolete: true
Attachment #247586 - Flags: review?(mano)
Attachment #203808 - Flags: review?(mconnor)
Comment on attachment 247586 [details] [diff] [review]
Updated patch2

r=mano if you catch the exception here as in http://lxr.mozilla.org/seamonkey/source/browser/base/content/nsContextMenu.js#563
Attachment #247586 - Flags: review?(mano) → review+
Flags: in-testsuite?
Checking in findbar.xml;
/cvsroot/mozilla/toolkit/content/widgets/findbar.xml,v  <--  findbar.xml
new revision: 1.4; previous revision: 1.3
done

Checked in on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attached file Unit test (obsolete) —
I finally managed to get a working test, this is appr. what you were looking for, not?
This testcase doesn't work well, when it is tested with an application that hasn't editor built.
Attachment #247750 - Flags: review?(mano)
Comment on attachment 247750 [details]
Unit test

   - The Initial Developer of the Original Code is
   - Mozilla Corporation.
   - Portions created by the Initial Developer are Copyright (C) 2006
   - the Initial Developer. All Rights Reserved.
   -
   - Contributor(s):
   -     Asaf Romano 

List yourself instead? :)

  <script type="application/javascript"><![CDATA[
    function _delayedOnLoad() {
      gBrowser.contentWindow.focus();
      enterStringIntoEditor("'");
      enterStringIntoEditor("/");

Do you really need both?

      ASSERT(gFindBar.getAttribute("hidden") == "true",

nit: use the hidden property.

    function enterStringIntoEditor(aString) {
      netscape.security.PrivilegeManager.enablePrivilege(
		'UniversalXPConnect UniversalBrowserRead');

No need to do this, this test has to run from chrome:// uri anyway.

r=mano otherwise.
Attachment #247750 - Flags: review?(mano) → review+
Attached patch Unit test v2 (obsolete) — Splinter Review
Ok, this addresses the comments, except:
(In reply to comment #18)
> Do you really need both?

Well, I don't really see the harm of using both, but if you feel strongly about it, I can remove one of them.

I've named the file 304188.test.xul, is that a good name. Where should this be checked in?
Attachment #247750 - Attachment is obsolete: true
Attached file Unit test v2 (obsolete) —
Title fix.
Attachment #247945 - Attachment is obsolete: true
No where until we figure out how to run these tests (thus the in-testsuite? flag).
Comment on attachment 247946 [details]
Unit test v2

s/gFindBar.hidden == true/gFindBar.hidden/ on checkin please
Attachment #247946 - Attachment is patch: false
Attachment #247946 - Attachment mime type: text/plain → application/vnd.mozilla.xul+xml
Attached patch converted to mochikit chrometest (obsolete) — Splinter Review
Sorry, I kinda forget about this test.
Attachment #261490 - Flags: review?(mano)
Attachment #261490 - Attachment mime type: application/octet-stream → text/plain
Attachment #261490 - Attachment is patch: true
Comment on attachment 261490 [details] [diff] [review]
converted to mochikit chrometest

>Index: toolkit/content/tests/chrome/bug304188_window.xul
>===================================================================

>+    }
>+    function finish() {
>+      window.close();
>+      window.opener.wrappedJSObject.SimpleTest.finish();
>+    }
>+
>+    function onLoad() {
>+      gFindBar = document.getElementById("FindToolbar");
>+      gBrowser = document.getElementById("content");
>+      var navigator = gBrowser.contentWindow
>+                              .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                              .getInterface(Components.interfaces.nsIWebNavigation);
>+      var docShell = navigator.QueryInterface(Components.interfaces.nsIDocShell);
>+      var webnav = docShell.QueryInterface(Components.interfaces.nsIWebNavigation);
>+      var edsession = webnav.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                            .getInterface(Components.interfaces.nsIEditingSession);

Couldn't you simplify this to:

>+      var webnav = gBrowser.webNavigation;
>+      var edsession = webnav.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                            .getInterface(Components.interfaces.nsIEditingSession);

>+      edsession.makeWindowEditable(gBrowser.contentWindow, 'html', false);
>+      setTimeout(_delayedOnLoad, 500);

hrm, why 500? is makeWindowEditable async?

>+  <commandset>
>+    <command id="cmd_find" oncommand="document.getElementById('FindToolbar').onFindCommand();"/>
>+  </commandset>

you don't need the commandset for this test.

>+  <browser id="content" flex="1" src="data:text/html;charset=utf-8,some%20random%20text" type="content-primary"/>
>+  <findbar id="FindToolbar" browserid="content"/>
>+</window>


r=mano otherwise.
Attachment #261490 - Flags: review?(mano) → review+
ping ping, has this landed?
pong, pong, no, not yet, sorry.
This addresses your previous comments. I also corrected some wrongly referenced bug numbers in test_bug304188.xul. And I retested this.
Attachment #247946 - Attachment is obsolete: true
Attachment #261490 - Attachment is obsolete: true
Attachment #264104 - Flags: review?(mano)
Comment on attachment 264104 [details] [diff] [review]
converted to mochikit chrometest v2

s/'html'/"html"/

r=mano, thanks.
Attachment #264104 - Flags: review?(mano) → review+
Checking in Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/chrome/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/chrome/bug304188_window.xul,v
done
Checking in bug304188_window.xul;
/cvsroot/mozilla/toolkit/content/tests/chrome/bug304188_window.xul,v  <--  bug30
4188_window.xul
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/chrome/test_bug304188.xul,v
done
Checking in test_bug304188.xul;
/cvsroot/mozilla/toolkit/content/tests/chrome/test_bug304188.xul,v  <--  test_bu
g304188.xul
initial revision: 1.1
done

Checked the mochikit chrometest in (with the s/'html'/"html"/ correction). Again, sorry for the delay.
Flags: in-testsuite? → in-testsuite+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: