WebKit2 doesn't implement ChromeClient::keyboardUIMode(), so full keyboard access setting has no effect. <rdar://problem/8963023>
Created attachment 84593 [details] proposed fix
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 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.
>> +#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.
Thanks, sounds good.
Committed <http://trac.webkit.org/changeset/80279>.
http://trac.webkit.org/changeset/80279 might have broken Qt Linux Release minimal, Chromium Linux Release, and EFL Linux Release (Build)