[Webkit-unassigned] [Bug 16135] [GTK] Support caret browsing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=16135





------- Comment #17 from xan.lopez at gmail.com  2009-04-22 00:00 PDT -------
(In reply to comment #16)
> (From update of attachment 29647 [review])
> > +    if (caretBrowsing) {
> > +        switch (keyEvent->windowsVirtualKeyCode()) {
> >              case VK_LEFT:
> > -                frame->selection()->modify(kevent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> > +                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> >                          SelectionController::LEFT,
> > -                        kevent->ctrlKey() ? WordGranularity : CharacterGranularity,
> > +                        keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
> >                          true);
> > -                break;
> > +                return true;
> 
> There seems to be code very similar to this one here, btw:
> EventHandler::handleKeyboardSelectionMovement (in
> WebCore/page/EventHandler.cpp). This code seems to be called Perhaps we are
> doing some unneeded work?

Mmm, it seems to be conditionally called if some a11y setting is enabled, but I
think it's not in our case. I guess you are talking about
EventHandler::defaultKeyboardEventHandler here? I can look into this.

> 
> > +    const PlatformKeyboardEvent* keyEvent = evt->keyEvent();
> > +    if (!keyEvent /*|| keyEvent->isSystemKey()*/)  // do not treat this as text input if it's a system key event
> > +        return false;
> 
> keyEvent->isSystemKey seems to be something used only by windows? We probably
> want to remove both comments here.

You are right.

> 
> > -    // FIXME: Use GtkBindingSet instead of this hard-coded switch
> > -    // http://bugs.webkit.org/show_bug.cgi?id=15911
> 
> We are now using GtkBindingSet in the webview, but we will be using the static
> table for editing? I am still trying to grasp how key handling works, but it
> seems to me like most things such as home/end/pageup/pagedown should be handled
> by that code you added to WebView, instead?
> 

The motion keys in editor mode handle and modify the selection, while the stuff
in the view scroll the view, so AFAICT they should not be handled by the same
code. What could be done here, probably, is change the code to use GtkBinding
too.

> > +    // It's not quite clear whether clipboard shortcuts and Undo/Redo should be handled
> > +    // in the application or in WebKit. We chose WebKit.
> > +    { 'C',       CtrlKey,            "Copy"                                        },
> > +    { 'V',       CtrlKey,            "Paste"                                       },
> > +    { 'X',       CtrlKey,            "Cut"                                         },
> > +    { 'A',       CtrlKey,            "SelectAll"                                   },
> > +    { VK_INSERT, CtrlKey,            "Copy"                                        },
> > +    { VK_DELETE, ShiftKey,           "Cut"                                         },
> > +    { VK_INSERT, ShiftKey,           "Paste"                                       },
> > +    { 'Z',       CtrlKey,            "Undo"                                        },
> > +    { 'Z',       CtrlKey | ShiftKey, "Redo"                                        },
> > +};
> [...]
> > +    Editor::Command command = frame->editor()->command(interpretKeyEvent(evt));
> [...]
> > +void EditorClient::handleKeyboardEvent(KeyboardEvent* event)
> > +{
> > +    if (handleEditingKeyboardEvent(event))
> > +        event->setDefaultHandled();
> >  }
> 
> An old commit seems to have moved cut/copy/paste/select-all from here to our
> WebView object (see http://trac.webkit.org/changeset/28386), so I think we
> should probably avoid handling them here? We should probably consider moving
> undo/redo, in the future, too.
> 

You are right, in this case it seems to be the same code. I'll check this too.

Thanks for the review!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.