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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: kalle, Assigned: martijn.martijn)
Details
(Keywords: testcase)
Attachments
(4 files, 7 obsolete files)
737 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
1.46 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
Details | Diff | Splinter Review | |
6.52 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
This testcase needs to be tested locally and be given the privilege it asks.
Assignee | ||
Comment 2•19 years ago
|
||
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
Assignee | ||
Comment 3•19 years ago
|
||
The bug also happens with typing any arbritrary character when I have the option "Begin finding when you begin typing" checked.
Assignee | ||
Comment 4•19 years ago
|
||
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).
Assignee | ||
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
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)
Assignee | ||
Comment 7•19 years ago
|
||
Comment on attachment 192286 [details] [diff] [review] typeaheadfind patch this one isn't necessary. Seamonky already works correct.
Attachment #192286 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•19 years ago
|
Assignee: nobody → martijn.martijn
Status: ASSIGNED → NEW
Comment 8•19 years ago
|
||
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);
Assignee | ||
Comment 9•19 years ago
|
||
Thanks Gavin! This seems to work fine.
Attachment #192283 -
Attachment is obsolete: true
Attachment #203808 -
Flags: review?(mconnor)
Attachment #192283 -
Flags: review?(mconnor)
Comment 10•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
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).
Comment 12•18 years ago
|
||
(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.
Assignee | ||
Comment 13•18 years ago
|
||
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 14•18 years ago
|
||
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+
Updated•18 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 15•18 years ago
|
||
Assignee | ||
Comment 16•18 years ago
|
||
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
Assignee | ||
Comment 17•18 years ago
|
||
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 18•18 years ago
|
||
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+
Assignee | ||
Comment 19•18 years ago
|
||
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
Comment 21•18 years ago
|
||
No where until we figure out how to run these tests (thus the in-testsuite? flag).
Comment 22•18 years ago
|
||
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
Assignee | ||
Comment 23•17 years ago
|
||
Sorry, I kinda forget about this test.
Attachment #261490 -
Flags: review?(mano)
Updated•17 years ago
|
Attachment #261490 -
Attachment mime type: application/octet-stream → text/plain
Updated•17 years ago
|
Attachment #261490 -
Attachment is patch: true
Comment 24•17 years ago
|
||
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+
Comment 25•17 years ago
|
||
ping ping, has this landed?
Assignee | ||
Comment 26•17 years ago
|
||
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 27•17 years ago
|
||
Comment on attachment 264104 [details] [diff] [review] converted to mochikit chrometest v2 s/'html'/"html"/ r=mano, thanks.
Attachment #264104 -
Flags: review?(mano) → review+
Assignee | ||
Comment 28•17 years ago
|
||
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+
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•