Bug 141704 – [Win] Popup menu is not accessible.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

It is not currently possible to get accessibility information for items in a popup menu.

Created attachment 246740 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1149:  AccessiblePopupMenu::get_accParent is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1154:  AccessiblePopupMenu::get_accChildCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1160:  AccessiblePopupMenu::get_accChild is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1170:  AccessiblePopupMenu::get_accName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1175:  AccessiblePopupMenu::get_accValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1191:  AccessiblePopupMenu::get_accDescription is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1196:  AccessiblePopupMenu::get_accRole is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1213:  AccessiblePopupMenu::get_accState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1224:  AccessiblePopupMenu::get_accHelp is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1229:  AccessiblePopupMenu::get_accKeyboardShortcut is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1234:  AccessiblePopupMenu::get_accFocus is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1239:  AccessiblePopupMenu::get_accSelection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1244:  AccessiblePopupMenu::get_accDefaultAction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1368:  AccessiblePopupMenu::put_accName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1373:  AccessiblePopupMenu::put_accValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1378:  AccessiblePopupMenu::get_accHelpTopic is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 16 in 4 files


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

I'm not sure the style issues can be fixed, since we are implementing methods in a Windows interface (IAccessible).

Created attachment 251459 [details]
Patch

Rebased patch.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1149:  AccessiblePopupMenu::get_accParent is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1154:  AccessiblePopupMenu::get_accChildCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1160:  AccessiblePopupMenu::get_accChild is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1170:  AccessiblePopupMenu::get_accName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1175:  AccessiblePopupMenu::get_accValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1191:  AccessiblePopupMenu::get_accDescription is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1196:  AccessiblePopupMenu::get_accRole is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1213:  AccessiblePopupMenu::get_accState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1224:  AccessiblePopupMenu::get_accHelp is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1229:  AccessiblePopupMenu::get_accKeyboardShortcut is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1234:  AccessiblePopupMenu::get_accFocus is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1239:  AccessiblePopupMenu::get_accSelection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1244:  AccessiblePopupMenu::get_accDefaultAction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1368:  AccessiblePopupMenu::put_accName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1373:  AccessiblePopupMenu::put_accValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1378:  AccessiblePopupMenu::get_accHelpTopic is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 16 in 4 files


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

[Brent Fulgham]

Comment on attachment 251459 [details]
Patch

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

Fantastic! I'm so excited you have this working (as I'm sure James Craig will be, too). I don't think it's quite ready to land, but it's almost there.

Are there any tests we can unskip with this fix in place?

> Source/WebCore/platform/win/PopupMenuWin.cpp:756
> +    if (lParam != OBJID_CLIENT)

This should be written as "if (static_cast<LONG>(lParam) != OBJID_CLIENT)"

See <https://bugs.webkit.org/show_bug.cgi?id=141391>

> Source/WebCore/platform/win/PopupMenuWin.cpp:1155
> +{

I like to check for null, and return E_POINTER if it's not valid.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1161
> +{

Ditto E_POINTER.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1176
> +{

E_POINTER if value is NULL.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1186
> +    *value = SysAllocString(itemText.characters16());

You might see if the BString class provides this functionality in a single function call.

BString itemText(m_popupMenu.client()->itemText(index));
*value = itemText.release();

> Source/WebCore/platform/win/PopupMenuWin.cpp:1197
> +{

E_POINTER

> Source/WebCore/platform/win/PopupMenuWin.cpp:1214
> +{

E_POINTER

> Source/WebCore/platform/win/PopupMenuWin.cpp:1255
> +{

E_POINTER for null 'left', 'top', etc.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1324
> +        return E_POINTER;

Yay! :-)

> Source/WebCore/platform/win/ScrollbarThemeWin.h:38
> +    virtual int scrollbarThickness(ScrollbarControlSize = RegularScrollbar) override;

We have been removing the 'virtual' when we use 'override'.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:40
> +    virtual void themeChanged() override;

Ditto.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:46
> +    virtual IntRect trackRect(ScrollbarThemeClient*, bool painting = false) override;

Ditto.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:53
> +    virtual bool shouldSnapBackToDragOrigin(ScrollbarThemeClient*, const PlatformMouseEvent&) override;

Ditto.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:58
> +    virtual void paintThumb(GraphicsContext*, ScrollbarThemeClient*, const IntRect&) override;

Ditto.

Created attachment 251480 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1150:  AccessiblePopupMenu::get_accParent is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1155:  AccessiblePopupMenu::get_accChildCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1164:  AccessiblePopupMenu::get_accChild is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1177:  AccessiblePopupMenu::get_accName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1182:  AccessiblePopupMenu::get_accValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1201:  AccessiblePopupMenu::get_accDescription is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1206:  AccessiblePopupMenu::get_accRole is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1226:  AccessiblePopupMenu::get_accState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1240:  AccessiblePopupMenu::get_accHelp is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1245:  AccessiblePopupMenu::get_accKeyboardShortcut is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1250:  AccessiblePopupMenu::get_accFocus is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1255:  AccessiblePopupMenu::get_accSelection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1260:  AccessiblePopupMenu::get_accDefaultAction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1387:  AccessiblePopupMenu::put_accName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1392:  AccessiblePopupMenu::put_accValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PopupMenuWin.cpp:1397:  AccessiblePopupMenu::get_accHelpTopic is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 16 in 4 files


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

(In reply to comment #7)
> Comment on attachment 251459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251459&action=review
> 
> Fantastic! I'm so excited you have this working (as I'm sure James Craig
> will be, too). I don't think it's quite ready to land, but it's almost there.
> 

Thanks alot for reviewing :)

> Are there any tests we can unskip with this fix in place?
> 

To be honest, I don't know which tests cover this, but I can have a look :)

[Brent Fulgham]

Comment on attachment 251480 [details]
Patch

r=me! Thanks for working on this.

Committed r183262: <http://trac.webkit.org/changeset/183262>