[webkit-reviews] review denied: [Bug 71854] Access key should work on all elements : [Attachment 115012] patch

Thu Nov 17 00:23:54 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 71854: Access key should work on all elements
https://bugs.webkit.org/show_bug.cgi?id=71854

Attachment 115012: patch
https://bugs.webkit.org/attachment.cgi?id=115012&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115012&action=review

r- due to nits.

> Source/WebCore/ChangeLog:9
> +	   says All HTML elements can have the accesskey content attribute set.
Adding "accessKey"

Nit: says All?

> Source/WebCore/ChangeLog:10
> +	   attribute idl file as [Reflect].

Please make 'Adding "accessKey" attribute idl file as [Reflect]' a full
sentence.

> Source/WebCore/ChangeLog:14
> +	   * bindings/objc/PublicDOMInterfaces.h: Moving properties form
subclass to base class.

s/Moving/Moved/

> LayoutTests/fast/forms/access-key-for-all-elements-expected.txt:1
> +This test checks to see if accesskey attributes works all elements.

Nit: works *on* all elements

> LayoutTests/fast/forms/access-key-for-all-elements.html:1
> +<html>

No DOCTYPE?

> LayoutTests/fast/forms/access-key-for-all-elements.html:10
> +		       
"body","br","button","canvas","caption","center","cite","code","col","colgroup"
,"command","datalist","dd","del","details","dfn","dir","div","dl","dt",

We don't normally indent like this. Just intend by 4 spaces please.

> LayoutTests/fast/forms/access-key-for-all-elements.html:19
> +    var parent = document.createElement(tagNames[i]);
> +    document.body.appendChild(parent);

This isn't a parent, right? This IS the test element.

> LayoutTests/fast/forms/access-key-for-all-elements.html:20
> +    var testElement = document.querySelector(tagNames[i]);

Why do we need to query-select it?

> LayoutTests/fast/forms/access-key-for-all-elements.html:25
> +    accesskey = testElement.accessKey; 
> +    shouldBe('accesskey',"'k'");

Why do we ned to copy it to accesskey here? You could have just written
shouldBe('testElement.accessKey',"'k'");