Bug 55633 – REGRESSION (WebKit2): Tab keys no longer observe Full Keyboard Access

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Alexey Proskuryakov]

WebKit2 doesn't implement ChromeClient::keyboardUIMode(), so full keyboard access setting has no effect.

<rdar://problem/8963023>

[Alexey Proskuryakov]

Created attachment 84593 [details]
proposed fix

[WebKit Review Bot]

Attachment 84593 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/wx/WebKitSupport/ChromeClientWx.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 84593 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=84593&action=review

> Source/WebCore/ChangeLog:13
> +        compared to the former.

I would say “superset of information returned by” rather than “superset of information compared to”.

> Source/WebKit2/ChangeLog:15
> +        Get the current state of full keybaord access, listening for change notifications.

Typo: keybaord.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:344
> +#if PLATFORM(MAC)
> +KeyboardUIMode WebChromeClient::keyboardUIMode()

Isn’t there a Mac-specific file to put this in?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1195
> +    bool fullKeyboardAccessEnabled = WebProcess::shared().fullKeyboardAccessEnabled();
> +    return static_cast<KeyboardUIMode>((fullKeyboardAccessEnabled ? KeyboardAccessFull : KeyboardAccessDefault) | (m_tabToLinks ? KeyboardAccessTabsToLinks : 0));

I think there’s a way to write this that avoids the typecasts:

    KeyboardUIMode mode = WebProcess::shared().fullKeyboardAccessEnabled() ? KeyboardAccessFull : KeyboardAccessDefault;
    if (m_tabToLinks)
        mode |= KeyboardAccessTabsToLinks;
    return mode;

Not sure you’ll like my version better, but if you do, please use it.

> Source/WebKit2/WebProcess/mac/FullKeyboardAccessWatcher.mm:45
> +        // The keyboard access mode is reported by two bits:
> +        // Bit 0 is set if feature is on
> +        // Bit 1 is set if full keyboard access works for any control, not just text boxes and lists.
> +        fullKeyboardAccessEnabled = (mode & 0x2);

This comment doesn’t make it clear why we look at only bit 1 here. The why part of the comment is the most important part!

> Source/WebKit2/WebProcess/mac/FullKeyboardAccessWatcher.mm:51
> +    self = [super init];

I think it would be slightly better if this class made it clear it was incorrect to create an instance. For example, in some classes I have overridden init to make it return nil or throw an exception or something like that, and then put the actual initialization in a method of a different name, say _init, with the call to [super init] there.

Nothing in the header tells you not to create an instance of this class.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:360
> +    return m_webPage->settings()->testAttribute(QWebSettings::LinksIncludedInFocusChain) ?
> +        KeyboardAccessTabsToLinks : KeyboardAccessDefault;

Normally we put the ? on the start of the second line, not the end of the first.

[Alexey Proskuryakov]

>> +#if PLATFORM(MAC)
>> +KeyboardUIMode WebChromeClient::keyboardUIMode()
>
> Isn’t there a Mac-specific file to put this in?

This should be compiled on all platforms, thanks for catching it!

> I think there’s a way to write this that avoids the typecasts:

GCC gives an error on your code (the result of |= is an int). Actually, I think that it's a mistake for KeyboardUIMode to be an enum!

    enum KeyboardUIMode {
        KeyboardAccessDefault     = 0x00000000,
        KeyboardAccessFull        = 0x00000001,
        // this flag may be or'ed with either of the two above
        KeyboardAccessTabsToLinks = 0x10000000
    };

> This comment doesn’t make it clear why we look at only bit 1 here. The why part of the comment is the most important part!

I don't know why we do it. There doesn't seem to be any definitive proof, even internal (some other code does the same, but that's it). I mentioned it in ChangeLog that even the remaining part of the comment may be not true, but I didn't feel comfortable removing it.

> I think it would be slightly better if this class made it clear it was incorrect to create an instance.

Since you said r+, I'm going to land this part as is. I'll keep this in mind in the future.

[Darin Adler]

Thanks, sounds good.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/80279>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80279 might have broken Qt Linux Release minimal, Chromium Linux Release, and EFL Linux Release (Build)