Bug 72715 – Implement AccessKeyLabel attribute.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Arko Saha]

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

[Arko Saha]

Vineet, it looks you are already implementing accessKey attribute, you might be interested to implement this too.

[Vineet Chaudhary (vineetc)]

Thanks Arko Saha,

I was about to log bug on this. Patch to follow.

[Vineet Chaudhary (vineetc)]

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.

[Vineet Chaudhary (vineetc)]

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.

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

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

[Vineet Chaudhary (vineetc)]

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

[Alexey Proskuryakov]

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.

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

Created attachment 116335 [details]
Patch

Caching  the modifier string.

Also added null check if any platform doesn't support accessKey Modifiers.

[Vineet Chaudhary (vineetc)]

Created attachment 116337 [details]
Patch

previous patch failed as LayoutTests/platform/chromium/test_expectations.txt has got modified :(

[Vineet Chaudhary (vineetc)]

Hello Alexey,

Can you please provide feedback on this?

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

More specifically, these characters would be in CharacterNames.h, not local constants.

[Vineet Chaudhary (vineetc)]

Created attachment 116719 [details]
Patch

[Alexey Proskuryakov]

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.

[Vineet Chaudhary (vineetc)]

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

[Alexey Proskuryakov]

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

[Vineet Chaudhary (vineetc)]

Created attachment 117012 [details]
updated_patch

[Vineet Chaudhary (vineetc)]

May I have review comments on this?

[Alexey Proskuryakov]

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.

[Vineet Chaudhary (vineetc)]

Created attachment 117356 [details]
Updated Patch

corrected test expectations.

[Alexey Proskuryakov]

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.

[Vineet Chaudhary (vineetc)]

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.

[Vineet Chaudhary (vineetc)]

> Created an attachment (id=117549) [details]
> updated_patch

Hi Alexey,
Is this patch oke?

[Alexey Proskuryakov]

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.

[Vineet Chaudhary (vineetc)]

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

[Alexey Proskuryakov]

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

[Vineet Chaudhary (vineetc)]

Created attachment 118992 [details]
Patch

Modified test case for cross platform results.

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Vineet Chaudhary (vineetc)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Noam Rosenthal]

Created attachment 400318 [details]
Patch

[Noam Rosenthal]

Created attachment 400326 [details]
Patch

[Noam Rosenthal]

Created attachment 400330 [details]
Patch

[Darin Adler]

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.

[Noam Rosenthal]

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

[Noam Rosenthal]

Created attachment 400428 [details]
Patch for landing

[EWS]

Committed r262235: <https://trac.webkit.org/changeset/262235>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400428 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63705342>