[webkit-reviews] review denied: [Bug 22935] navigate elements with directional pad : [Attachment 26143] add directional navigation to elements

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Cary Clark <caryclark at google.com>'s
request for review:
Bug 22935: navigate elements with directional pad
https://bugs.webkit.org/show_bug.cgi?id=22935

Attachment 26143: add directional navigation to elements
https://bugs.webkit.org/attachment.cgi?id=26143&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +#if !defined(ENABLE_DPAD_NAVIGATION)
> +#define ENABLE_DPAD_NAVIGATION 0
> +#endif

I don't think DPAD is an abbreviation for "directional pad" that everyone
looking at WebKit would recognize, so please use
ENABLE_DIRECTIONAL_PAD_NAVIGATION.

> @@ -196,6 +196,7 @@ bool HTMLInputElement::isKeyboardFocusab
>	   if (name().isEmpty())
>	       return false;
>  
> +if !ENABLE(DPAD_NAVIGATION)
>	   // Never allow keyboard tabbing to leave you in the same radio
group.	Always
>	   // skip any other elements in the group.
>	   Node* currentFocusedNode = document()->focusedNode();
> @@ -205,6 +206,7 @@ bool HTMLInputElement::isKeyboardFocusab
>		   focusedInput->name() == name())
>		   return false;
>	   }
> +#endif
>	   
>	   // Allow keyboard focus if we're checked or if nothing in the group
is checked.
>	   return checked() ||
!checkedRadioButtons(this).checkedButtonForGroup(name());

I don't understand the connection between a "directional pad" and this change,
and the change log comments don't make it clear either. Is the tab key
considered to be input from the directional pad? If so, is that the best
design? Maybe this should not be a compile time setting, but rather a different
event, related to the tab key but not identical. The reason isKeyboardFocusable
gets a KeyboardEvent argument is so that different types of keyboard navigation
can support different rules about what's skipped.

I don't think this change is designed correctly.

> +#if ENABLE(DPAD_NAVIGATION)
> +		       if (!checked()) // allow enter to set state of radio
> +			   clickElement = true;
> +		       break;
> +#else
>		       break; // Don't do anything for enter on a radio button.

> +#endif

Same comment here. I think the directional pad Enter event should contain
something in it that makes it different at runtime. Just compiling in different
behavior for all Enter events doesn't seem like the right design.

Looking at the other two changes, I have exactly the same thought.

This patch seems to be a set of behavior changes at compile time that should
instead be tied to some difference in the directional pad events.

Or maybe you could convince me otherwise.

review- for now