WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68201
Report AXValueChanged when value changes in element with role=textbox.
https://bugs.webkit.org/show_bug.cgi?id=68201
Summary
Report AXValueChanged when value changes in element with role=textbox.
Alice Boxhall
Reported
2011-09-15 17:47:37 PDT
Report AXValueChanged when value changes in element with role=textbox.
Attachments
Patch
(6.65 KB, patch)
2011-09-15 17:48 PDT
,
Alice Boxhall
no flags
Details
Formatted Diff
Diff
Patch
(6.79 KB, patch)
2011-09-16 13:35 PDT
,
Alice Boxhall
no flags
Details
Formatted Diff
Diff
Patch
(6.77 KB, patch)
2011-09-16 13:42 PDT
,
Alice Boxhall
no flags
Details
Formatted Diff
Diff
Patch
(7.43 KB, patch)
2011-09-16 16:34 PDT
,
Alice Boxhall
no flags
Details
Formatted Diff
Diff
Patch
(10.82 KB, patch)
2011-09-19 23:26 PDT
,
Alice Boxhall
no flags
Details
Formatted Diff
Diff
Patch
(8.88 KB, patch)
2011-09-20 17:53 PDT
,
Alice Boxhall
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Alice Boxhall
Comment 1
2011-09-15 17:48:28 PDT
Created
attachment 107573
[details]
Patch
Alice Boxhall
Comment 2
2011-09-15 17:49:48 PDT
Comment on
attachment 107573
[details]
Patch Oops, will remove the .pbxproj file shortly.
chris fleizach
Comment 3
2011-09-16 09:58:16 PDT
Comment on
attachment 107573
[details]
Patch 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
Alice Boxhall
Comment 4
2011-09-16 10:03:05 PDT
(In reply to
comment #3
)
> (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
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.
Alice Boxhall
Comment 5
2011-09-16 13:35:29 PDT
Created
attachment 107717
[details]
Patch
Alice Boxhall
Comment 6
2011-09-16 13:39:56 PDT
Comment on
attachment 107717
[details]
Patch Just noticed an error in this patch.
Alice Boxhall
Comment 7
2011-09-16 13:42:42 PDT
Created
attachment 107718
[details]
Patch
chris fleizach
Comment 8
2011-09-16 14:32:38 PDT
Comment on
attachment 107718
[details]
Patch 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
Alice Boxhall
Comment 9
2011-09-16 16:34:07 PDT
Created
attachment 107743
[details]
Patch
Alice Boxhall
Comment 10
2011-09-16 16:35:56 PDT
(In reply to
comment #8
)
> (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
Done.
chris fleizach
Comment 11
2011-09-16 18:02:31 PDT
Comment on
attachment 107743
[details]
Patch 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
Alice Boxhall
Comment 12
2011-09-19 23:26:27 PDT
Created
attachment 107968
[details]
Patch
Alice Boxhall
Comment 13
2011-09-19 23:30:11 PDT
Comment on
attachment 107743
[details]
Patch 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.
chris fleizach
Comment 14
2011-09-20 08:53:02 PDT
Comment on
attachment 107968
[details]
Patch 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)
Alice Boxhall
Comment 15
2011-09-20 17:53:02 PDT
Created
attachment 108091
[details]
Patch
Alice Boxhall
Comment 16
2011-09-20 18:00:07 PDT
Comment on
attachment 107968
[details]
Patch 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.
chris fleizach
Comment 17
2011-09-21 08:41:54 PDT
Comment on
attachment 108091
[details]
Patch looks ok. r=me
WebKit Review Bot
Comment 18
2011-09-21 09:44:18 PDT
Comment on
attachment 108091
[details]
Patch Clearing flags on attachment: 108091 Committed
r95641
: <
http://trac.webkit.org/changeset/95641
>
WebKit Review Bot
Comment 19
2011-09-21 09:44:23 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug