Bug 130386

Summary: AX: Bug in AccessibilityNodeObject::helpText
Product: WebKit Reporter: Dominic Mazzoni <dmazzoni>
Component: AccessibilityAssignee: James Craig <jcraig>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, clopez, commit-queue, jcraig, jdiggs, mario, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
cfleizach: review-, cfleizach: commit-queue-
patch with review feedback none

Dominic Mazzoni
Reported 2014-03-17 23:39:33 PDT
Discovered by ch.dumez@samsung.com in Blink (Chromium) code: https://code.google.com/p/chromium/issues/detail?id=352771 The bug was introduced here: https://bugs.webkit.org/show_bug.cgi?id=99502 In this snippet, getAttribute should be curr->getAttribute. 1403 for (Node* curr = node(); curr; curr = curr->parentNode()) { 1404 const AtomicString& summary = getAttribute(summaryAttr); 1405 if (!summary.isEmpty()) 1406 textOrder.append(AccessibilityText(summary, SummaryText)); 1407 1408 // The title attribute should be used as help text unless it is already being used as descriptive text. 1409 const AtomicString& title = getAttribute(titleAttr); 1410 if (!title.isEmpty()) 1411 textOrder.append(AccessibilityText(title, TitleTagText));
Attachments
patch (7.04 KB, patch)
2014-03-31 02:40 PDT, James Craig 		no flags
DetailsFormatted DiffDiff
patch (8.09 KB, patch)
2014-04-01 18:19 PDT, James Craig 		cfleizach: review-
cfleizach: commit-queue-
DetailsFormatted DiffDiff
patch with review feedback (7.91 KB, patch)
2014-04-08 23:35 PDT, James Craig 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
chris fleizach
Comment 1 2014-03-17 23:40:46 PDT
good catch
Radar WebKit Bug Importer
Comment 2 2014-03-18 12:41:15 PDT
<rdar://problem/16357555>
James Craig
Comment 3 2014-03-31 02:40:45 PDT
Created attachment 228161 [details] patch Might need some better examples in the test, and/or commented in the source. I can't really determine why we're ever getting helpText from the parentNode, so I'm not sure this is an effective test.
chris fleizach
Comment 4 2014-04-01 10:06:08 PDT
(In reply to comment #0) > Discovered by ch.dumez@samsung.com in Blink (Chromium) code: https://code.google.com/p/chromium/issues/detail?id=352771 > The bug was introduced here: https://bugs.webkit.org/show_bug.cgi?id=99502 > > In this snippet, getAttribute should be curr->getAttribute. > > 1403 for (Node* curr = node(); curr; curr = curr->parentNode()) { > 1404 const AtomicString& summary = getAttribute(summaryAttr); > 1405 if (!summary.isEmpty()) > 1406 textOrder.append(AccessibilityText(summary, SummaryText)); > 1407 > 1408 // The title attribute should be used as help text unless it is already being used as descriptive text. > 1409 const AtomicString& title = getAttribute(titleAttr); > 1410 if (!title.isEmpty()) > 1411 textOrder.append(AccessibilityText(title, TitleTagText)); (In reply to comment #3) > Created an attachment (id=228161) [details] > patch > > Might need some better examples in the test, and/or commented in the source. I can't really determine why we're ever getting helpText from the parentNode, so I'm not sure this is an effective test. One thing that's held me up on this patch is whether we actually want to do this behavior. I am not completely sure we do
James Craig
Comment 5 2014-04-01 18:19:02 PDT
Created attachment 228353 [details] patch Okay, given that this clearly never worked and we don't know why it ever should, I'm pulling out the loop.
chris fleizach
Comment 6 2014-04-02 13:49:56 PDT
Comment on attachment 228353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 > + Node* node = this->node(); I don't think you even need to get node anymore. i don't see it being used
James Craig
Comment 7 2014-04-02 17:12:42 PDT
Comment on attachment 228353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 >> + Node* node = this->node(); > > I don't think you even need to get node anymore. i don't see it being used It's used on the next few lines. 1399 if (!node) 1399 return; 1400 if (node->isElementNode()) {
chris fleizach
Comment 8 2014-04-02 17:13:44 PDT
Comment on attachment 228353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review >>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 >>> + Node* node = this->node(); >> >> I don't think you even need to get node anymore. i don't see it being used > > It's used on the next few lines. > > 1399 if (!node) > 1399 return; > 1400 if (node->isElementNode()) { but it doesn't seem like we need it after that
James Craig
Comment 9 2014-04-02 17:18:18 PDT
Comment on attachment 228353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review >>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 >>>> + Node* node = this->node(); >>> >>> I don't think you even need to get node anymore. i don't see it being used >> >> It's used on the next few lines. >> >> 1399 if (!node) >> 1399 return; >> 1400 if (node->isElementNode()) { > > but it doesn't seem like we need it after that I need it for the conditional. What change are you suggesting? Combining it all on one line like this? if (this->() && this->node->isElementNode()) {
James Craig
Comment 10 2014-04-02 17:18:55 PDT
Comment on attachment 228353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review >>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 >>>>> + Node* node = this->node(); >>>> >>>> I don't think you even need to get node anymore. i don't see it being used >>> >>> It's used on the next few lines. >>> >>> 1399 if (!node) >>> 1399 return; >>> 1400 if (node->isElementNode()) { >> >> but it doesn't seem like we need it after that > > I need it for the conditional. What change are you suggesting? Combining it all on one line like this? > > if (this->() && this->node->isElementNode()) { if (this->node() && this->node()->isElementNode()) {
chris fleizach
Comment 11 2014-04-07 10:08:45 PDT
(In reply to comment #10) > (From update of attachment 228353 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review > > >>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 > >>>>> + Node* node = this->node(); > >>>> > >>>> I don't think you even need to get node anymore. i don't see it being used > >>> > >>> It's used on the next few lines. > >>> > >>> 1399 if (!node) > >>> 1399 return; > >>> 1400 if (node->isElementNode()) { > >> > >> but it doesn't seem like we need it after that > > > > I need it for the conditional. What change are you suggesting? Combining it all on one line like this? > > > > if (this->() && this->node->isElementNode()) { > > if (this->node() && this->node()->isElementNode()) { Yea it looks like we don't need this anymore
James Craig
Comment 12 2014-04-07 18:34:10 PDT
Comment on attachment 228353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review >>>>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397 >>>>>>> + Node* node = this->node(); >>>>>> >>>>>> I don't think you even need to get node anymore. i don't see it being used >>>>> >>>>> It's used on the next few lines. >>>>> >>>>> 1399 if (!node) >>>>> 1399 return; >>>>> 1400 if (node->isElementNode()) { >>>> >>>> but it doesn't seem like we need it after that >>> >>> I need it for the conditional. What change are you suggesting? Combining it all on one line like this? >>> >>> if (this->() && this->node->isElementNode()) { >> >> if (this->node() && this->node()->isElementNode()) { > > Yea it looks like we don't need this anymore Don't we want to ensure the node exists and is an element node before attempting to to call these attribute methods on it?
James Craig
Comment 13 2014-04-08 23:35:08 PDT
Created attachment 228945 [details] patch with review feedback Stylistic change based on review feedback. Avoids extra conditional here (and simplifies code) for the element case even if it causes slightly more execution (conditionals embedded in getAttribute, etc) in the less common, non-element case.
chris fleizach
Comment 14 2014-04-09 16:48:37 PDT
Comment on attachment 228945 [details] patch with review feedback thanks!
WebKit Commit Bot
Comment 15 2014-04-09 17:19:16 PDT
Comment on attachment 228945 [details] patch with review feedback Clearing flags on attachment: 228945 Committed r167054: <http://trac.webkit.org/changeset/167054>
WebKit Commit Bot
Comment 16 2014-04-09 17:19:21 PDT
All reviewed patches have been landed. Closing bug.
Carlos Alberto Lopez Perez
Comment 17 2014-04-10 10:05:21 PDT
The test accessibility/help-text.html introduced here by r167054 is failing on platform GTK ( see bug #131496 )
chris fleizach
Comment 18 2014-04-10 10:09:55 PDT
(In reply to comment #17) > The test accessibility/help-text.html introduced here by r167054 is failing on platform GTK ( see bug #131496 ) Maybe we need a GTK expectation file for differences?
Note You need to log in before you can comment on or make changes to this bug.