[Webkit-unassigned] [Bug 71854] Access key should work on all elements

Thu Nov 17 00:23:55 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=71854

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #115012|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #33 from Ryosuke Niwa <rniwa at webkit.org>  2011-11-17 00:23:54 PST ---
(From update of attachment 115012)
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'");

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.