Bug 37134 - Remove unused DIRECTIONAL_PAD_NAVIGATION
Summary: Remove unused DIRECTIONAL_PAD_NAVIGATION
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-05 20:16 PDT by Antonio Gomes
Modified: 2010-04-06 07:33 PDT (History)
4 users (show)

See Also:


Attachments
(committed in r57148) patch (2.57 KB, patch)
2010-04-05 20:23 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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