Bug 37134

Summary: Remove unused DIRECTIONAL_PAD_NAVIGATION
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: AccessibilityAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: caryclark, eric, joseph.ligman, laszlo.gombos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
(committed in r57148) patch none

Description Antonio Gomes 2010-04-05 20:16:32 PDT 
bug 23145 was filed intending to add a DIRECTIONAL_PAD_NAVIGATION build flag to WebCore (see attached patch in that bug), but according to https://bugs.webkit.org/show_bug.cgi?id=23145#c3 this is no longer needed once android does not implement Î…directional navigation" in engine side, nor seems to plan so. Bug was then RESOLVED as INVALID.

In the meanwhile, bug 23163 landed a patch that relies on the DIRECTIONAL_PAD_NAVIGATION flag, and it is not dead code.

I propose to remove any refenrece to DIRECTIONAL_PAD_NAVIGATION from trunk

ps: we have "Spatial Navigation" available in trunk (see bug 18862 and dependency list).
Comment 1 Antonio Gomes 2010-04-05 20:23:17 PDT 
Created attachment 52601 [details]
(committed in r57148) patch
Comment 2 Antonio Gomes 2010-04-05 20:26:22 PDT 
s/it is not dead code/it is NOW dead code/
Comment 3 Laszlo Gombos 2010-04-05 20:39:37 PDT 
Bug 18862 does not seems to be related to Spatial Navigation.

ChangeLog has some minor typos, spacing issues, please fix it when landing.

This patch looks ok to me, but I'd like to give some time to Cary to give feedback before giving an r+. I do not think we have enough evidence that this flag is not used by any ports. The fact that this is not listed in Platform.h is not enough evidence.
Comment 4 Antonio Gomes 2010-04-05 20:43:05 PDT 
(In reply to comment #3)
> Bug 18862 does not seems to be related to Spatial Navigation.

err, it is bug 18662. Sorry.

> ChangeLog has some minor typos, spacing issues, please fix it when landing.

sure, thank you.

> This patch looks ok to me, but I'd like to give some time to Cary to give
> feedback before giving an r+. I do not think we have enough evidence that this
> flag is not used by any ports. The fact that this is not listed in Platform.h
> is not enough evidence.

cary, eric, what do you guys think?
Comment 5 Cary Clark 2010-04-06 05:49:24 PDT 
The DIRECTIONAL_PAD_NAVIGATION condition is obsolete. D-pad navigation on Android used to change the DOM focus; now, it only changes the mouse position. Since the focus is only set when the page initiates Javascript, or through mouse click events, this scroll in response to changing focus no longer needs to be suppressed.

Android separately ignores scroll requests that are not initiated by user actions, so it's fine to remove this.
Comment 6 Antonio Gomes 2010-04-06 05:54:40 PDT 
(In reply to comment #5)
> The DIRECTIONAL_PAD_NAVIGATION condition is obsolete. D-pad navigation on
> Android used to change the DOM focus; now, it only changes the mouse position.
> Since the focus is only set when the page initiates Javascript, or through
> mouse click events, this scroll in response to changing focus no longer needs
> to be suppressed.
> 
> Android separately ignores scroll requests that are not initiated by user
> actions, so it's fine to remove this.

thank you Cary. Laszlo, what typos you referred to?
Comment 7 Laszlo Gombos 2010-04-06 06:55:59 PDT 
Comment on attachment 52601 [details]
(committed in r57148) patch

LGTM based on Cary's comments. The typo seems to be only in the bug report and not in ChangeLog.
Comment 8 Antonio Gomes 2010-04-06 07:33:44 PDT 
Comment on attachment 52601 [details]
(committed in r57148) patch

thanks laszlo