Bug 71854

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?

(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

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;

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?

Created attachment 114247 [details] Proposed Patch Attaching proposed patch. Please let me know your comments on this.

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.

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)?

(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.

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.

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

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.

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.

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.

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

Created attachment 114462 [details] Patch Updated patch with proper indentation.

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

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.

Comment on attachment 114462 [details] Patch Clearing flags on attachment: 114462 Committed r100013: <http://trac.webkit.org/changeset/100013>

All reviewed patches have been landed. Closing bug.

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?

(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

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.

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.

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.

(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.

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.

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.

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

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.

> 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?

Created attachment 115012 [details] patch Updating patch to incorporate Erik's suggestions.

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

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

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.

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.

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

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.

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.

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?

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.

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.

Comment on attachment 115776 [details] Updated Patch Thanks Ryosuke!

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.

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.

I'll replace ol by div and land manually.

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.

Comment on attachment 115841 [details] patch Clearing flags on attachment: 115841 Committed r100805: <http://trac.webkit.org/changeset/100805>

All reviewed patches have been landed. Closing bug.

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.