Bug 55633 - REGRESSION (WebKit2): Tab keys no longer observe Full Keyboard Access
: REGRESSION (WebKit2): Tab keys no longer observe Full Keyboard Access
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P1 Normal
Assigned To:
:
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2011-03-02 17:16 PST by
Modified: 2011-03-03 13:44 PST (History)


Attachments
proposed fix (41.00 KB, patch)
2011-03-03 11:14 PST, Alexey Proskuryakov
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-03-02 17:16:22 PST
WebKit2 doesn't implement ChromeClient::keyboardUIMode(), so full keyboard access setting has no effect.

<rdar://problem/8963023>
------- Comment #1 From 2011-03-03 11:14:54 PST -------
Created an attachment (id=84593) [details]
proposed fix
------- Comment #2 From 2011-03-03 11:16:45 PST -------
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.
------- Comment #3 From 2011-03-03 11:30:20 PST -------
(From update of attachment 84593 [details])
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.
------- Comment #4 From 2011-03-03 11:45:57 PST -------
>> +#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.
------- Comment #5 From 2011-03-03 11:57:54 PST -------
Thanks, sounds good.
------- Comment #6 From 2011-03-03 13:34:56 PST -------
Committed <http://trac.webkit.org/changeset/80279>.
------- Comment #7 From 2011-03-03 13:44:27 PST -------
http://trac.webkit.org/changeset/80279 might have broken Qt Linux Release minimal, Chromium Linux Release, and EFL Linux Release (Build)