Bug 72715 - Implement AccessKeyLabel attribute.
Summary: Implement AccessKeyLabel attribute.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL: http://www.whatwg.org/specs/web-apps/...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-11-18 04:13 PST by Arko Saha
Modified: 2020-05-27 22:33 PDT (History)
18 users (show)

See Also:


Attachments
proposed patch (5.46 KB, patch)
2011-11-19 00:10 PST, Vineet Chaudhary (vineetc)
ap: review-
Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2011-11-23 02:53 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Patch (7.92 KB, patch)
2011-11-23 03:02 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Patch (10.77 KB, patch)
2011-11-28 02:36 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
updated_patch (10.83 KB, patch)
2011-11-29 11:44 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Updated Patch (10.79 KB, patch)
2011-11-30 23:54 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
updated_patch (10.82 KB, patch)
2011-12-01 20:07 PST, Vineet Chaudhary (vineetc)
ap: review-
Details | Formatted Diff | Diff
Patch (8.71 KB, patch)
2011-12-13 03:17 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Patch (8.99 KB, patch)
2020-05-27 03:04 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (26.01 KB, patch)
2020-05-27 06:16 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (27.36 KB, patch)
2020-05-27 07:00 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (27.29 KB, patch)
2020-05-27 21:55 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 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 Arko Saha 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 Vineet Chaudhary (vineetc) 2011-11-18 04:21:18 PST
Thanks Arko Saha,

I was about to log bug on this. Patch to follow.
Comment 3 Vineet Chaudhary (vineetc) 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 Vineet Chaudhary (vineetc) 2011-11-19 00:10:44 PST
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 5 Alexey Proskuryakov 2011-11-19 02:03:28 PST
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?
Comment 6 Alexey Proskuryakov 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 Vineet Chaudhary (vineetc) 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])
> 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 Alexey Proskuryakov 2011-11-21 11:28:52 PST
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.
Comment 9 Vineet Chaudhary (vineetc) 2011-11-22 04:48:02 PST
(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).
Comment 10 Vineet Chaudhary (vineetc) 2011-11-23 02:53:40 PST
Created attachment 116335 [details]
Patch

Caching  the modifier string.

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

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

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

corrected test expectations.
Comment 23 Alexey Proskuryakov 2011-12-01 09:01:49 PST
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.
Comment 24 Vineet Chaudhary (vineetc) 2011-12-01 20:07:19 PST
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.
Comment 25 Vineet Chaudhary (vineetc) 2011-12-05 12:03:03 PST
> Created an attachment (id=117549) [details]
> updated_patch

Hi Alexey,
Is this patch oke?
Comment 26 Alexey Proskuryakov 2011-12-05 12:28:26 PST
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.
Comment 27 Vineet Chaudhary (vineetc) 2011-12-05 19:46:20 PST
(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 28 Alexey Proskuryakov 2011-12-05 21:19:41 PST
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).
Comment 29 Vineet Chaudhary (vineetc) 2011-12-13 03:17:08 PST
Created attachment 118992 [details]
Patch

Modified test case for cross platform results.
Comment 30 Alexey Proskuryakov 2011-12-13 09:05:57 PST
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 31 Darin Adler 2011-12-13 09:33:02 PST
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 32 Darin Adler 2011-12-13 09:34:47 PST
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 33 Darin Adler 2011-12-13 09:37:58 PST
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 34 Vineet Chaudhary (vineetc) 2011-12-14 01:20:28 PST
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 35 Eric Seidel (no email) 2011-12-19 10:44:50 PST
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 36 Eric Seidel (no email) 2011-12-19 10:44:56 PST
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.
Comment 37 krinklemail 2014-07-13 11:27:52 PDT
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
Comment 38 Noam Rosenthal 2020-05-27 03:04:23 PDT
Created attachment 400318 [details]
Patch
Comment 39 Noam Rosenthal 2020-05-27 06:16:04 PDT
Created attachment 400326 [details]
Patch
Comment 40 Noam Rosenthal 2020-05-27 07:00:08 PDT
Created attachment 400330 [details]
Patch
Comment 41 Darin Adler 2020-05-27 10:15:21 PDT
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.
Comment 42 Noam Rosenthal 2020-05-27 10:36:45 PDT
(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!
Comment 43 Noam Rosenthal 2020-05-27 21:55:54 PDT
Created attachment 400428 [details]
Patch for landing
Comment 44 EWS 2020-05-27 22:32:54 PDT
Committed r262235: <https://trac.webkit.org/changeset/262235>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400428 [details].
Comment 45 Radar WebKit Bug Importer 2020-05-27 22:33:18 PDT
<rdar://problem/63705342>