[Webkit-unassigned] [Bug 72715] Implement AccessKeyLabel attribute.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72715





--- Comment #34 from Vineet Chaudhary (vineetc) <rgf748 at motorola.com>  2011-12-14 01:20:28 PST ---
(From update of attachment 118992)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.