Bug 72715 - Implement AccessKeyLabel attribute.
: Implement AccessKeyLabel attribute.
Status: UNCONFIRMED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://www.whatwg.org/specs/web-apps/...
:
:
:
  Show dependency treegraph
 
Reported: 2011-11-18 04:13 PST by
Modified: 2012-05-15 00:54 PST (History)


Attachments
proposed patch (5.46 KB, patch)
2011-11-19 00:10 PST, Vineet Chaudhary (vineetc)
ap: review-
Review Patch | Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2011-11-23 02:53 PST, Vineet Chaudhary (vineetc)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.92 KB, patch)
2011-11-23 03:02 PST, Vineet Chaudhary (vineetc)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.77 KB, patch)
2011-11-28 02:36 PST, Vineet Chaudhary (vineetc)
no flags Review Patch | Details | Formatted Diff | Diff
updated_patch (10.83 KB, patch)
2011-11-29 11:44 PST, Vineet Chaudhary (vineetc)
no flags Review Patch | Details | Formatted Diff | Diff
Updated Patch (10.79 KB, patch)
2011-11-30 23:54 PST, Vineet Chaudhary (vineetc)
no flags Review Patch | Details | Formatted Diff | Diff
updated_patch (10.82 KB, patch)
2011-12-01 20:07 PST, Vineet Chaudhary (vineetc)
ap: review-
Review Patch | Details | Formatted Diff | Diff
Patch (8.71 KB, patch)
2011-12-13 03:17 PST, Vineet Chaudhary (vineetc)
darin: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-18 04:13:42 PST
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
------- Comment #1 From 2011-11-18 04:17:17 PST -------
Vineet, it looks you are already implementing accessKey attribute, you might be interested to implement this too.
------- Comment #2 From 2011-11-18 04:21:18 PST -------
Thanks Arko Saha,

I was about to log bug on this. Patch to follow.
------- Comment #3 From 2011-11-18 10:34:25 PST -------
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.
------- Comment #4 From 2011-11-19 00:10:44 PST -------
Created an attachment (id=115935) [details]
proposed patch

Attaching proposed patch to which provides implementation of accessKeyLabel attribute.
Please let me know your review comments on this.
------- Comment #5 From 2011-11-19 02:03:28 PST -------
(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.

> 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?
------- Comment #6 From 2011-11-19 02:05:19 PST -------
> 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).
------- Comment #7 From 2011-11-21 04:17:22 PST -------
(In reply to comment #5)
Thank you Alexey for providing feedback on this.

> (From update of attachment 115935 [details] [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 #8 From 2011-11-21 11:28:52 PST -------
(From update of attachment 115935 [details])
> 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.
------- Comment #9 From 2011-11-22 04:48:02 PST -------
(In reply to comment #8)
> (From update of attachment 115935 [details] [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).
------- Comment #10 From 2011-11-23 02:53:40 PST -------
Created an attachment (id=116335) [details]
Patch

Caching  the modifier string.

Also added null check if any platform doesn't support accessKey Modifiers.
------- Comment #11 From 2011-11-23 03:02:37 PST -------
Created an attachment (id=116337) [details]
Patch

previous patch failed as LayoutTests/platform/chromium/test_expectations.txt has got modified :(
------- Comment #12 From 2011-11-24 23:20:19 PST -------
Hello Alexey,

Can you please provide feedback on this?
------- Comment #13 From 2011-11-25 10:42:36 PST -------
> 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.
------- Comment #14 From 2011-11-25 18:47:31 PST -------
More specifically, these characters would be in CharacterNames.h, not local constants.
------- Comment #15 From 2011-11-28 02:36:27 PST -------
Created an attachment (id=116719) [details]
Patch
------- Comment #16 From 2011-11-28 08:59:10 PST -------
(From update of attachment 116719 [details])
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.
------- Comment #17 From 2011-11-29 09:07:13 PST -------
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
------- Comment #18 From 2011-11-29 09:45:46 PST -------
> 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.
------- Comment #19 From 2011-11-29 11:44:33 PST -------
Created an attachment (id=117012) [details]
updated_patch
------- Comment #20 From 2011-11-30 21:56:33 PST -------
May I have review comments on this?
------- Comment #21 From 2011-11-30 22:55:04 PST -------
(From update of attachment 117012 [details])
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.
------- Comment #22 From 2011-11-30 23:54:47 PST -------
Created an attachment (id=117356) [details]
Updated Patch

corrected test expectations.
------- Comment #23 From 2011-12-01 09:01:49 PST -------
(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.
------- Comment #24 From 2011-12-01 20:07:19 PST -------
Created an attachment (id=117549) [details]
updated_patch

(In reply to comment #23)
> (From update of attachment 117356 [details] [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.
------- Comment #25 From 2011-12-05 12:03:03 PST -------
> Created an attachment (id=117549) [details] [details]
> updated_patch

Hi Alexey,
Is this patch oke?
------- Comment #26 From 2011-12-05 12:28:26 PST -------
(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.
------- Comment #27 From 2011-12-05 19:46:20 PST -------
(In reply to comment #26)
> (From update of attachment 117549 [details] [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 #28 From 2011-12-05 21:19:41 PST -------
(From update of attachment 117549 [details])
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).
------- Comment #29 From 2011-12-13 03:17:08 PST -------
Created an attachment (id=118992) [details]
Patch

Modified test case for cross platform results.
------- Comment #30 From 2011-12-13 09:05:57 PST -------
(From update of attachment 118992 [details])
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 #31 From 2011-12-13 09:33:02 PST -------
(From update of attachment 118992 [details])
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 #32 From 2011-12-13 09:34:47 PST -------
(From update of attachment 118992 [details])
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 #33 From 2011-12-13 09:37:58 PST -------
(From update of attachment 118992 [details])
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 #34 From 2011-12-14 01:20:28 PST -------
(From update of attachment 118992 [details])
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 #35 From 2011-12-19 10:44:50 PST -------
(From update of attachment 117012 [details])
Cleared Alexey Proskuryakov's review+ from obsolete attachment 117012 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #36 From 2011-12-19 10:44:56 PST -------
(From update of attachment 117356 [details])
Cleared Alexey Proskuryakov's review+ from obsolete attachment 117356 [details] so that this bug does not appear in http://webkit.org/pending-commit.