Closed Bug 994063 Opened 7 years ago Closed 5 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)

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: 5 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.