Closed Bug 994063 Opened 10 years ago Closed 9 years ago

Typing ' or / on a page that contains contenteditable attribute is not opening the quickfind bar.

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: VarCat, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files)

Attached file test.html
Environment:

FF 31
Build Id: 20140409030203
OS: Win 7 x64, Mac Os X 10.9.2, Ubuntu 13.04 x64

Steps to reproduce:
1. Open the attached page.
2. Be sure that the focused is set outside of the "contenteditable" field.
3. Type ' or / 

Expected:
Quickfind bar is opened

Actual:
Nothing happens.

Note:
After pressing the download link the shortcut is working as expected.

The bug is reproducible on Firefox 4 so this is not a regression.
Oh wow. The quickfind bar is so annyoing. Thanks for filing this.
If I can help you with anything please let me know.
I found the similar bug 936933, about contenteditable in a page eating key presses.
Root cause may be here. I'll need to compile to investigate further:

dom/xbl/nsXBLWindowKeyHandler.cpp:249
    // Now determine which handlers we should be using.
    if (IsHTMLEditableFieldFocused()) {
      sXBLSpecialDocInfo->GetAllHandlers("editor", &mHandler, &mUserHandler);
    }
    else {
      sXBLSpecialDocInfo->GetAllHandlers("browser", &mHandler, &mUserHandler);
    }

I'm guessing the contenteditable div is considered to have focus, so the '/' press is passed to the editor's handler (which is at editor/libeditor/nsEditorEventListener.cpp).
Component: Find Toolbar → Event Handling
Product: Toolkit → Core
Version: unspecified → Trunk
Moving to Core->Event Handling (similar to bug 936933) as I think the issue is there.
Flags: needinfo?(ehsan)
Sorry, switching back to the Find Toolbar component, since I narrowed it down to this test:

toolkit/modules/BrowserUtils.jsm:shouldFastFind()

  if (editingSession.windowIsEditable(win))
    return false;

Not sure why this test exists.
Component: Event Handling → Find Toolbar
Product: Core → Toolkit
Sorry for spamming with comments instead of waiting to collect more info. Got a bit excited.

The test above, that checks if the window is editable was added in bug 304188.

The test-case for that bug no longer works. It's a XUL that calls makeEditable() on an <editor> object.
Is this still relevant?

shouldFastFind() already checks for contenteditable a few lines earlier:
  if (elt.isContentEditable)
    return false;

Removing the windowIsEditable() test makes the testcase attached to this bug work:
1) Pressing '/' while no element is focused opens the Find Toolbar.
2) Pressing '/' while the contenteditable div is focused types a '/' and does not open the Find Toolbar.

... but does it affect anything negatively?
Afaik, there are still XUL editors out there.
Ori, if I understand you correctly, this is a regression from bug 304188?
(In reply to Martijn Wargers [:mwargers] (QA) from comment #8)
> Afaik, there are still XUL editors out there.
> Ori, if I understand you correctly, this is a regression from bug 304188?

In some sense, yes. This piece of code is the only thing preventing '/' from opening the Quick find bar.
However, I'm not sure bug 304188 broke something when it was first introduced - was contenteditable around at the time?

If the code is still required for XUL editors:
1) Can you point me at a XUL editor so I can test it?
2) Perhaps the code should specifically check for a XUL editor and not a regular browser window.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #8)
> Afaik, there are still XUL editors out there.

We already support [disablefastfind], those applications can use that.
Flags: needinfo?(ehsan)
Thanks for your investigation, Ori!  I will submit a patch soon.
Comment on attachment 8673277 [details] [diff] [review]
Properly disable the quick-find bar only if an editable element is focused, or if the document is in design mode

Review of attachment 8673277 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

Thanks for picking this up, Ehsan!

::: toolkit/content/tests/browser/browser.ini
@@ +47,5 @@
>  [browser_mute.js]
>  [browser_mute2.js]
>  skip-if = buildapp == 'mulet' || buildapp == 'b2g'
> +[browser_quickfind_editable.js]
> +skip-if = e10s # synthesizeKey() doesn't work in e10s mode

...except when the patch I just r+'ed in bug 1140512 lands, we will have support for `sendChar`. Would you like to wait for that, adjust the test a bit and land without disabling this test in e10s mode?

::: toolkit/content/tests/browser/browser_quickfind_editable.js
@@ +1,1 @@
> +const PAGE = "data:text/html,<div contenteditable>foo</div><input><textarea></textarea>";

Missing: CC license header.

@@ +21,5 @@
> +  });
> +}
> +
> +add_task(function* test_hotkey_on_editable_element() {
> +  yield BrowserTestUtils.withNewTab({

/me just <3 BrowserTestUtils!
Attachment #8673277 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #13)
> ::: toolkit/content/tests/browser/browser.ini
> @@ +47,5 @@
> >  [browser_mute.js]
> >  [browser_mute2.js]
> >  skip-if = buildapp == 'mulet' || buildapp == 'b2g'
> > +[browser_quickfind_editable.js]
> > +skip-if = e10s # synthesizeKey() doesn't work in e10s mode
> 
> ...except when the patch I just r+'ed in bug 1140512 lands, we will have
> support for `sendChar`. Would you like to wait for that, adjust the test a
> bit and land without disabling this test in e10s mode?

I just tried out the patch there locally and it doesn't fix the e10s issue for this test.  Can I land this as is?

> ::: toolkit/content/tests/browser/browser_quickfind_editable.js
> @@ +1,1 @@
> > +const PAGE = "data:text/html,<div contenteditable>foo</div><input><textarea></textarea>";
> 
> Missing: CC license header.

Will fix.

> @@ +21,5 @@
> > +  });
> > +}
> > +
> > +add_task(function* test_hotkey_on_editable_element() {
> > +  yield BrowserTestUtils.withNewTab({
> 
> /me just <3 BrowserTestUtils!

I know, right?!  :-)
Flags: needinfo?(mdeboer)
(In reply to Ehsan Akhgari (don't ask for review please) from comment #14)
> I just tried out the patch there locally and it doesn't fix the e10s issue
> for this test.  Can I land this as is?

I really meant s/synthesizeKey/sendChar/ atop the patch there, but I assume you tried that. Please feel free to land the patch as-is.
Flags: needinfo?(mdeboer)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
(In reply to Mike de Boer [:mikedeboer] from comment #15)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #14)
> > I just tried out the patch there locally and it doesn't fix the e10s issue
> > for this test.  Can I land this as is?
> 
> I really meant s/synthesizeKey/sendChar/ atop the patch there, but I assume
> you tried that.

Yes, that's what I tried.
See Also: → 1215576
https://hg.mozilla.org/mozilla-central/rev/c1cf9458c72a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Confirmed fixed in the latest nightly build.

Thanks for the help!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: