Bug 23163 - suppress scrolling in Element::updateFocusAppearance
Summary: suppress scrolling in Element::updateFocusAppearance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Cary Clark
URL:
Keywords:
Depends on: 23145
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-07 07:33 PST by Cary Clark
Modified: 2010-01-29 13:41 PST (History)
3 users (show)

See Also:


Attachments
don't scroll when setting focus for devices with directional pad navigation (1.22 KB, patch)
2009-01-07 07:39 PST, Cary Clark
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cary Clark 2009-01-07 07:33:26 PST
A device with a directional pad may set the focus while navigating, to allow javascript to modify the DOM as appropriate. The same directional pad may navigate by scrolling -- and there's no way to know if the user intended to move the focus or scroll. 

On the desktop, changing the focus also attempts to scroll the focused element into view. This is undesirable for devices that use a directional pad for navigation.

(patch depends on 23145 for the definition of ENABLE_DIRECTIONAL_PAD_NAVIGATION)
Comment 1 Cary Clark 2009-01-07 07:39:25 PST
Created attachment 26491 [details]
don't scroll when setting focus for devices with directional pad navigation
Comment 2 Darin Adler 2009-01-10 15:49:24 PST
Comment on attachment 26491 [details]
don't scroll when setting focus for devices with directional pad navigation

It seems to me that the real issue here isn't exactly "directional pad navigation" -- it's more "small screen with explicit scrolling", and I suppose the explicit scrolling part is what's really relevant. I could easily imagine someone using WebKit on a web browser for a TV screen, and using a directional pad on a remote control. It's entirely unclear to me whether the presence of the directional pad would also cause the developers to want this behavior in that case.

I'm going to say r=me with this, but I think we're being a bit unclear on what DIRECTIONAL_PAD_NAVIGATION means and that could become a problem down the line.
Comment 3 Eric Seidel (no email) 2009-02-04 16:36:16 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/dom/Element.cpp
Committed r40647
Comment 4 Eric Seidel (no email) 2009-02-04 17:28:07 PST
Patch was wrong, fixed:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/dom/Element.cpp
Committed r40654
Comment 5 Antonio Gomes 2010-01-28 07:39:45 PST
it is interesing that this patch landed, and its BLOCKER did not. It introduced:

=======================================
#if !ENABLE(DIRECTIONAL_PAD_NAVIGATION)
    else if (renderer() && !renderer()->isWidget())
        renderer()->enclosingLayer()->scrollRectToVisible(getRect());
#endif
=======================================

but DIRECTIONAL_PAD_NAVIGATION is not even in Platform.h . Grep'ing for it outputs only:

$ grep -nHR DIRECTIONAL_PAD_NAVIGATION .
./WebCore/dom/Element.cpp:1293:#if !ENABLE(DIRECTIONAL_PAD_NAVIGATION)

cc'ing darin who reviwed and eric who landed it.
Comment 6 Darin Adler 2010-01-29 13:41:39 PST
This is something that was put in for Android. Beyond that I have little insight into what it is or if it should be removed or kept.