[Webkit-unassigned] [Bug 17052] Scrolling with arrow keys does not update cursor

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32408|review?                     |review-
               Flag|                            |




--- Comment #21 from Darin Adler <darin at apple.com>  2009-07-16 08:10:10 PDT ---
(From update of attachment 32408)
> +    const bool result = handleWheelEventInternal(e);

There are many local variables that could be marked const. Normally it is not
our style in the WebKit project to do this just because the variable is not
modified in the function. It seems an unnecessary stylistic difference in your
new code.

> +    // For Windows and GTK, we need to simulate a mousemove event after scrolling by mouse wheel or up/down/space key.
> +    // For MAC, we don't need to, because the OS sends a mousemove event after a mousewheel event, and hides the mouse cursor while scrolling by keys.
> +    // TODO: Examine other platforms.

The name of the platforms are Windows and Mac. Not Windows and MAC.

This comment seems incorrect to me. An almost identical issue exists on the
Mac. I think the issue is that the Mac has code in WebKit to handle this, and
the other platforms don't. I don't think the difference is in the OS. You
should patch the platforms to be the same instead of creating an unnecessary
platform difference.

Even if what you said was true about the wheel events, it's definitely not true
about keyboard events.

In fact, by leaving this code turned off for Mac you miss the opportunity to
fix a bug I've seen where scrolling with arrow keys on Mac doesn't result in
update of hover. Worse, by including PLATFORM(CHROMIUM), you'd fix the Mac
version of Chrome but leave this broken in the Mac version of Safari.

> +    // Simulating a mousemove event has a side effect that onmousemove handler is invoked when the mouse doesn't actually move.

Tiny language nitpick. Referring to the "onmousemove" handler is unnecessarily
inaccurate. Setting an onmousemove attribute is only one of many ways to set up
a handler for a mousemove event, and not one that needs to be emphasized in
comments.

> +    bool keyEventInternal(const PlatformKeyboardEvent&);
> +    bool handleWheelEventInternal(PlatformWheelEvent&);

Why does one of these functions have "handle" in its name, but not the other? I
guess because they reflect the names of the original functions that you broke
into two pieces.

It's a little ugly to just add a level of function call so you can wrap the
function in new code. If people find other similar requirements and go on
repeating this pattern, then we will end up with Internal2, Internal3, versions
of these functions. The names do nothing to tell programmers what should be in
the outer and what should be in the inner version.

I don't think it's a good design pattern for the long term, although I think
it's OK here.

It's strange that we will record scroll positions and simulate mouse move
events even for keyboard events we choose not to handle. I think this patch
really isn't tackling this at the right level. It's true that this is the
easiest level to patch for someone who's not familiar with the code, but
scrolling can also be triggered by scripts rather than by events, and the
scripts could run based on a timer. It's not right to trigger the simulated
events based on the real events. Instead I think the simulated mouse move
events should be triggered more directly by the actual document scrolling.

A good test case would be a timer that scrolls. The cursor should be updated
each time the scroll happens even if there is not keyboard or mouse wheel
events.

> +    void updateMousePositions(const PlatformMouseEvent& mouseEvent);

We do not give names to arguments like mouseEvent in function declarations when
the type already makes the argument's purpose clear.

> +        const IntPoint& globalPos() const { return m_globalPosition; }

I know this is just following the standard of the original code, but I think
globalPosition() would be a better name for this new function.

I think it's unnecessary to have the simulated event have so much incorrect
state, such as false for all the modifier keys. Instead we should make the
simulated event reflect the real state as much as possible.

I also think that the simulated event should not contain the same mouse
position as the last real event. The mapping from a global position to a local
one can be affected by things such as the window position, and that could have
changed since the last real mouse event, for example if a script moved the
window.

The idea of this code is great, but I think it needs more work and to be less
platform-specific.

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