Bug 68201 - Report AXValueChanged when value changes in element with role=textbox.
: Report AXValueChanged when value changes in element with role=textbox.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-09-15 17:47 PST by
Modified: 2011-09-21 09:44 PST (History)


Attachments
Patch (6.65 KB, patch)
2011-09-15 17:48 PST, Alice Boxhall
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2011-09-16 13:35 PST, Alice Boxhall
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.77 KB, patch)
2011-09-16 13:42 PST, Alice Boxhall
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2011-09-16 16:34 PST, Alice Boxhall
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.82 KB, patch)
2011-09-19 23:26 PST, Alice Boxhall
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2011-09-20 17:53 PST, Alice Boxhall
no flags 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-09-15 17:47:37 PST
Report AXValueChanged when value changes in element with role=textbox.
------- Comment #1 From 2011-09-15 17:48:28 PST -------
Created an attachment (id=107573) [details]
Patch
------- Comment #2 From 2011-09-15 17:49:48 PST -------
(From update of attachment 107573 [details])
Oops, will remove the .pbxproj file shortly.
------- Comment #3 From 2011-09-16 09:58:16 PST -------
(From update of attachment 107573 [details])
i think using isTextControl() may cast a wider net then we want. won't it include native text controls tools? we should probably limit this to aria text controls, because the native controls are already handled
------- Comment #4 From 2011-09-16 10:03:05 PST -------
(In reply to comment #3)
> (From update of attachment 107573 [details] [details])
> i think using isTextControl() may cast a wider net then we want. won't it include native text controls tools? we should probably limit this to aria text controls, because the native controls are already handled

Ah yes, you're right: I was confused because it checks for textbox AX roles, but they are set for native text boxes as well.
------- Comment #5 From 2011-09-16 13:35:29 PST -------
Created an attachment (id=107717) [details]
Patch
------- Comment #6 From 2011-09-16 13:39:56 PST -------
(From update of attachment 107717 [details])
Just noticed an error in this patch.
------- Comment #7 From 2011-09-16 13:42:42 PST -------
Created an attachment (id=107718) [details]
Patch
------- Comment #8 From 2011-09-16 14:32:38 PST -------
(From update of attachment 107718 [details])
i would move isARIATextControl to AXObject.h so that you don't have to do toAccessibilityRenderObject
That's the direction the code has been moving so that we can abstract away more from AXRenderObject and only have to worry about AXObject
otherwise looks ok to me
------- Comment #9 From 2011-09-16 16:34:07 PST -------
Created an attachment (id=107743) [details]
Patch
------- Comment #10 From 2011-09-16 16:35:56 PST -------
(In reply to comment #8)
> (From update of attachment 107718 [details] [details])
> i would move isARIATextControl to AXObject.h so that you don't have to do toAccessibilityRenderObject
> That's the direction the code has been moving so that we can abstract away more from AXRenderObject and only have to worry about AXObject
> otherwise looks ok to me

Done.
------- Comment #11 From 2011-09-16 18:02:31 PST -------
(From update of attachment 107743 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=107743&action=review

r=me, after fix for the brackets

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3371
>          }

no brackets needed for this one line else if
------- Comment #12 From 2011-09-19 23:26:27 PST -------
Created an attachment (id=107968) [details]
Patch
------- Comment #13 From 2011-09-19 23:30:11 PST -------
(From update of attachment 107743 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=107743&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3371
>>          }
> 
> no brackets needed for this one line else if

Actually, I ended up changing this a bit more significantly: removed the else, and the break above. My reasoning: 
- The method below doesn't break out of the loop when it posts the AXLiveRegionChanged notification, and it seems like the logic should be similar across the two methods (in fact, possibly the loop needs to be refactored out into a common method)
- If there is both aria-live=true and role=textbox, both notifications are relevant

Please note, I also modified the selection-value-changes-for-aria-textbox LayoutTest as part of this update; it was broken by this change before.
------- Comment #14 From 2011-09-20 08:53:02 PST -------
(From update of attachment 107968 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=107968&action=review

thanks. a few thoughts inline

> LayoutTests/platform/mac/accessibility/selection-value-changes-for-aria-textbox.html:34
>          }

it seems like we should not be sending out two AXValueChange notifications for the same object (For the same value change). 
i suspect we might want to check that an object is only an aria text control and not a standard text control...

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3370
>      }

it looks like there's already a local instance of axObjectCache() (cache) that can be used here (and also in the live region block)
------- Comment #15 From 2011-09-20 17:53:02 PST -------
Created an attachment (id=108091) [details]
Patch
------- Comment #16 From 2011-09-20 18:00:07 PST -------
(From update of attachment 107968 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=107968&action=review

>> LayoutTests/platform/mac/accessibility/selection-value-changes-for-aria-textbox.html:34
>>          }
> 
> it seems like we should not be sending out two AXValueChange notifications for the same object (For the same value change). 
> i suspect we might want to check that an object is only an aria text control and not a standard text control...

That makes sense. This is what happens when I try to code the same day as getting off a transcontinental flight.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3370
>>      }
> 
> it looks like there's already a local instance of axObjectCache() (cache) that can be used here (and also in the live region block)

Fixed.
------- Comment #17 From 2011-09-21 08:41:54 PST -------
(From update of attachment 108091 [details])
looks ok. r=me
------- Comment #18 From 2011-09-21 09:44:18 PST -------
(From update of attachment 108091 [details])
Clearing flags on attachment: 108091

Committed r95641: <http://trac.webkit.org/changeset/95641>
------- Comment #19 From 2011-09-21 09:44:23 PST -------
All reviewed patches have been landed.  Closing bug.