481626 - contentEditable breaks scrolling with arrows

Status: RESOLVED DUPLICATE of bug 669026

(Core :: XBL, defect)

Description

[Andrew Valums]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090304 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090304 Minefield/3.2a1pre

Applying contentEditable property to an element, breaks page scrolling with keyboard arrows.

Reproducible: Always

Steps to Reproduce:
Try to scroll-down with keyboard arrow on the attached page.

Actual Results:  
Nothing

Expected Results:  
Scrolling down

Comment 1

[Andrew Valums]

Test page
http://valums.com/wp-content/uploads/bugs/ff/contenteditable-scrolling.htm

Comment 2

[Andrew Valums]

Attachment: test page

Comment 3

[arno renevier]

I'd be happy to work on that bug

Comment 4

[arno renevier]

I investigated that bug:
it happens because nsXBLWindowKeyHandler believes it's in a editor document: nsXBLWindowKeyHandler::IsEditor looks if selection flag is DISPLAY_ALL; that's correct for an editor document or for an non editable document, but that's not correct for a partially editable document (contenteditable element).

So, when pressing arrow on a focused non editable element, an editor command will be executed in a non editable context.

It would be better to look at editable status of focused element instead of looking at editable status of full document, but I'm not really sure how to do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Peter might know.  In any case, this is only loosely XBL... nsXBLWindowKeyHandler isn't really XBL in spite of the naming.

Comment 6

[Jaakko Kiviluoto]

This happens also with the facebook.com main page, since they use contentEditable=true on the "What's on your mind?" input field.

Once you click on that field, it gets the editable status, and the whole document becomes caret-browsable - the caret just isn't visible. However, if you now for example click on the plain text of somebody's status update, and then keep the down arrow key pressed for a while, you notice the document eventually starts scrolling down line by line.

Build identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.10 (karmic) Firefox/3.5.5

Comment 7

[Jaakko Kiviluoto]

Attachment: Fix proposal to prevent unfocused editors from consuming keys

- Checks editor status from NODE_IS_EDITABLE flag
- Enables/disables editors based on input focus

The intent is that while we have an editable element on the document, the editor's key handler should not consume key press events when the editable content won't be modified anyway.

Added review request based on comment 5.

Tested on microb-engine 20091202-1.9.2-2

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #7)
> - Enables/disables editors based on input focus

Won't that disable spellchecking on editable regions without focus?

Comment 9

[Jaakko Kiviluoto]

Comment on attachment 416900 [details] [diff] [review]
Fix proposal to prevent unfocused editors from consuming keys

This patch actually had cases when it didn't even work, so obsoleting it. The use case is temporarily patched to microb-engine with kind of a hack, but I don't have any pretty AND working solution for now.

Comment 10

[Andrew Hurle [:ahurle]]

If it hasn't been noticed already, contenteditable regions also break scrolling with Page Up/Down and Home/End keys.  On the test page, if you increase the height of the body element to 2000px, you can better notice the wonky behavior of these buttons.  For me, Page Down and End can't scroll me past a certain point on the page.  Page Up seems to work fine (though it breaks on other pages where I use contenteditable).  Home will sometimes not scroll all the way up, or not scroll at all.

Comment 11

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

taking this bug. This should be able to be fixed easily after bug 389372.

Comment 12

[(no longer active)]

(In reply to comment #11)
> taking this bug. This should be able to be fixed easily after bug 389372.

Have you had a chance to test this with your patch in bug 389372?  I can confirm that my patch in bug 289384 does not change this behavior (and it would surprise me if it would).

Comment 13

[(no longer active)]

Comment on attachment 416900 [details] [diff] [review]
Fix proposal to prevent unfocused editors from consuming keys

So I guess a proper fix would be to modify the first hunk so that it checks the NODE_IS_EDITABLE flag on the target of the event, and its associated document, in the reverse order.  The element which has the focus is not relevant here.

And I think the rest of the hunks used for disabling the editor are wrong, and not needed.

Comment 14

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

I guess that nsXBLWindowKeyHandler::EnsureHandlers() shouldn't use editor keybindings when it's not in designMode.

And the contenteditable host element should have the handlers *only* when the editor has focus (e.g., some elements in the host element are not editable by contenteditable="false").

Comment 15

[(no longer active)]

(In reply to comment #14)
> I guess that nsXBLWindowKeyHandler::EnsureHandlers() shouldn't use editor
> keybindings when it's not in designMode.

That's starting to make more sense, yes.

> And the contenteditable host element should have the handlers *only* when the
> editor has focus (e.g., some elements in the host element are not editable by
> contenteditable="false").

Well, the only thing which we can really look at is whether the event target node is editable or not, right?  Which is what I suggested in comment 13.

Again, please note that focus here is not an issue, because events could be targeted at elements which do not have the focus.

Comment 16

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

Ah, I see. By the patch of bug 389372, editor is going to ignore the untrusted input events when the editor isn't active element (this is same as WebKit's behavior). Therefore, I thought that [contenteditable="true"]:focus might be useful. However, it's not correct when the DOM window is deactive... So, you're right.

Comment 17

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

But when the editor isn't focused on the DOM window, we should ignore the events because the selection may not be in the contenteditable host. For example:

<body>
<p contenteditable="true">editor1</p>
<p contenteditable="true">editor2</p>
</body>

When "editor1" has focus, the selection in it. At this time, if you dispatch a input event to "editor2" and try to handle them, the reaction should be happened in "editor1".  E.g., caret is moved in the "editor1" by arrow key.

So, I still think that focus is an important factor.

Comment 18

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

Can we ignore the untrusted event issues?
http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLWindowKeyHandler.cpp#328

Comment 19

[(no longer active)]

(In reply to comment #17)
> But when the editor isn't focused on the DOM window, we should ignore the
> events because the selection may not be in the contenteditable host. For
> example:
> 
> <body>
> <p contenteditable="true">editor1</p>
> <p contenteditable="true">editor2</p>
> </body>
> 
> When "editor1" has focus, the selection in it. At this time, if you dispatch a
> input event to "editor2" and try to handle them, the reaction should be
> happened in "editor1".  E.g., caret is moved in the "editor1" by arrow key.
> 
> So, I still think that focus is an important factor.

Both of those contenteditable elements will be managed by the same editor object.  So, why does it matter?

Comment 20

[(no longer active)]

(In reply to comment #18)
> Can we ignore the untrusted event issues?
> http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLWindowKeyHandler.cpp#328

I'm not sure what you're asking.  Do you mean that can we remove that code?  In that case, the answer is no, since that code is responsible for other things besides the editor as well.

Comment 21

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

I meant that if we don't handle untrusted events by the XBL key handlers, we don't need to assume that the key events fired on unfocused elements in their DOM window.

I.e., I think that when nsHTMLEditor gets focus, it should bind the key handlers to the new selection root element. And when the focused element loses focus *in the DOM window level*, it should unbind the handlers.

Comment 22

[(no longer active)]

(In reply to comment #21)
> I meant that if we don't handle untrusted events by the XBL key handlers, we
> don't need to assume that the key events fired on unfocused elements in their
> DOM window.
> 
> I.e., I think that when nsHTMLEditor gets focus, it should bind the key
> handlers to the new selection root element. And when the focused element loses
> focus *in the DOM window level*, it should unbind the handlers.

That causes synthesized events dispatched to those elements to not work, right?  After thinking about this for some time, I don't think I care that much any more!

Comment 23

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

The synthesized input events are dispatched to a last focused element of a last focused DOM window in same top level window. So, it should work.