Bug 141704

Summary: [Win] Popup menu is not accessible.
Product: WebKit Reporter: peavo
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch bfulgham: review+

Description peavo 2015-02-17 07:28:31 PST
It is not currently possible to get accessibility information for items in a popup menu.
Comment 1 peavo 2015-02-17 07:50:45 PST
Created attachment 246740 [details]
Patch
Comment 2 WebKit Commit Bot 2015-02-17 07:52:25 PST
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.
Comment 3 peavo 2015-02-17 07:55:00 PST
I'm not sure the style issues can be fixed, since we are implementing methods in a Windows interface (IAccessible).
Comment 4 peavo 2015-04-23 11:45:13 PDT
Created attachment 251459 [details]
Patch
Comment 5 peavo 2015-04-23 11:46:11 PDT
Rebased patch.
Comment 6 WebKit Commit Bot 2015-04-23 11:47:39 PDT
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.
Comment 7 Brent Fulgham 2015-04-23 12:10:57 PDT
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.
Comment 8 peavo 2015-04-23 13:32:23 PDT
Created attachment 251480 [details]
Patch
Comment 9 WebKit Commit Bot 2015-04-23 13:34:40 PDT
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.
Comment 10 peavo 2015-04-23 13:36:30 PDT
(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 :)
Comment 11 Brent Fulgham 2015-04-23 16:08:59 PDT
Comment on attachment 251480 [details]
Patch

r=me! Thanks for working on this.
Comment 12 peavo 2015-04-24 04:47:26 PDT
Committed r183262: <http://trac.webkit.org/changeset/183262>