AccessKeyLabel exposes the key shortcuts that we assign to an AccessKey. For example: If AccessKey is "a", AccessKeyLabel is probably "Alt + a". Here is the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-accesskeylabel
Vineet, it looks you are already implementing accessKey attribute, you might be interested to implement this too.
Thanks Arko Saha, I was about to log bug on this. Patch to follow.
See Also https://bugzilla.mozilla.org/show_bug.cgi?id=583533 Actually this bug is depends on https://bugs.webkit.org/show_bug.cgi?id=71854 but have access mark this as depend.
Created attachment 115935 [details] proposed patch Attaching proposed patch to which provides implementation of accessKeyLabel attribute. Please let me know your review comments on this.
Comment on attachment 115935 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=115935&action=review > Source/WebCore/html/HTMLElement.cpp:791 > + String accessKeyLabelValue; > + unsigned accessKeyModifierValue = EventHandler::accessKeyModifiers(); Key modifiers change rarely. It would be nice to cache the modifier string. > Source/WebCore/html/HTMLElement.cpp:796 > + if (accessKeyModifierValue & PlatformKeyboardEvent::AltKey) > + accessKeyLabelValue += "Alt+"; On the Mac, the key is named Option, not Alt. > Source/WebCore/html/HTMLElement.cpp:800 > + accessKeyLabelValue += accessKeyValue; What about platforms that do not support access keys, like I would guess most mobile ones?
> On the Mac, the key is named Option, not Alt. I would actually say that on the Mac, we might need the same format as in application menus (symbols, not key names).
(In reply to comment #5) Thank you Alexey for providing feedback on this. > (From update of attachment 115935 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115935&action=review > > > Source/WebCore/html/HTMLElement.cpp:791 > > + String accessKeyLabelValue; > > + unsigned accessKeyModifierValue = EventHandler::accessKeyModifiers(); > > Key modifiers change rarely. It would be nice to cache the modifier string. So is it good if we add member to RareData for caching? > > Source/WebCore/html/HTMLElement.cpp:800 > > + accessKeyLabelValue += accessKeyValue; > > What about platforms that do not support access keys, like I would guess most mobile ones? Thank you for pointing this! I think inn that case it should return null string. > > Source/WebCore/html/HTMLElement.cpp:796 > > + if (accessKeyModifierValue & PlatformKeyboardEvent::AltKey) > > + accessKeyLabelValue += "Alt+"; > > On the Mac, the key is named Option, not Alt. I used "Alt" looking at http://trac.webkit.org/browser/trunk/Source/WebCore/page/mac/EventHandlerMac.mm#L724 here it returns accessKeyModifiers() as "Ctrl and Alt". Should we add "Option" to this enum http://trac.webkit.org/browser/trunk/Source/WebCore/platform/PlatformKeyboardEvent.h#L85 for mac specific? > I would actually say that on the Mac, we might need the same format as in application menus (symbols, not key names). Do you mean instead of "Ctrl+<access_key>" it should return "<some_symbol>+<access_key>"? If yes may I have some symbols can be used?
Comment on attachment 115935 [details] proposed patch > So is it good if we add member to RareData for caching? Memory impact would likely be too large. I was thinking of caching just the modifiers in a static variable. > Should we add "Option" to this enum http://trac.webkit.org/browser/trunk/Source/WebCore/platform/PlatformKeyboardEvent.h#L85 for mac specific? No. For internal purposes, Alt is just fine, it's the same key, just named differently. Some modern Apple keyboard have both "option" and "alt" on the key, but not all. > Do you mean instead of "Ctrl+<access_key>" it should return "<some_symbol>+<access_key>"? If yes may I have some symbols can be used? Ctrl: U+2303 UP ARROWHEAD (⌃) Alt: U+2325 OPTION KEY (⌥) Shift: U+21E7 UPWARDS WHITE ARROW (⇧) Command: U+2318 PLACE OF INTEREST SIGN (⌘) They should come in this order. Practically, on the Mac it will usually be ⌃⌥. Marking r-, as you're going to modify the patch.
(In reply to comment #8) > (From update of attachment 115935 [details]) > Ctrl: U+2303 UP ARROWHEAD (⌃) > Alt: U+2325 OPTION KEY (⌥) > Shift: U+21E7 UPWARDS WHITE ARROW (⇧) > Command: U+2318 PLACE OF INTEREST SIGN (⌘) > > They should come in this order. Practically, on the Mac it will usually be ⌃⌥. I tried to convert Unicode symbols(⌘,⇧,⌥,⌃) to string like, accessKeyLabelValue += "⌥"; but it gives some garbage values. May be this wrong way of doing. Is there any way to convert these symbols to string? Also I observed that on MAC-book keyboard it doesn't seems to have ⇧,⌥,⌃ these keys but the key names "shift","alt" and "control". Having key-names will be consistent with other ports too (ie. like other port mac also returns key names instead of symbols).
Created attachment 116335 [details] Patch Caching the modifier string. Also added null check if any platform doesn't support accessKey Modifiers.
Created attachment 116337 [details] Patch previous patch failed as LayoutTests/platform/chromium/test_expectations.txt has got modified :(
Hello Alexey, Can you please provide feedback on this?
> but it gives some garbage values. May be this wrong way of doing. You need to escape the strings, e.g. "\u2303". > Also I observed that on MAC-book keyboard it doesn't seems to have ⇧,⌥,⌃ these keys but the key names "shift","alt" and "control". My opinion is that we should do the same thing Apple does in menus for consistency, using symbols instead of names.
More specifically, these characters would be in CharacterNames.h, not local constants.
Created attachment 116719 [details] Patch
Comment on attachment 116719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116719&action=review > Source/JavaScriptCore/wtf/unicode/CharacterNames.h:67 > +#if OS(DARWIN) These shouldn't be in ifdefs. > Source/JavaScriptCore/wtf/unicode/CharacterNames.h:84 > +const UChar upArrowHead = 0x2303; Should be "upArrowhead" to match Unicode name. > Source/JavaScriptCore/wtf/unicode/CharacterNames.h:85 > +const UChar upWardWhiteArrow = 0x21E7; Should be "upwardsWhiteArrow". > Source/WebCore/html/HTMLElement.cpp:785 > + String accessKeyValue = getAttribute(accesskeyAttr); Per HTML5, accesskey attribute can list multiple keys, which the engine will choose from. We should use assigned access key here, not the whole attribute string. > Source/WebCore/html/HTMLElement.cpp:791 > + static String accessKeyLabelValue; > + if (accessKeyLabelValue.isNull()) { Several issues here. 1. accessKeyLabelValue will not be cached when null (on platforms that don't support access keys). 2. It will not be updated when EventHandler::accessKeyModifiers() changes. That can happen in practice, see e.g. version in EventHandlerMac.mm. > Source/WebCore/html/HTMLElement.cpp:793 > +#if OS(DARWIN) Matching OS X menu style does not sound like an OS(DARWIN) thing. But I'm not sure what would be a better match. > Source/WebCore/html/HTMLElement.cpp:813 > + return String(accessKeyLabelValue + accessKeyValue); Why did you need to explicitly construct a String to return? I think that key itself should be converted to upper case - the regular way to represent shortcuts is Alt+A or ⌥A, not Alt+a and ⌥a.
View in context: https://bugs.webkit.org/attachment.cgi?id=116719&action=review >> Source/WebCore/html/HTMLElement.cpp:785 >> + String accessKeyValue = getAttribute(accesskeyAttr); > > Per HTML5, accesskey attribute can list multiple keys, which the engine will choose from. We should use assigned access key here, not the whole attribute string. Right, I think this is the bug current implementation of accessKey attribute because Looking at the current code engine does not choose form multiple keys. http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L644 it just adds the whole attribute string to buildAccessKeyMap(). Also in case of multiple keys WebKit doesn't respond to any of keys (Same as Firefox). Should we have new bug for this? >> Source/WebCore/html/HTMLElement.cpp:791 >> + if (accessKeyLabelValue.isNull()) { > > Several issues here. > > 1. accessKeyLabelValue will not be cached when null (on platforms that don't support access keys). Actually all the browsers supports/implements accessKeyModifiers() So I don't see the case where accessKeyLabelValue would be null. > 2. It will not be updated when EventHandler::accessKeyModifiers() changes. That can happen in practice, see e.g. version in EventHandlerMac.mm. In that case for MAC we can not cache accessKeyLabelValue as accessKeyModifiers may changes which is not the case with other ports. >> Source/WebCore/html/HTMLElement.cpp:813 >> + return String(accessKeyLabelValue + accessKeyValue); > > Why did you need to explicitly construct a String to return? > > I think that key itself should be converted to upper case - the regular way to represent shortcuts is Alt+A or ⌥A, not Alt+a and ⌥a. Oke I will correct that. Actually I am referring to chromium keyboardShortcut() implementation http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebAccessibilityObject.cpp#L553
> Also in case of multiple keys WebKit doesn't respond to any of keys (Same as Firefox). If it actually doesn't work at all (as opposed to using the first character), then the current code seems OK for now. We should make sure there is no disagreement with the spec, either by implementing multiple keys, or by getting the incompatible feature removed form HTML5. > > 1. accessKeyLabelValue will not be cached when null (on platforms that don't support access keys). > Actually all the browsers supports/implements accessKeyModifiers() So I don't see the case where accessKeyLabelValue would be null. As previously discussed, mobile phones generally have no keys, and don't implement accesskey. So, accessKeyLabel should be empty (and cached). > > 2. It will not be updated when EventHandler::accessKeyModifiers() changes. That can happen in practice, see e.g. version in EventHandlerMac.mm. > In that case for MAC we can not cache accessKeyLabelValue as accessKeyModifiers may changes which is not the case with other ports. You can still cache it together with modifiers. Only if modifiers change, modifier string will be recomputed.
Created attachment 117012 [details] updated_patch
May I have review comments on this?
Comment on attachment 117012 [details] updated_patch View in context: https://bugs.webkit.org/attachment.cgi?id=117012&action=review r=me on code changes, but test expectations don't seem right. > LayoutTests/platform/chromium/test_expectations.txt:3831 > +// Need rebaseline on MAC It's Mac, not MAC. It doesn't seem like the test needs a "rebaseline", it needs to be fixed.
Created attachment 117356 [details] Updated Patch corrected test expectations.
Comment on attachment 117356 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117356&action=review > LayoutTests/fast/forms/access-key-label.html:20 > +if (navigator.userAgent.search(/\bMac OS X\b/) != -1) > + shouldBe("input_item.accessKeyLabel",'Ctrl+Alt+A'); > +else > + shouldBe("input_item.accessKeyLabel","'Alt+A'"); This still looks wrong to me. These are not the expected values on Mac, so it's not something to fix in test expectations.
Created attachment 117549 [details] updated_patch (In reply to comment #23) > (From update of attachment 117356 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117356&action=review > > > LayoutTests/fast/forms/access-key-label.html:20 > > +if (navigator.userAgent.search(/\bMac OS X\b/) != -1) > > + shouldBe("input_item.accessKeyLabel",'Ctrl+Alt+A'); > > +else > > + shouldBe("input_item.accessKeyLabel","'Alt+A'"); > > This still looks wrong to me. These are not the expected values on Mac, so it's not something to fix in test expectations. Ahh right, Sorry I forgot modify the test case after new code for Mac. Currently I have access to gtk port so I have added the test in test_expectations because for Mac test behavior would be the different from gtk which I think needs to be rebaseline.
> Created an attachment (id=117549) [details] > updated_patch Hi Alexey, Is this patch oke?
Comment on attachment 117549 [details] updated_patch View in context: https://bugs.webkit.org/attachment.cgi?id=117549&action=review > LayoutTests/platform/mac/test_expectations.txt:37 > +// Needs to be rebaseline on Mac. > +BUGWK72715 : fast/forms/access-key-label.html = TEXT Why? Looks like it should be passing now.
(In reply to comment #26) > (From update of attachment 117549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117549&action=review > > > LayoutTests/platform/mac/test_expectations.txt:37 > > +// Needs to be rebaseline on Mac. > > +BUGWK72715 : fast/forms/access-key-label.html = TEXT > > Why? Looks like it should be passing now. I actually current access-key-label-expected.txt is for gtk platform on which it would returns Alt+A, but on Mac as it is expected to return ⌃⌥A it will fail isn't it? Please correct me if I am wrong.
Comment on attachment 117549 [details] updated_patch Ah right. You can do something like this for cross-platofrm results: if (navigator.userAgent.search(/\bMac OS X\b/) != -1) expected = "⌥A"; else expected = "Alt+A"; shouldBe("input_item.accessKeyLabel",'expected'); Looking at this part of the test closely, I noticed that you're missing quotes in Mac case (evaluating 'ââ¥A' will give an error).
Created attachment 118992 [details] Patch Modified test case for cross platform results.
Comment on attachment 118992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118992&action=review > Source/WebCore/html/HTMLElement.cpp:796 > + String accessKeyValue = getAttribute(accesskeyAttr); I'm not quite sure about this, but I think that the function to use is fastGetAttribute - getAttribute is only needed for animatable SVG attributes.
Comment on attachment 118992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118992&action=review >> Source/WebCore/html/HTMLElement.cpp:796 >> + String accessKeyValue = getAttribute(accesskeyAttr); > > I'm not quite sure about this, but I think that the function to use is fastGetAttribute - getAttribute is only needed for animatable SVG attributes. That’s correct. It’s also better to use const AtomicString& for the result of the function, otherwise there is unnecessary churn. > Source/WebCore/html/HTMLElement.cpp:803 > + static String accessKeyLabelValue; We don’t use variables like this that create global destructors in WebKit code. The build will fail on Mac if we do.
Comment on attachment 118992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118992&action=review > LayoutTests/fast/forms/access-key-label.html:24 > +shouldBeTrue('input_item.accessKeyLabel == expected'); Ideally should use shouldBe, not shouldBeTrue. Unfortunately that will require different results on different platforms. This test covers too little. I could delete much of the code in the patch and still pass the test. For example, I could delete the code to handle Control and Shift. And the test does not exercise the caching at all since it only tests a single label. We need a test that covers all the added code.
Comment on attachment 118992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118992&action=review > Source/WebCore/html/HTMLElement.cpp:822 > + if (oldAccessKeyModifierValue != accessKeyModifierValue) { > + oldAccessKeyModifierValue = accessKeyModifierValue; > +#if PLATFORM(MAC) > + if (accessKeyModifierValue & PlatformKeyboardEvent::CtrlKey) > + accessKeyLabelValue.append(upArrowhead); > + if (accessKeyModifierValue & PlatformKeyboardEvent::AltKey) > + accessKeyLabelValue.append(optionKey); > + if (accessKeyModifierValue & PlatformKeyboardEvent::ShiftKey) > + accessKeyLabelValue.append(upwardsWhiteArrow); > +#else > + if (accessKeyModifierValue & PlatformKeyboardEvent::CtrlKey) > + accessKeyLabelValue += "Ctrl+"; > + if (accessKeyModifierValue & PlatformKeyboardEvent::AltKey) > + accessKeyLabelValue += "Alt+"; > + if (accessKeyModifierValue & PlatformKeyboardEvent::ShiftKey) > + accessKeyLabelValue += "Shift+"; > +#endif > + } There’s an obvious bug here where if you keep doing this with different modifier combinations, the prefix will keep getting longer and longer. If you alternatively call this for Control A and Shift A you will eventually get Ctrl+Shift+Ctrl+Shift+A.
Comment on attachment 118992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118992&action=review >>> Source/WebCore/html/HTMLElement.cpp:796 >> >> I'm not quite sure about this, but I think that the function to use is fastGetAttribute - getAttribute is only needed for animatable SVG attributes. > > That’s correct. It’s also better to use const AtomicString& for the result of the function, otherwise there is unnecessary churn. Oke I will do that. >> Source/WebCore/html/HTMLElement.cpp:803 >> + static String accessKeyLabelValue; > > We don’t use variables like this that create global destructors in WebKit code. The build will fail on Mac if we do. right should be DEFINE_STATIC_LOCAL(String, accessKeyLabelValue, ()); > Source/WebCore/html/HTMLElement.cpp:806 > + oldAccessKeyModifierValue = accessKeyModifierValue; For repeating prefix mentioned below I can add check here like if (!accessKeyLabelValue.isNull()) accessKeyLabelValue = ""; >> Source/WebCore/html/HTMLElement.cpp:822 > > There’s an obvious bug here where if you keep doing this with different modifier combinations, the prefix will keep getting longer and longer. If you alternatively call this for Control A and Shift A you will eventually get Ctrl+Shift+Ctrl+Shift+A. Is it possible to call alternatively for Control A and Shift A? Because looking at the current code in EventHandlers of all ports, I think the modifiers values wont change. I mean for all the ports EventHandler::accessKeyModifiers() will give "AltKey" as modifier, except for Mac port where modifiers may change depending on VoiceOver is enabled or not. >> LayoutTests/fast/forms/access-key-label.html:24 >> +shouldBeTrue('input_item.accessKeyLabel == expected'); > > Ideally should use shouldBe, not shouldBeTrue. Unfortunately that will require different results on different platforms. > > This test covers too little. I could delete much of the code in the patch and still pass the test. For example, I could delete the code to handle Control and Shift. And the test does not exercise the caching at all since it only tests a single label. We need a test that covers all the added code. As currently modifiers could be altKey/ctrlKey or combination of both only. Should I remove "shiftKey" then? Please let me know your comments on this will modify the patch accordingly.
Comment on attachment 117012 [details] updated_patch Cleared Alexey Proskuryakov's review+ from obsolete attachment 117012 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 117356 [details] Updated Patch Cleared Alexey Proskuryakov's review+ from obsolete attachment 117356 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Progress on this? Firefox has implemented in mean-while: https://bugzilla.mozilla.org/show_bug.cgi?id=583533 Safari/WebKit is still waiting. Chromium/Blink as well https://code.google.com/p/chromium/issues/detail?id=393466
Created attachment 400318 [details] Patch
Created attachment 400326 [details] Patch
Created attachment 400330 [details] Patch
Comment on attachment 400330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400330&action=review Is this the correct string, even for unusual keys on the keyboard? > Source/WTF/wtf/unicode/CharacterNames.h:95 > +const UChar upArrowHead = 0x2303; Looked this up in the Unicode character database: Should be upArrowhead. The word "arrowhead" is one word. > Source/WebCore/html/HTMLElement.cpp:730 > + const AtomString& accessKey = attributeWithoutSynchronization(accesskeyAttr); I suggest auto& here. > Source/WebCore/html/HTMLElement.cpp:731 > + if (accessKey.isEmpty() || accessKey.isNull()) Null strings are also empty; no need to check both. > Source/WebCore/html/HTMLElement.cpp:737 > + OptionSet<PlatformEvent::Modifier> modifiers = EventHandler::accessKeyModifiers(); I suggest auto here.
(In reply to Darin Adler from comment #41) > Comment on attachment 400330 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400330&action=review > > Is this the correct string, even for unusual keys on the keyboard? > > > Source/WTF/wtf/unicode/CharacterNames.h:95 > > +const UChar upArrowHead = 0x2303; > > Looked this up in the Unicode character database: Should be upArrowhead. The > word "arrowhead" is one word. > > > Source/WebCore/html/HTMLElement.cpp:730 > > + const AtomString& accessKey = attributeWithoutSynchronization(accesskeyAttr); > > I suggest auto& here. > > > Source/WebCore/html/HTMLElement.cpp:731 > > + if (accessKey.isEmpty() || accessKey.isNull()) > > Null strings are also empty; no need to check both. > > > Source/WebCore/html/HTMLElement.cpp:737 > > + OptionSet<PlatformEvent::Modifier> modifiers = EventHandler::accessKeyModifiers(); > > I suggest auto here. All sounds like good suggestions, will land with them in. Thanks!
Created attachment 400428 [details] Patch for landing
Committed r262235: <https://trac.webkit.org/changeset/262235> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400428 [details].
<rdar://problem/63705342>