Last Comment Bug 669026 - Scrolling with arrow keys is broken if any element on the page has contenteditable=true
: Scrolling with arrow keys is broken if any element on the page has contentedi...
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 All
: -- normal with 1 vote (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
: Makoto Kato [:m_kato]
Mentors:
: 481626 505732 674860 681418 731924 (view as bug list)
Depends on: 1248186 1250321 1260840 714164 757368
Blocks: contenteditable 702064
  Show dependency treegraph
 
Reported: 2011-07-02 18:54 PDT by Jesse Ruderman
Modified: 2016-03-30 11:39 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch (v1) (8.42 KB, patch)
2011-07-27 14:43 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Proof of concept (3.06 KB, patch)
2011-07-27 15:55 PDT, neil@parkwaycc.co.uk
enndeakin: review-
Details | Diff | Splinter Review
Fixed proof of concept (4.51 KB, patch)
2011-10-10 09:00 PDT, neil@parkwaycc.co.uk
enndeakin: review+
Details | Diff | Splinter Review
Proposed patch (34.99 KB, patch)
2011-10-25 17:10 PDT, neil@parkwaycc.co.uk
ehsan: review+
Details | Diff | Splinter Review
Fixed patch (49.71 KB, patch)
2011-10-28 07:00 PDT, neil@parkwaycc.co.uk
ehsan: review+
Details | Diff | Splinter Review
With test fixes (for check in) (58.19 KB, patch)
2011-12-23 07:52 PST, neil@parkwaycc.co.uk
ehsan: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-07-02 18:54:31 PDT
Split from bug 549262.

1. Load the testcase, https://bugzilla.mozilla.org/attachment.cgi?id=459420
2. Press the down arrow key several times.

Result: scrolls to the top of the pink div and stops.
Comment 1 (mostly gone) XtC4UaLL [:xtc4uall] 2011-07-03 04:18:53 PDT
Happens on Windows too => OS = All.
Comment 2 :Ehsan Akhgari 2011-07-15 12:24:01 PDT
Possibly even a dupe of bug 549262...
Comment 3 :Ehsan Akhgari 2011-07-15 14:01:38 PDT
(In reply to comment #2)
> Possibly even a dupe of bug 549262...

Oh, forget this comment, I was totally confused.

Boris, is it possible to specify a "fallback" action for the arrow keys?
Comment 4 :Ehsan Akhgari 2011-07-15 14:02:00 PDT
I'm on fire today!  Boris, see comment 3.  :-)
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 16:27:36 PDT
I don't know offhand.... Neil might.
Comment 6 neil@parkwaycc.co.uk 2011-07-15 16:30:17 PDT
(In reply to comment #5)
> I don't know offhand.... Neil might.
I don't understand the question...
Comment 7 :Ehsan Akhgari 2011-07-18 11:25:13 PDT
So, here is the problem.  In platformHTMLBindings.xml, we define a VK_DOWN handler for caret navigation command so that you can go to the next line in editor by pressing down for example.  If there is no editable element focused, we need the default behavior of scrolling the page down to take place, so we need some kind of a fallback event handler being bound to VK_DOWN.  Is there any way to do that?
Comment 8 neil@parkwaycc.co.uk 2011-07-18 14:39:03 PDT
(In reply to comment #7)
> So, here is the problem.  In platformHTMLBindings.xml, we define a VK_DOWN
> handler for caret navigation command so that you can go to the next line in
> editor by pressing down for example.  If there is no editable element
> focused, we need the default behavior of scrolling the page down to take
> place, so we need some kind of a fallback event handler being bound to
> VK_DOWN.  Is there any way to do that?
There is a way to do that, but you have to bind your handlers and attach your controllers to the focused element rather than the window root as you currently do.

The second approach is to fix the editor's command controller to work with editable regions.

The third approach is to stop editor from handling the up and down arrow keys. If you look at the global window controller you'll see that if it thinks there's a caret then it will try to do caret movement instead of scrolling. I think the test detects design mode but not contentEditable regions, so it still tries to do scrolling in that case.
Comment 9 :Ehsan Akhgari 2011-07-27 11:40:41 PDT
*** Bug 505732 has been marked as a duplicate of this bug. ***
Comment 10 neil@parkwaycc.co.uk 2011-07-27 12:58:13 PDT
(In reply to comment #8)
> The third approach is to stop editor from handling the up and down arrow
> keys. If you look at the global window controller you'll see that if it
> thinks there's a caret then it will try to do caret movement instead of
> scrolling. I think the test detects design mode but not contentEditable
> regions, so it still tries to do scrolling in that case.
It does detect contentEditable regions. But some focus code to make link focusing work in caret browsing mode blurs the region :-( See nsSelectMoveScrollCommand::DoCommandBrowseWithCaretOn()
Comment 11 :Ehsan Akhgari 2011-07-27 14:43:01 PDT
Created attachment 548924 [details] [diff] [review]
Patch (v1)
Comment 12 :Ehsan Akhgari 2011-07-27 14:56:59 PDT
*** Bug 481626 has been marked as a duplicate of this bug. ***
Comment 13 neil@parkwaycc.co.uk 2011-07-27 15:55:08 PDT
Created attachment 548949 [details] [diff] [review]
Proof of concept

The editor-base.inc change remaps VK_UP to cmd_scrollLineUp because that was easier to prototype, but ideally it would be renamed back to cmd_linePrevious throughout the codebase. The editor command handler is ruled out of the equation, and that code should be removed. The nsGlobalWindowCommands already knows how to move a caret, so that doesn't change, but the focus manager has to be told not to blur a contenteditable region.

Note that even with caret browsing on, this code doesn't move focus in to a contenteditable region either. I don't know whether this is reasonable.
Comment 14 :Ehsan Akhgari 2011-07-27 16:02:04 PDT
I don't understand why the focus manager change there is OK...
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-07-27 17:31:53 PDT
Why don't you make nsEditor::IsActiveInDOMWindow() an insterface method of nsIEditor?

And, PageUp, PageDown, Home, End keys need to be handled too. (I'm not sure about Shift+Arrow or them)
Comment 16 :Ehsan Akhgari 2011-07-28 10:53:31 PDT
*** Bug 674860 has been marked as a duplicate of this bug. ***
Comment 17 neil@parkwaycc.co.uk 2011-07-28 11:41:58 PDT
Comment on attachment 548949 [details] [diff] [review]
Proof of concept

Well, I might as well ask for review on the nsFocusManager change, just in case... the point here is that we don't normally get here because the editor command handlers are active in contenteditable mode, but I want to be able to remove the editor motion command handlers because the global ones work anyway, except for the issue that they blur contenteditable regions, which is unwanted.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-31 18:20:27 PDT
Ehsan, does Neil's approach sound better to you? It does to me.
Comment 19 Asa Dotzler [:asa] 2011-08-01 15:06:02 PDT
Is this a regression in 6? How widespread is this likely to be? We're just a few days from the final Beta of 6.
Comment 20 Frédéric Buclin 2011-08-01 15:08:21 PDT
(In reply to comment #19)
> Is this a regression in 6? How widespread is this likely to be? We're just a
> few days from the final Beta of 6.

This is not a regression in 6, from what Ehsan said on IRC, but e.g. planet.m.o is unusable using keys for several days now, due to this bug.
Comment 21 :Ehsan Akhgari 2011-08-02 09:15:40 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > Is this a regression in 6? How widespread is this likely to be? We're just a
> > few days from the final Beta of 6.
> 
> This is not a regression in 6, from what Ehsan said on IRC, but e.g.
> planet.m.o is unusable using keys for several days now, due to this bug.

While that is true, this bug has existed for ages (I think from Firefox 3+) and there is no good reason why we should track it for Firefox 6...
Comment 22 :Ehsan Akhgari 2011-08-02 09:26:56 PDT
(In reply to comment #18)
> Ehsan, does Neil's approach sound better to you? It does to me.

As I've said before, I don't understand the focus manager code, so if Neil (Deakin) vouches on that, I'd be fine with Neil's approach.  I'm assuming that he's going to address the todo items in comment 13 once Neil reviews the focus manager change.  Also, Neil, can you check to see if my test case passes successfully with your patch?

(In reply to comment #13)
> Note that even with caret browsing on, this code doesn't move focus in to a
> contenteditable region either. I don't know whether this is reasonable.

That's definitely not desired, but I tested it and we are doing the same thing right now even without this patch, so I don't think we need to worry about this problem here.
Comment 23 Neil Deakin 2011-08-03 06:35:29 PDT
Comment on attachment 548949 [details] [diff] [review]
Proof of concept

The focus manager change looks ok, but the editor-base.inc change is not.

The former fixes an issue where there is a selection within the editable area, but the editable area is not focusable, then tab is pressed. (The removed code from nsFocusManager is only called when there is nothing focused).

The editor-base.inc change though makes scrolling occur in a number of situations where it shouldn't, for example, scrolling should not occur while caret browsing is on, nor while the caret is within the focused editable area.

Is the editor attaching it's command controller to the document and not the editable area?
Comment 24 neil@parkwaycc.co.uk 2011-08-03 06:42:59 PDT
(In reply to comment #23)
> The former fixes an issue where there is a selection within the editable
> area, but the editable area is not focusable, then tab is pressed. (The
> removed code from nsFocusManager is only called when there is nothing
> focused).
I could only get that code to be hit when there was a caret. I'm not sure how to make an unfocusable editable area, but I guess it won't have a caret.

> The editor-base.inc change though makes scrolling occur in a number of
> situations where it shouldn't, for example, scrolling should not occur while
> caret browsing is on, nor while the caret is within the focused editable
> area.
The global window commands already know about caret browsing. (If you look at the browser-base.inc then you'll see the bindings are the same. At some point we may want to rename them back to reduce confusion.) And, except for the code block in the focus manager, its idea of a caret also applies to editable areas, which was why I wanted to remove the code block!

> Is the editor attaching it's command controller to the document and not the
> editable area?
Correct. There was some issue with attaching the commands to the editable area which ehsan has pointed me towards several times but I still forget the bug #.
Comment 25 :Ehsan Akhgari 2011-08-18 08:30:29 PDT
(In reply to neil@parkwaycc.co.uk from comment #24)
> > Is the editor attaching it's command controller to the document and not the
> > editable area?
> Correct. There was some issue with attaching the commands to the editable
> area which ehsan has pointed me towards several times but I still forget the
> bug #.

I tried very hard in bug 289384 to do that, but I failed miserably...  Maybe because I didn't have a good idea what I'm doing. :/
Comment 26 Neil Deakin 2011-08-18 10:00:17 PDT
Comment on attachment 548949 [details] [diff] [review]
Proof of concept

I'm not sure if something else is being asked of me here.

With this patch:
 1. Click in the editable area
 2. Press cursor up/down

Expected:
  The caret moves up/down a line
Actual:
  Page scrolls, so keyboard users cannot edit other lines.
Comment 27 neil@parkwaycc.co.uk 2011-08-18 10:05:09 PDT
(In reply to Neil Deakin from comment #26)
> Expected:
>   The caret moves up/down a line
Well, that's what I get locally. Does caret browsing work in your build?
Comment 28 Neil Deakin 2011-08-18 10:27:15 PDT
I get the same whether caret browsing is enabled or not.
Comment 29 neil@parkwaycc.co.uk 2011-08-18 11:44:02 PDT
(In reply to Neil Deakin from comment #28)
> (In reply to neil@parkwaycc.co.uk from comment #27)
> > (In reply to Neil Deakin from comment #26)
> > > Expected:
> > >   The caret moves up/down a line
> > Well, that's what I get locally. Does caret browsing work in your build?
> I get the same whether caret browsing is enabled or not.
Sorry, I wasn't clear, I meant caret browsing in general, not just in editable areas.
Comment 30 :Ehsan Akhgari 2011-09-12 15:28:30 PDT
Ping?
Comment 31 dindog 2011-10-05 10:25:03 PDT
*** Bug 681418 has been marked as a duplicate of this bug. ***
Comment 32 dindog 2011-10-05 21:23:42 PDT
just in case, HOME/END key is breaks too...
Comment 33 Neil Deakin 2011-10-06 09:02:17 PDT
There seems to be some expectation that I respond here but I have nothing else to add. The patch creates an issue as described in comment 26. The same issue occurs with caret browsing enabled or disabled.
Comment 34 Neil Deakin 2011-10-07 06:45:05 PDT
Caret browsing doesn't work with this patch. The page scrolls when I try to move the caret up and down.
Comment 35 neil@parkwaycc.co.uk 2011-10-10 09:00:43 PDT
Created attachment 565950 [details] [diff] [review]
Fixed proof of concept
Comment 36 Neil Deakin 2011-10-13 14:11:13 PDT
Comment on attachment 565950 [details] [diff] [review]
Fixed proof of concept

Testing and debugging shows that this seems to work.
Comment 37 :Ehsan Akhgari 2011-10-13 14:25:56 PDT
Neil, does your patch make the test part of attachment 548924 [details] [diff] [review] pass?  If so, we can get them landed.  :-)
Comment 38 neil@parkwaycc.co.uk 2011-10-13 14:37:45 PDT
(In reply to Ehsan Akhgari from comment #37)
> If so, we can get them landed.  :-)

Except it's very much proof of concept (although I wouldn't mind landing the nsFocusManager.cpp change which would be necessary anyway).

In particular, I'm not sure a) which keys need to be changed b) whether those keys should map to the same commands in textareas and browsers/editors c) how native Linux key bindings fit in.
Comment 39 neil@parkwaycc.co.uk 2011-10-25 17:10:05 PDT
Created attachment 569554 [details] [diff] [review]
Proposed patch

The names of the commands are configured in three places:
* GTK2 Native keybindings
* Editor bindings (editor-base.inc and libeditor)
* Browser bindings (browser-base.inc and nsGlobalWindowCommands)
The browser bindings have six differences to the other two sets of bindings,
all cmd_scroll* names, which I have renamed for consistency.
The browser bindings were missing implementations for selecting by page.
The stubs also used the wrong command names, so I corrected that too.
This then allowed me to remove the editor implementation.
Comment 40 :Ehsan Akhgari 2011-10-25 19:24:06 PDT
Comment on attachment 569554 [details] [diff] [review]
Proposed patch

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

Looks very good!

Does my test case pass with this patch?
Comment 41 neil@parkwaycc.co.uk 2011-10-26 02:13:44 PDT
(In reply to Ehsan Akhgari from comment #40)
> Does my test case pass with this patch?
Yes, it does.
Comment 42 neil@parkwaycc.co.uk 2011-10-28 04:29:19 PDT
So, things didn't go quite as well as I'd hoped.

Text areas and input fields need to have the editor behaviour.
Design mode and contenteditable windows need to have the global behaviour.
Comment 43 neil@parkwaycc.co.uk 2011-10-28 07:00:00 PDT
Created attachment 570250 [details] [diff] [review]
Fixed patch

So, I ended up having to separate the cursor movement commands into their own command table so that I could append that controller for textareas and inputs.
Comment 44 dindog 2011-11-24 08:23:04 PST
Neil, any news about this bug?
It affects Fx9+(current beta), if we don't fix it in Fx11, then user have to wait at least 3*6=18 weeks before thay can copy text in those page again...
Comment 45 dindog 2011-11-24 08:25:56 PST
Sorry, I mistaken this bug for bug 692153.
Comment 46 neil@parkwaycc.co.uk 2011-11-24 11:42:43 PST
(In reply to dindog from comment #44)
> Neil, any news about this bug?
ehsan, should I be asking someone else for review?

> It affects Fx9+(current beta), if we don't fix it in Fx11, then user have to
> wait at least 3*6=18 weeks before thay can copy text in those page again...

(In reply to dindog from comment #45)
> Sorry, I mistaken this bug for bug 692153.
Actually it turned out to be fairly straightforward to adapt this patch to apply to Copy, but it won't help with the Select All case.
Comment 47 :Ehsan Akhgari 2011-12-21 11:49:39 PST
Comment on attachment 570250 [details] [diff] [review]
Fixed patch

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

I'm not crazy about "editor" vs. "editing" controllers, but I couldn't really think of anything better, so r=me.

Thanks a lot for your work on this, Neil, and sorry that I did a very bad job at responding to the review request in time.
Comment 48 Mozilla RelEng Bot 2011-12-22 01:40:27 PST
Try run for e3859a3ea446 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e3859a3ea446
Results (out of 208 total builds):
    success: 144
    warnings: 50
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-e3859a3ea446
Comment 49 neil@parkwaycc.co.uk 2011-12-22 02:14:10 PST
test_showcaret.xul: uses cmd_scrollBottom, got renamed to cmd_moveBottom
test_selection_move_commands.xul: many renames...
test_movement_by_characters/word.html: rely on the buggy behaviour!
Comment 50 neil@parkwaycc.co.uk 2011-12-22 05:48:00 PST
test_selection_move_commands actually tests for the broken commands. Sigh...
Comment 51 neil@parkwaycc.co.uk 2011-12-22 06:04:20 PST
test_movement_by_characters.html's reliance on this bug is due to it failing to focus the content before performing the navigation.

test_movement_by_words.html has the above bug but did also find a real bug; with this patch, you can't move by words to the very end of a contenteditable area, because it tries to move right out of the contenteditable area...
Comment 52 Mozilla RelEng Bot 2011-12-22 19:10:31 PST
Try run for 8c5b9300caf0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8c5b9300caf0
Results (out of 208 total builds):
    exception: 1
    success: 137
    warnings: 56
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-8c5b9300caf0
Comment 53 neil@parkwaycc.co.uk 2011-12-23 07:52:53 PST
Created attachment 584062 [details] [diff] [review]
With test fixes (for check in)
Comment 54 Mozilla RelEng Bot 2011-12-23 09:10:32 PST
Try run for f0084cbf3b9d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f0084cbf3b9d
Results (out of 206 total builds):
    success: 167
    warnings: 24
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-f0084cbf3b9d
Comment 55 neil@parkwaycc.co.uk 2011-12-23 09:52:37 PST
>     warnings: 24
>     failure: 15
Most of these are hidden; 3 randomorange show up on tbpl by default.
More oranges and the reds show up with noignore or via self-serve but they're all Lion, Tegra and/or Jetpack so nobody cares about them.
Comment 56 dindog 2011-12-24 07:13:39 PST
This bug is fixed in 20121224 nightly. Neil, would you please look into the copy menuitem too?

Ehsan, sorry again for the other day urge you to review this bug in IRC. I got a reason to worried. Two major sites in China use contenteditable element in part of there service, (In fact, they rank No.1 & No.2 in China and No.5 & No.10 all over the world in Alexa), both are popular things, try imagining as they are Chinese Facebook and Google group, you'll got the picture.

I heard complaint about unable to copy and scroll after Fx9.0 was released. I don't want Firefox losing users. I'll would be better have it fixed not only in nightly, but also beta, even release.
Comment 57 dindog 2011-12-24 07:15:23 PST
*It* 'll would be better have it fixed not only in nightly, but also beta, even release. 

sorry for the misspell
Comment 58 neil@parkwaycc.co.uk 2011-12-24 07:43:15 PST
Pushed changeset 4d0391866459 to mozilla-central.

(In reply to dindog from comment #56)
> Neil, would you please look into the copy menuitem too?
Yes, I'll do copy (but not select all, because it's harder) in bug 692153.
Comment 59 Dietrich Ayala (:dietrich) 2011-12-30 10:01:15 PST
"Dude, where's my commands?!" This change removed commands that have been around for many years, breaking add-ons: bug 714164.
Comment 60 :Ehsan Akhgari 2011-12-30 10:45:00 PST
Neil, I think you need to add those commands back, and just make us not use them... :(
Comment 61 neil@parkwaycc.co.uk 2011-12-30 12:55:08 PST
Fair enough, and I can put the test back too, but I might as well use bug 714164.
Comment 62 :Ehsan Akhgari 2012-01-03 13:39:54 PST
*** Bug 702064 has been marked as a duplicate of this bug. ***
Comment 63 Alex Keybl [:akeybl] 2012-01-06 09:19:19 PST
Bug 696020 appears to have exacerbated this problem in 10 (see bug 702064). The attached patch appears to be higher risk than we're comfortable for Beta (please correct me if I'm wrong), but I'm wondering if this is a good candidate for Aurora 11.
Comment 64 :Ehsan Akhgari 2012-01-10 12:49:07 PST
(In reply to Alex Keybl [:akeybl] from comment #63)
> Bug 696020 appears to have exacerbated this problem in 10 (see bug 702064).
> The attached patch appears to be higher risk than we're comfortable for Beta
> (please correct me if I'm wrong), but I'm wondering if this is a good
> candidate for Aurora 11.

I'd be much more comfortable if we could back out bug 696020 from Aurora.  smaug, can we do that?
Comment 65 Olli Pettay [:smaug] 2012-01-11 01:35:40 PST
No. We were thinking to release 9.0.2 with bug 696020 fixed.
Comment 66 Olli Pettay [:smaug] 2012-01-11 01:36:46 PST
Or if we do that, we need to backout all
bug 696020, bug 689564 and bug 659350.
Comment 67 Alice0775 White 2012-02-29 23:21:59 PST
*** Bug 731924 has been marked as a duplicate of this bug. ***
Comment 68 Jeffrey 'jf' Lim 2012-03-01 00:21:53 PST
If I could request a summary of the status, where are we on this bug now? dindog@163's saying that it's fixed in 20121224 nightly? How about in release? When is this going to be seen in a release, and when?
Comment 69 neil@parkwaycc.co.uk 2012-03-01 01:26:06 PST
If I've done my sums right, you should find that it's currently available in Firefox Aurora, will become available in Beta on the 13th, and then released on the 24th of April. (If you think 17 weeks is a long time to wait, don't forget that Firefox 4 came out 14 months after Firefox 3.6!)
Comment 70 garryb 2012-04-25 00:38:45 PDT
Woot, I see this now in Firefox 12. (This is the main reason we're still using a designMode iframe in G+)
Comment 71 :Ehsan Akhgari 2012-04-25 20:01:11 PDT
(In reply to garryb from comment #70)
> Woot, I see this now in Firefox 12. (This is the main reason we're still
> using a designMode iframe in G+)

Great!  Please file new bugs if you see more problems blocking you from moving to contenteditable elements.
Comment 72 Jakub Ondrusek 2012-07-23 02:40:25 PDT
As of 13.0.1 this still hasn't been fixed. Please see attached example:
http://pastebin.com/WTiitrQy
Comment 73 (mostly gone) XtC4UaLL [:xtc4uall] 2012-07-23 09:03:10 PDT
(In reply to Jakub Ondrusek from comment #72)

Please file a new Report in the Core/Editor Component with your Testcase attached. Thanks!

Note You need to log in before you can comment on or make changes to this bug.