936933 - Unable to scroll overflowing block with arrow keys if page contains a contenteditable element

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Boris Zbarsky [:bzbarsky]]

TESTCASE:

data:text/html,<div style="overflow: auto; white-space: nowrap; width: 100px">Some text that is very long and just doesn't fit at all</div><p contenteditable="true"></p>

STEPS TO REPRODUCE:

1)  Tab to the scrollable div.
2)  Hit the right arrow key.

EXPECTED RESULTS:  Scrolls to the right.

ACTUAL RESULTS: Nothing happens.

The scrolling works if the contenteditable element is removed, so it's possible that the editor is adding some broken event listeners here.

Comment 1

[(no longer active)]

The keypress event listener that the editor uses is installed in nsEditorEventListener::InstallToEditor.  How is it broken?

Comment 2

[Mats Palmgren (inactive)]

WFM on Linux and Win7.  I'd guess it has something to do with those invisible OSX scrollbars.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I have those turned off, but I can still reproduce....

Comment 4

[Boris Zbarsky [:bzbarsky]]

So what happens here is that we land in nsXBLWindowKeyHandler::WalkHandlers which does:

324   bool isEditor;
325   nsresult rv = EnsureHandlers(&isEditor);
....

355   if (isEditor && GetEditorKeyBindings()) {
...
370         handled = sNativeEditorBindings->KeyPress(*keyEvent,
371                                                   DoCommandCallback,
372                                                   controllers);
....
388     if (handled)
389       aKeyEvent->PreventDefault();


In this case isEditor ends up true, because that seems to be based off of the GetSelectionFlags() on the presshell and we apparently set that up if the document has any designMode things.  So we land in NativeKeyBindings::KeyPress over in NativeKeyBindings.mm, this finds the "cmd_charNext" command (in my case, since I pressed the right arrow key) and does aCallback(geckoCommand, aCallbackData).  Then it claims it handled the keypress.

But of course in this case aCallback is DoCommandCallback, which immediately returns because aData is null, which happens because root->GetControllers() returned null.

It's not clear to me whether this is a bug in the nsXBLWindowKeyHandler::WalkHandlers code (should assume null controllers means not handled) or in the nsINativeKeyBindings API (should let the callback function which is actually doing the handling call preventDefault on the event as needed)...  But the combination leads to breakage.

Note that we have no nsINativeKeyBindings on Windows, which is why the bug doesn't appear there.  On Linux, nsNativeKeyBindings::KeyPress only returns true if nsNativeKeyBindings::KeyPressInternal returns true, which only happens if the global variable (!) gHandled gets set to true, which is done by all the functions that actually do anything under this method.  In particular, this does not call back into the Gecko command machinery at all, unlike the Mac code.

I wonder who even owns this code nowadays.  Olli, do you know?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I guess I or masayuki own that code.

Looking (after reading the rest of the bugmail)

Comment 6

[(no longer active)]

Neil, please see comment 4.  Thanks!

Comment 7

[[e-mail]]

Although bug 282097 added Mac native key bindings, it didn't actually make any significant changes to the XBL side of things, in particular the only function it changed was DoCommandCallback and then only to add an extra check that the command was actually enabled.

I don't understand the bit about the Linux keybindings not calling back into the Gecko command machinery. gHandled is set to true after calling gCurrentCallback (which is always DoCommandCallback). The difference on Linux appears to be that the motion keys are handled by platformHTMLBindings.xml in the first place.

I also don't understand the bit about not having any controllers. It's vaguely possible that it's not trying hard enough to find some controllers; compare nsXBLPrototypeHandler::DispatchXBLCommand.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I also don't understand the bit about not having any controllers

Dunno.  Just what my debugger claimed.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The isEditor should be false, in this case...

Comment 10

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> The isEditor should be false, in this case...

Why do you think that?  I'm fairly sure that it should be true...

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > The isEditor should be false, in this case...
> 
> Why do you think that?  I'm fairly sure that it should be true...

When the scrollable div has focus, the editor of the document doesn't have focus actually since the editor should handle events only when their target is the editing host of the editor.

In nsEditorEventListener, ns(HTML)Editor::IsAcceptableInputEvent() is used for checking if the editor actually has focus. So, similar check is necessary before using native editor key bindings.

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > > The isEditor should be false, in this case...
> > 
> > Why do you think that?  I'm fairly sure that it should be true...
> 
> When the scrollable div has focus, the editor of the document doesn't have
> focus actually since the editor should handle events only when their target is
> the editing host of the editor.
> 
> In nsEditorEventListener, ns(HTML)Editor::IsAcceptableInputEvent() is used for
> checking if the editor actually has focus. So, similar check is necessary
> before using native editor key bindings.

Oh, I see what you mean now.  Yes, I think that makes sense.  That's probably the right fix here.

Comment 13

[[e-mail]]

(In reply to comment #7)
> I also don't understand the bit about not having any controllers. It's
> vaguely possible that it's not trying hard enough to find some controllers;
> compare nsXBLPrototypeHandler::DispatchXBLCommand.

OK, so the window root only finds the controllers for the focused element, which needs to be a XUL element or a text area, input or contenteditable element. This is an unnecessary step as the window root already knows how to execute a command.

Comment 14

[[e-mail]]

Attachment: Possible patch

This should work, but I'm on Windows, so all I know is that it compiles.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8333574 [details] [diff] [review]
Possible patch

Fixes the str, at least.  ;)

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8333574 [details] [diff] [review]
Possible patch

I'm not sure if I'm really the best reviewer for this, but it looks OK to me.

Comment 17

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/253bf04e5c49

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/253bf04e5c49

Comment 19

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0		

Reproduced the initial issue on a nightly build from 2013-11-10 and verified that the issue is fixed on latest Aurora 28.0a2.