935784 - [e10s] Using keyboard shortcuts while a text input is focused causes the shortcut to trigger

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

If you have the chat window open in GMail, and you try to type a message that contains a "single quote" character, you will notice the find bar appear and anything you type at that point goes into the find bar.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Updating title since I noticed that this occurs in any text input, including the text input fields on this very bugzilla page

Comment 2

[Reuben Morais [:reuben]]

Also happens for Ctrl/Cmd+Left/Right (for navigating history and selecting text), / for quick search, and any other shortcut that should not work while an input element is focused.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That's weird. I used to have this bug, but I thought Tom fixed it. It doesn't happen to me on Linux anymore. Maybe it is platform-specific.

Comment 4

[Reuben Morais [:reuben]]

I'm on OS X, and it looks like Tim is on Windows, so it looks like it was only fixed on Linux.

Comment 5

[Tom S [:evilpie]]

This is indeed weird. The fix should be platform independent. Maybe there is some underlying issue on windows. Or maybe the gmail mail input box is not something we consider a text field and thus show the findbar.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Would you guys mind trying to find a more specific STR? I tested on a Mac running 10.9 by visiting this bug and focusing on the comment box. Typing / and ' doesn't cause the findbar to come up. Cmd-Left/Right does cause navigation to happen, which is a known issue. However, I expect that the / and ' issues are a lot more annoying, so I'd like to concentrate those first.

Comment 7

[Reuben Morais [:reuben]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Would you guys mind trying to find a more specific STR? I tested on a Mac
> running 10.9 by visiting this bug and focusing on the comment box. Typing /
> and ' doesn't cause the findbar to come up.

Try doing it on the Google search input.

> Cmd-Left/Right does cause
> navigation to happen, which is a known issue. However, I expect that the /
> and ' issues are a lot more annoying, so I'd like to concentrate those first.

Can you point to the relevant bugs/code? I want to dogfood e10s but I really need those shortcuts to work. How can I help?

Comment 8

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #7)
> (In reply to Bill McCloskey (:billm) from comment #6)
> > Would you guys mind trying to find a more specific STR? I tested on a Mac
> > running 10.9 by visiting this bug and focusing on the comment box. Typing /
> > and ' doesn't cause the findbar to come up.
> 
> Try doing it on the Google search input.

(ie. it happens on <input>s, not <textarea>s)

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hmm, weird. I tried google.com and I'm not seeing the problem with / or '.

The specific bug for / and ' is bug 917406. The more general bug, which should cover Cmd-Left/Right, is bug 862519. It started out as a bug to fix the handling of backspace, but it morphed to cover all keys that are significant in both chrome and content.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

This happens for me on Google, Bugzilla, and various other websites in a local build created from today's m-c head.

For example the apostrophes in the previous sentence were added by using copy+paste rather than using the apostrophe key.

I'm running Windows 8.1 on a Lenovo IdeaPad Yoga 13.

Let me know if I can provide any useful debugging info!

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, the patch that was supposed to fix this added code here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#684

It would help to see if any errors show up in the browser console when you type ' or /. Anything more than that requires experience changing the Firefox frontend. Let me know if you know how to do that.

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I added some logging to find out what those values are set to when I hit that piece of code. Adding the logging caused me to build toolkit/content, and building toolkit/content caused the bug to disappear! I've reverted the change and still can't repro in my local build anymore. I'll post back if it re-occurs.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I was able to repro again and we had a mini debugging session in #e10s.

The line mentioned in comment 11:
   elt = elt.contentDocument.activeElement;

That line did execute while I was debugging, and after it executed, elt was an instance of HTMLIFrameElement, which presumably isn't input-like enough for the subsequent lines to believe that an input element is focused.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: findbar-fix

I'd like to fix this as soon as possible. Having the child trigger the findbar makes more sense, but it's easier to do it this way.

I also consolidated the new code with the existing stuff for sending contentWindow.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 831121 [details] [diff] [review]
findbar-fix

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

::: toolkit/content/widgets/remote-browser.xml
@@ +86,5 @@
>  
>        <field name="_contentWindow">null</field>
>  
>        <property name="contentWindow"
> +                onget="return this.syncHandler.getContentWindow();"

I don't think we should do this for contentwindow. This is going to add an other round trip. getFocusedElement is very cold, but browser.contentWindow is used more often.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: findbar-fix v2

OK, good point. Here's a new version.

Comment 17

[Tom S [:evilpie]]

Comment on attachment 831703 [details] [diff] [review]
findbar-fix v2

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

Looks good, thanks Bill.

::: toolkit/content/widgets/remote-browser.xml
@@ +95,5 @@
>                  readonly="true"/>
>  
> +      <field name="_syncHandler">null</field>
> +
> +      <property name="syncHandler"

I wish we could make this private somehow. I don't want random stuff to touch this. But I don't have a very good idea right now.

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I still see this issue, even with the patch applied. Is that expected?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #18)
> I still see this issue, even with the patch applied. Is that expected?

No, that's not expected. I have a testcase that uses iframes and this fixes it. There may be more issues though. Would you mind doing more debugging with the patch applied?

Comment 20

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 831703 [details] [diff] [review]
findbar-fix v2

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

::: toolkit/content/widgets/findbar.xml
@@ +682,5 @@
>  
>            // Temporary fix for e10s.
>            if (elt instanceof XULElement && elt.tagName == "xul:browser" &&
>                elt.getAttribute("remote")) {
> +            elt = elt.syncHandler.getFocusedElement();

In the gmail "hangouts" chat box, when I type an apostrophe and reach this |if| statement:
  - |elt| is a XULElement
  - |elt.tagName| is "browser" not "xul:browser" :(

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: findbar-fix v3

Thanks Tim. Can you see if this one works? I think it deals with the namespace better.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Now this line executes:
   elt = elt.syncHandler.getFocusedElement();

But after that line executes, |elt| is a |HTMLDivElement| so the subsequent code still doesn't realize that an input element has focus.

On the plus side, this issue isn't occurring in bugzilla at all anymore. I'm not sure when exactly that became the case, but it's rather freeing to be able to type 's again :)

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: findbar-fix v4

This fixes the Google Hangout issue for me. It turns out that there was some later code that was covering the DIV case, and we weren't using CPOWs there.

Tim, can you make check to see if this works everywhere for you?

Comment 24

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Works for me in the Gmail Hangout thing as well. That was really my only test case. Looks good!

Comment 25

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 831969 [details] [diff] [review]
findbar-fix v4

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

I gave the code a once-over but someone else should probably be providing the feedback on this patch ;)

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77f53f9d3099

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77f53f9d3099

Comment 28

[Ed Morley [:emorley]]

Backed out along with the other changesets in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b92529577644 for something in that push causing shutdown hangs on OS X, eg:

https://tbpl.mozilla.org/php/getParsedLog.php?id=30796582&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30802180&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30816360&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30818266&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/mozilla-central/rev/f020616fe213

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5dd91e7d90f

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e5dd91e7d90f

Comment 31

[Reuben Morais [:reuben]]

This bug only fixed the shortcut findbars. Is there a bug for general text editing shortcuts? Cmd+Left/Right, Option+Left/Right, etc.

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think that should be fixed when bug 862519 and bug 977904 land.