| Summary: | AX: Bug in AccessibilityNodeObject::helpText | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dominic Mazzoni <dmazzoni> | ||||||||
| Component: | Accessibility | Assignee: | 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
Dominic Mazzoni
2014-03-17 23:39:33 PDT
good catch 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.
(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 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.
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 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()) { 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 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()) { 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()) { (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 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? 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.
Comment on attachment 228945 [details]
patch with review feedback
thanks!
Comment on attachment 228945 [details] patch with review feedback Clearing flags on attachment: 228945 Committed r167054: <http://trac.webkit.org/changeset/167054> All reviewed patches have been landed. Closing bug. The test accessibility/help-text.html introduced here by r167054 is failing on platform GTK ( see bug #131496 ) (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? |