Confirmed that it doesn't work in Chrome 4.0.212.1

this regressed probably due to the cocoa keybinding changes, is my guess.

Thakis, who landed those?

Can we get a bisection on when this broke? should be easy.

This showed up first in the build at r26700

Yup. Thakis' keybinding's change seems to be it: 
http://build.chromium.org/buildbot/snapshots/chromium-rel-
mac/26700/changelog.xml

 Issue 22644  has been merged into this issue.

This is not the first time this has broken (I think Hironori already fixed this once by 
explicitly removing the selectors that Cocoa fires for this event from the list of selectors 
we catch on the browser side).

If it isn't too much trouble I think it would be great if the fix for this could include a 
browser test to verify the behavior of cmd-up/down so we don't keep breaking it :|

For my own notes: hbono's fix was http://codereview.chromium.org/164309

Thank you for your bug report.
I assume everybody is getting understand why I repeatedly told James to write unit tests even though I received negative feedbacks. :)

By the way, the reason that caused this problem is simple: WebFrameImpl::executeCommand() executes an edit command even when the focused node 
is NOT editable (*1). (Does it make any sense to execute an edit command when the focused node is <select>?) When a renderer executes an edit 
command, RenderView::handleCUrrentKeyEvent() returns true and prevents from dispatching a keyboard event to the focused node (*2) and its 
default action, i.e. WebViewImpl::ScrollViewWithKeyboard(). Thus, my previous fix has not been working since r26694.

In my personal opinion, this function should return false without executing an edit command if the focused node is not editable. Also, we 
should change RenderView::handleCurrentKeyEvent() to return false when WebFrameImpl::executeCommand() returns false so that a keyboard event is 
dispatched to the focused node. Is it possible to give me your feedbacks?

As for writing a unit test for this issue, the easiest step is adding a RenderViewTest that calls RenderView::OnSetEditCommandsForNextEvent(), 
sends a cmd-key event, and checks if it is sent to a JavaScript function. (This test is very similar with RenderViewTest.OnHandleKeyboardEvent, 
which is only available on Windows now, i.e. we need to port this test to Mac and Linux.)

(*1) Many functions in the WebCore::Editor class just ignores an edit command and returns true if the focused node is not editable.
(*2) As an active member of the DOM Level 3 Event spec, I'm wondering if the current implementation of Chrome (a user-agent may prevent from 
dispatching a keyboard event to the focused DOM node) is compliant with the spec.

Regards,

hbono, you should never ever get negative feedback for asking for a unit test.
If you do let me know and I'll make jeremy hurt them.

(No comment was entered for this change.)

Hironori, thank you very much for your detailed comment. A CL that does what you 
propose is at http://codereview.chromium.org/254002 and it seems to fix this issue. I 
agree that it's very important to have tests in this area, so I'll try to add a test or two to 
this CL before asking for review.

I'm nearly done writing a test, but the suggested fix doesn't quite work: Disabling edit 
commands for non-editable nodes breaks "selectAll".

thakis,

Sorry for my slow updates. (I have been taking a vacation this week.)
You are correct. I also noticed it last Friday while prototyping my idea. Even though I think copying the 
logic in EditorClientImpl::handleEditingKeyboardEvent() (*1) can solve this problem, I'm wondering if it is a 
good solution because a copy-and-paste solution makes the Chromium code more complicated and harder to debug.

(*1) WebFrameImpl::executeCommand() executes an editor command but returns false if the focused node is not 
ediable.

To share such logic (or magics), I personally preferred adding a method WebViewClient::interpretKeyEvent(), 
which returns an editor command from a key event, so that it can return the queued edit command instead of 
looking up a hash table when a browser sent it. (Maybe we need to talk with darin?)

Regards,

The patch at http://codereview.chromium.org/254002 is working by now, it simply 
adds some special-case logic to WebFrameImpl::executeCommand(). If the try servers 
like the patch too, I will send it out for review.

On Windows and Linux, SelectAll logic on non-editable node is handled in 
WebViewImpl::KeyEventDefault(). Not sure if Mac can share the same code path.

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=27464 

------------------------------------------------------------------------
r27464 | thakis@chromium.org | 2009-09-28 20:56:51 -0700 (Mon, 28 Sep 2009) | 14 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome.gyp?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/render_view.cc?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/render_view.h?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/render_view_unittest.cc?r1=27464&r2=27463
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/render_view_unittest_mac.mm
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/render_view_test.cc?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/render_view_test.h?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/webframe_impl.cc?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/webview_impl.cc?r1=27464&r2=27463
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/webview_impl.h?r1=27464&r2=27463

Fix cmd-up/cmd-down.

The approach is to special-case moveToBeginningOfDocument and moveToEndOfDocument in WebFrameImpl::executeCommand(). With this (and the cocoa keyboard bindings patch being landed), the special-case code in WebViewImpl::ScrollViewWithKeyboard() can be removed.

Also add a test for cmd-up/down.

Change chrome.gyp so that it supports osx-only unit tests (_unittest_mac.mm).

Move OnPrintPageAsBitmap to the other printing tests.

BUG=22585
TEST=Go to a page with both text box and scrollbar (e.g. http://en.wikipedia.org ). Pressing cmd-down should scroll to end of page, cmd-up back to start. Clicking the text box and trying some emacs shortcuts should work (ctrl-a jumps to start of line, cmd-h acts as backspace, etc).

Review URL: http://codereview.chromium.org/254002
------------------------------------------------------------------------

(No comment was entered for this change.)

Platform:
  Hostname: Macintosh-0017f2d64524.local
  Mac OS X Version 10.5.8 (Build 9L27)
  Processor: 2 Intel 2.33 GHz
  RAM: 2048 MB

Chrome:
  Chrome version: 4.0.221.0  <<<Release>>>
  QuickTime Player: 7.6.4
  QuickTime PlayerX: <unknown>
  Flash Player: 10.0.32

(No comment was entered for this change.)