Access key should work on all elements

[Erik Arvidsson]

Access key should work on all elements

[Ojan Vafai]

Makes sense to me, but we should investigate what other browsers do first. Also, do you have a link to the spec that claims this should work?

[Erik Arvidsson]

(In reply to comment #1)
> Makes sense to me, but we should investigate what other browsers do first. Also, do you have a link to the spec that claims this should work?

See URL of the bug

[Vineet Chaudhary (vineetc)]

Created attachment 114223 [details]
Test Case 1

I tested this behavior on IE(7), Firefox(7.0.1) and Opera with attached test case.

It is observed that except webkit(gtk port) all returns null(empty srting) value where as webkit gtk alerts undefined.

Adding below code to HTMLElement.idl will enable to work accessKey to other elements too.

attribute [Reflect] DOMString accessKey;

[Vineet Chaudhary (vineetc)]

Created attachment 114225 [details]
Test Case 2

Also it is observed that on IE(7) apart from input and anchor element other element (here in case) div element also have accessKey property.
On IE div element gets focus on pressing Alt+d. 

May be this could be handled in HTMLElement::accessKeyAction(bool sendToAnyElement).

Please let me know your thoughts on this?

[Vineet Chaudhary (vineetc)]

Created attachment 114247 [details]
Proposed Patch

Attaching proposed patch.

Please let me know your comments on this.

[Vineet Chaudhary (vineetc)]

Comment on attachment 114247 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114247&action=review

> LayoutTests/fast/forms/access-key-for-all-elements.html:8
> +<div id="test"></div>

Currently it is checking only for DIV element which doesn't works with "accessKey" by default. With this patch it is expected to work now for DIV as well as all other elements.
I tried to have test to check for all element but couldn't find any. Please let me know if anyone have better thoughts to check for all "Elements", thanks.

[Adam Barth]

Comment on attachment 114247 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114247&action=review

> Source/WebCore/ChangeLog:10
> +        As per specification http://dev.w3.org/html5/spec/Overview.html#elements-in-the-dom
> +        says All HTML elements can have the accesskey content attribute set. Adding "accessKey"
> +        attribute idl file as [Reflect].

Do we need to remove accessKey from any other IDL files (i.e., because they're now redundant)?

[Vineet Chaudhary (vineetc)]

(In reply to comment #7)
> (From update of attachment 114247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114247&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        As per specification http://dev.w3.org/html5/spec/Overview.html#elements-in-the-dom
> > +        says All HTML elements can have the accesskey content attribute set. Adding "accessKey"
> > +        attribute idl file as [Reflect].
> 
> Do we need to remove accessKey from any other IDL files (i.e., because they're now redundant)?

I think we should keep them because "accessKey" attribute implementation is different for different elements.
Eg. For Input element(type=text) it sets the Focus while for anchor element it simulates click events.
Ref :  http://trac.webkit.org/browser/trunk/Source/WebCore/html/InputType.cpp#L445 and http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLAnchorElement.cpp#L236

Or else should we move these implementations to HTMLElement::accessKeyAction() with if checks for each different element as you suggests.

[Adam Barth]

I'm not sure I understand.  This IDL attribute just reflects the DOM attribute.  Many DOM attribute mean different things on different elements.  Usually there isn't much of a reason to specify them redundantly in the IDL.

[Ryosuke Niwa]

What Adam is talking about IDL attributes such as:
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.idl#L34
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLAnchorElement.idl#L24

[Ryosuke Niwa]

Comment on attachment 114247 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114247&action=review

r- because there are redundant IDL attribute entries.

>> LayoutTests/fast/forms/access-key-for-all-elements.html:8
>> +<div id="test"></div>
> 
> Currently it is checking only for DIV element which doesn't works with "accessKey" by default. With this patch it is expected to work now for DIV as well as all other elements.
> I tried to have test to check for all element but couldn't find any. Please let me know if anyone have better thoughts to check for all "Elements", thanks.

We need to test more than just div. I suggest you copy the list of elements out of HTMLNames and test all elements in some loop.

[Vineet Chaudhary (vineetc)]

Created attachment 114441 [details]
Updated Patch

Thanks Adam/Ryosuke for inputs.

(In reply to comment #11)
> r- because there are redundant IDL attribute entries.

Removed redundant IDL attribute entries.

> >> LayoutTests/fast/forms/access-key-for-all-elements.html:8
> We need to test more than just div. I suggest you copy the list of elements out of HTMLNames and test all elements in some loop.

Added more test coverage to all elements.

[Adam Barth]

Comment on attachment 114441 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114441&action=review

> LayoutTests/fast/forms/access-key-for-all-elements.html:32
> +for (var i =0 ; i < test.length; i++) {
> +var parent = document.createElement(test[i]);
> +document.body.appendChild(parent);
> +var test_me = document.getElementsByTagName(test[i])[0];
> +debug('Check for ' + test_me.tagName + ' tag');
> +shouldBeDefined('test_me.accessKey');
> +test_me.accessKey ='k';
> +accesskey = test_me.accessKey; 
> +shouldBe('accesskey',"'k'");
> +debug('');
> +}

Please intent this code properly.

[WebKit Review Bot]

Comment on attachment 114441 [details]
Updated Patch

Attachment 114441 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10399267

New failing tests:
fast/forms/access-key-for-all-elements.html

[Vineet Chaudhary (vineetc)]

Created attachment 114462 [details]
Patch

Updated patch with proper indentation.

[WebKit Review Bot]

Comment on attachment 114462 [details]
Patch

Rejecting attachment 114462 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/trunk@9897; UUID don't match and there is local changes in /mnt/git/webkit-commit-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 111.

Full output: http://queues.webkit.org/results/10403364

[Vineet Chaudhary (vineetc)]

Comment on attachment 114462 [details]
Patch

I am not sure why its failing to checkout the v8, but I see many patches in commit queue are failing right now with the same reason :(. Setting cq flag again.

[WebKit Review Bot]

Comment on attachment 114462 [details]
Patch

Clearing flags on attachment: 114462

Committed r100013: <http://trac.webkit.org/changeset/100013>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

I'm assuming after this I'm seeing build errors from the bindings generator on Mac builds:

    Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 323.
    Public API change. There are missing public properties and/or methods from the "DOMHTMLAnchorElement" class.
    @property(copy) NSString *accessKey;

CodeGeneratorObjC.pm checks the new APIs against
Source/WebCore/bindings/objc/PublicDOMInterfaces.h. Should this file
be updated now that accessKey is available on DOMHTMLElement?

[Joseph Pecoraro]

(In reply to comment #20)
> I'm assuming after this I'm seeing build errors from the bindings generator on Mac builds:
> 
>     Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 323.
>     Public API change. There are missing public properties and/or methods from the "DOMHTMLAnchorElement" class.
>     @property(copy) NSString *accessKey;
> 
> CodeGeneratorObjC.pm checks the new APIs against
> Source/WebCore/bindings/objc/PublicDOMInterfaces.h. Should this file
> be updated now that accessKey is available on DOMHTMLElement?

I opened:
<http://webkit.org/b/72206> ObjC API accessKey moved up to superclass DOMHTMLElement

[Nikolas Zimmermann]

This breaks the mac build! When I landed my patch from bug 72105, the problem first appeared on the bots, but several people on IRC reported that build was already broken.


Public API change. There are missing public properties and/or methods from the "DOMHTMLAreaElement" class.
@property(copy) NSString *accessKey;
Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 323.
Public API change. There are missing public properties and/or methods from the "DOMHTMLAnchorElement" class.
@property(copy) NSString *accessKey;
Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 323.

...

PublicDOMInterfaces.h in bindings/objc lists accessKey. Now if you remove it, CodeGenObjC.pm will die with a fatal error!

I guess we should roll it out quickly, it induces problems on all mac builds since a while.

[Erik Arvidsson]

This is easy to fix. You need to update Source/WebCore/bindings/objc/PublicDOMInterfaces.h and add accessKey to DOMHTMLElement  and remove it from all the its subclasses.

[Vineet Chaudhary (vineetc)]

Created attachment 114851 [details]
Updated patch as per review comments to fix mac builds.

(In reply to comment #23)
> This is easy to fix. You need to update Source/WebCore/bindings/objc/PublicDOMInterfaces.h and add accessKey to DOMHTMLElement  and remove it from all the its subclasses.

Sorry for delayed response and inconvenience caused.
As I see this patch was rolled-out in http://trac.webkit.org/changeset/100076.

Thanks Erik for help but unfortunately I don't have access to MAC machine/build. Still I have updated PublicDOMInterfaces.h moving accessKey from subclasses to base class but needs to be verified. 
It would be great help if someone with MAC access helps verifying this new patch.

[Ryosuke Niwa]

(In reply to comment #24)
> Created an attachment (id=114851) [details]
> Updated patch as per review comments to fix mac builds.

I have confirmed that this patch builds on Mac.

[Adam Barth]

Comment on attachment 114851 [details]
Updated patch as per review comments to fix mac builds.

My understanding is that this should be ok because you're just moving the method up the object hierarchy.  We can check with more Objective-C knowledgable folks if you'd like.

[Vineet Chaudhary (vineetc)]

Thanks Ryosuke, for quick update. :)


(In reply to comment #26)
> (From update of attachment 114851 [details])
> My understanding is that this should be ok because you're just moving the method up the object hierarchy.  We can check with more Objective-C knowledgable folks if you'd like.

Yes sure.
Hello Sam Weinig,

Can you please review this change relates to Objective-C tin PublicDOMInterfaces.h.

[Joseph Pecoraro]

*** Bug 72206 has been marked as a duplicate of this bug. ***

[Erik Arvidsson]

Comment on attachment 114851 [details]
Updated patch as per review comments to fix mac builds.

View in context: https://bugs.webkit.org/attachment.cgi?id=114851&action=review

Some hints on making the test cleaner.

> LayoutTests/fast/forms/access-key-for-all-elements.html:6
> +<p>This test checks to see if accesskey attributes works all elements.</p>

Use the description function instead.

> LayoutTests/fast/forms/access-key-for-all-elements.html:7
> +<div id="test"></div>

This element is never used

> LayoutTests/fast/forms/access-key-for-all-elements.html:8
> +<div id="console"></div>

Remove console element. It is not needed

> LayoutTests/fast/forms/access-key-for-all-elements.html:12
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

Remove, these are note needed

> LayoutTests/fast/forms/access-key-for-all-elements.html:14
> +var test = new Array("a","abbr","acronym","address","applet","area","article","aside","audio","b","base","basefont","bdo","bgsound","big","blockquote",

var test = ["a", ...];

> LayoutTests/fast/forms/access-key-for-all-elements.html:25
> +    var test_me = document.getElementsByTagName(test[i])[0];

WebKit style is to use camelCase.

A simpler solution might be to do:

var testElement = document.querySelector(tagNames[i]);

btw, I would probably rename test to tagNames or something more descriptive.

[Alexey Proskuryakov]

> Use the description function instead.

In fact, I recommend to the contrary. The description function is a leftover from the days when we had a separate and immutable HTML wrapper. Using a <p> element is a much more direct way to have a text in output.

> document.querySelector

Do all browsers we want to compare to support document.querySelector? It's good to get testing for newer features, as long as that doesn't make tests less compatible.

That said, does the test pass in both IE and Firefox as is?

[Vineet Chaudhary (vineetc)]

Created attachment 115012 [details]
patch

Updating patch to incorporate Erik's suggestions.

[Vineet Chaudhary (vineetc)]

Could someone help me to land this patch. This also blocking Bug 72359.

[Ryosuke Niwa]

Comment on attachment 115012 [details]
patch

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'");

[Ryosuke Niwa]

Comment on attachment 115012 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115012&action=review

Also, I'll add a test to make sure accessKey is actually working; i.e. click() is called (see http://dev.w3.org/html5/spec/Overview.html#command-facet-action).

> Source/WebCore/ChangeLog:8
> +        As per specification http://dev.w3.org/html5/spec/Overview.html#elements-in-the-dom

This should be http://dev.w3.org/html5/spec/Overview.html#the-accesskey-attribute instead.

[Vineet Chaudhary (vineetc)]

Created attachment 115558 [details]
Updated Patch

(In reply to comment #33)

> > Source/WebCore/ChangeLog:9
> > +        says All HTML elements can have the accesskey content attribute set. Adding "accessKey"
> Nit: says All?
Corrected.
 
> > Source/WebCore/ChangeLog:10
> > +        attribute idl file as [Reflect].
> Please make 'Adding "accessKey" attribute idl file as [Reflect]' a full sentence.
Corrected.

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

> > 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
Corrected.
 
> > LayoutTests/fast/forms/access-key-for-all-elements.html:1
> > +<html>
> No DOCTYPE?
Corrected.
 
> > 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.
Corrected.
 
> > 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.
Corrected.
> > LayoutTests/fast/forms/access-key-for-all-elements.html:20
> > +    var testElement = document.querySelector(tagNames[i]);
> 
> Why do we need to query-select it?
Removed as using direct created element. 
> > 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'");
Corrected.

Also modified test to make sure accessKey is actually working.
Checking onclick/onfocus events are fired for elements.

[WebKit Review Bot]

Comment on attachment 115558 [details]
Updated Patch

Attachment 115558 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10510174

New failing tests:
fast/forms/access-key-for-all-elements.html

[Vineet Chaudhary (vineetc)]

Comment on attachment 115558 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115558&action=review

> LayoutTests/fast/forms/access-key-for-all-elements-expected.txt:101
> +BUTTON element was focused.

Surprisingly when this test runs alone it sends two extra focus events, but when it run as run-webkit-tests LayoutTests/fast/forms/ ie. complete folder it doesn't send extra focus events interdependent of this patch.

[Vineet Chaudhary (vineetc)]

Created attachment 115586 [details]
fixing faling test

(In reply to comment #37)
> (From update of attachment 115558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115558&action=review
> 
> > LayoutTests/fast/forms/access-key-for-all-elements-expected.txt:101
> > +BUTTON element was focused.
> 
> Surprisingly when this test runs alone it sends two extra focus events, but when it run as run-webkit-tests LayoutTests/fast/forms/ ie. complete folder it doesn't send extra focus events interdependent of this patch.

Attempting to fix failing test case.

[Ryosuke Niwa]

Comment on attachment 115586 [details]
fixing faling test

View in context: https://bugs.webkit.org/attachment.cgi?id=115586&action=review

> Source/WebCore/html/HTMLElement.cpp:779
> +    dispatchSimulatedClick(0, sendToAnyElement);

I don't understand why we're passing sendToAnyElement to the second argument of dispatchSimulatedClick. Could you elaborate on why we do this?

[Ryosuke Niwa]

Comment on attachment 115586 [details]
fixing faling test

View in context: https://bugs.webkit.org/attachment.cgi?id=115586&action=review

> Source/WebCore/html/HTMLElement.cpp:777
>  void HTMLElement::accessKeyAction(bool sendToAnyElement)

Per IRC discussion, we should rename this argument to sendMouseEvents or sendMouseDownAndMouseUp. r- because we definitely don't want to land the patch as is.

> LayoutTests/fast/forms/access-key-for-all-elements.html:31
> +    testElement.onclick = function () { debug(testElement.tagName + ' element was clicked.') };
> +    testElement.onfocus = function () { debug(testElement.tagName + ' element was focused.') };

Instead of using debug here. You can do something like:

var clicked = false;
testElement.onclick = function () { clicked = true; }
var focused = false;
testElement.onclick = function () { focused = true; }

shouldBe("pressKey(testElement.accessKey);[clicked, focused]", expected_value);

where expected_value is specific to each tagName.

[Vineet Chaudhary (vineetc)]

Created attachment 115776 [details]
Updated Patch

(In reply to comment #40)
> (From update of attachment 115586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115586&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:777
> >  void HTMLElement::accessKeyAction(bool sendToAnyElement)
> 
> Per IRC discussion, we should rename this argument to sendMouseEvents or sendMouseDownAndMouseUp. r- because we definitely don't want to land the patch as is.

Done!

> > LayoutTests/fast/forms/access-key-for-all-elements.html:31
> > +    testElement.onclick = function () { debug(testElement.tagName + ' element was clicked.') };
> > +    testElement.onfocus = function () { debug(testElement.tagName + ' element was focused.') };
> 
> Instead of using debug here. You can do something like:
> 
> var clicked = false;
> testElement.onclick = function () { clicked = true; }
> var focused = false;
> testElement.onclick = function () { focused = true; }
> 
> shouldBe("pressKey(testElement.accessKey);[clicked, focused]", expected_value);
> 
> where expected_value is specific to each tagName.

Done!

As per the specification http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#using-the-accesskey-attribute-on-a-label-element-to-define-a-command input, button, select.. have different behavior than other elements. eg. input element only gets focused on accessKey pressed.
To test the behavior of such elements we already have test case access-key.html. Added select and textArea to the same.

[Vineet Chaudhary (vineetc)]

Comment on attachment 115776 [details]
Updated Patch

Thanks Ryosuke!

[Erik Arvidsson]

Comment on attachment 115776 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115776&action=review

FYI

> LayoutTests/fast/forms/access-key-for-all-elements.html:12
> +    if (navigator.userAgent.search(/\bMac OS X\b/) != -1)

if (/\bMac OS X\b/.test(navigator.userAgent))

is shorter and easier to understand

> LayoutTests/fast/forms/access-key-for-all-elements.html:42
> +<ol id="console"></ol>

remove console

or

fix the tag name. It should be div.

[Ryosuke Niwa]

Comment on attachment 115776 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115776&action=review

>> LayoutTests/fast/forms/access-key-for-all-elements.html:42
>> +<ol id="console"></ol>
> 
> remove console
> 
> or
> 
> fix the tag name. It should be div.

Oops, I didn't catch that. cq- because of this.

[Ryosuke Niwa]

I'll replace ol by div and land manually.

[Vineet Chaudhary (vineetc)]

Created attachment 115841 [details]
patch

(In reply to comment #45)
> I'll replace ol by div and land manually.

Ryosuke, I saw your comment lately, I already prepared corrected patch.

[WebKit Review Bot]

Comment on attachment 115841 [details]
patch

Clearing flags on attachment: 115841

Committed r100805: <http://trac.webkit.org/changeset/100805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

This change adds accessKey to DOMHTMLElement without the proper WebKit version availability macro. It really should have AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER (which hasn't been defined yet in WebKitAvailability.h).

And accessKey should likely stay on the other subclasses to indicate it was available in older releases.

(In reply to comment #49)
> This change adds accessKey to DOMHTMLElement without the proper WebKit version availability macro. It really should have AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER (which hasn't been defined yet in WebKitAvailability.h).
> 
> And accessKey should likely stay on the other subclasses to indicate it was available in older releases.

Filed bug 72756.