101369 2012-11-06 10:30:23 -0800 AX: Textfields don't get focus when navigated to from 'show all tabs' button 2012-11-07 11:24:41 -0800 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) Unspecified OS X 10.8 RESOLVED FIXED InRadar P2 Normal --- 1 cfleizach cfleizach apinheiro darin dmazzoni jdiggs webkit.review.bot oldest_to_newest 759836 0 cfleizach 2012-11-06 10:30:23 -0800 When going from the tabs to HTML content with a text field, the keyboard focus remains on a previous element. You can't type in the field when it's not focused. 759838 1 cfleizach 2012-11-06 10:30:39 -0800 rdar://12263874 759843 2 172614 cfleizach 2012-11-06 10:35:20 -0800 Created attachment 172614 patch 759859 3 172614 darin 2012-11-06 10:50:39 -0800 Comment on attachment 172614 patch View in context: https://bugs.webkit.org/attachment.cgi?id=172614&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1506 > + Document* document = this->document(); > if (!on) > - m_renderer->document()->setFocusedNode(0); > + document->setFocusedNode(0); > else { > - if (m_renderer->node()->isElementNode()) > - static_cast<Element*>(m_renderer->node())->focus(); > + Node* node = this->node(); > + // If this node is already the currently focused node, then calling focus() won't do anything. > + // That is a problem when focus is removed from the webpage to chrome, and then returns. > + // In these cases, we need to do what keyboard and mouse focus do, which is reset focus first. > + if (document->focusedNode() == node) > + document->setFocusedNode(0); > + > + if (node && node->isElementNode()) > + toElement(node)->focus(); > else > - m_renderer->document()->setFocusedNode(m_renderer->node()); > + document->setFocusedNode(node); > } Is there some way to share this code rather than repeating this logic here? 759877 4 cfleizach 2012-11-06 10:59:58 -0800 (In reply to comment #3) > (From update of attachment 172614 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172614&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1506 > > + Document* document = this->document(); > > if (!on) > > - m_renderer->document()->setFocusedNode(0); > > + document->setFocusedNode(0); > > else { > > - if (m_renderer->node()->isElementNode()) > > - static_cast<Element*>(m_renderer->node())->focus(); > > + Node* node = this->node(); > > + // If this node is already the currently focused node, then calling focus() won't do anything. > > + // That is a problem when focus is removed from the webpage to chrome, and then returns. > > + // In these cases, we need to do what keyboard and mouse focus do, which is reset focus first. > > + if (document->focusedNode() == node) > > + document->setFocusedNode(0); > > + > > + if (node && node->isElementNode()) > > + toElement(node)->focus(); > > else > > - m_renderer->document()->setFocusedNode(m_renderer->node()); > > + document->setFocusedNode(node); > > } > > Is there some way to share this code rather than repeating this logic here? I couldn't think of a way to do that easily without altering long-standing behavior. What I thought might be reasonable would be to call setFocusedNode(0) when the WebPage loses focus, but I wasn't sure of the implications of doing that. The document's notion of what is focused would be lost. Maybe that's not important, but it was hard to know. The keyboard tabbing method always assumes that focus will be on the first focusable item when it sets focus. The mouse method is tied up heavily in normal mouse event handling. The addition here (minus the refactoring) are just these two lines >> + if (document->focusedNode() == node) > > + document->setFocusedNode(0); so it's not a lot of duplication 759884 5 172614 darin 2012-11-06 11:07:41 -0800 Comment on attachment 172614 patch View in context: https://bugs.webkit.org/attachment.cgi?id=172614&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1500 > + if (document->focusedNode() == node) > + document->setFocusedNode(0); Is this needed for the non-element case, or only for the element case? 759918 6 cfleizach 2012-11-06 11:33:57 -0800 (In reply to comment #5) > (From update of attachment 172614 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172614&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1500 > > + if (document->focusedNode() == node) > > + document->setFocusedNode(0); > > Is this needed for the non-element case, or only for the element case? I think this would only ever be an issue in the element case. I can limit it to that 759924 7 172626 cfleizach 2012-11-06 11:39:30 -0800 Created attachment 172626 patch patch that only applies this logic to Element types (instead of all Nodes) 760816 8 172626 darin 2012-11-07 10:04:41 -0800 Comment on attachment 172626 patch OK, I can live with this, but there’s something about this that seems wrong. I don’t see why there’s no function on node to do this whole dance. 760909 9 172626 webkit.review.bot 2012-11-07 11:24:37 -0800 Comment on attachment 172626 patch Clearing flags on attachment: 172626 Committed r133785: <http://trac.webkit.org/changeset/133785> 760910 10 webkit.review.bot 2012-11-07 11:24:41 -0800 All reviewed patches have been landed. Closing bug. 172614 2012-11-06 10:35:20 -0800 2012-11-06 11:39:30 -0800 patch patch text/plain 2638 cfleizach SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMzYzNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDEyLTExLTA2ICBDaHJpcyBG bGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IFRleHRmaWVsZHMg ZG9uJ3QgZ2V0IGZvY3VzIHdoZW4gbmF2aWdhdGVkIHRvIGZyb20gJ3Nob3cgYWxsIHRhYnMnIGJ1 dHRvbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAx MzY5CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hl biBmb2N1cyBsZWF2ZXMgYSB3ZWJwYWdlLCB0aGUgd2VicGFnZSBkb2VzIG5vdCBub3JtYWxseSBj bGVhciBpdHMgZm9jdXNlZCBub2RlLiBXaGVuIGZvY3VzCisgICAgICAgIHJldHVybnMgKHRocm91 Z2ggdGFiLCBvciBtb3VzZSBjbGljayksIHRoZW4gZm9jdXMgd2lsbCBiZSBjbGVhcmVkIGFuZCBy ZXN0b3JlZC4KKworICAgICAgICBIb3dldmVyLCB3aGVuIGZvY3VzIGlzIHJldHVybmVkIHRocm91 Z2ggYWNjZXNzaWJpbGl0eSBtZWFucywgZm9jdXMgaXMgbm90IGNsZWFyZWQgZmlyc3QsIHdoaWNo CisgICAgICAgIG1lYW5zIHN1YnNlcXVlbnQgZm9jdXMgYWN0aW9ucyBkbyBub3RoaW5nLgorCisg ICAgICAgIFRoaXMgZml4IHdpbGwgY2hlY2sgaWYgdGhlIGZvY3VzZWQgbm9kZSBpcyB0aGUgc2Ft ZSBhcyB0aGUgbm9kZSBiZWluZyBmb2N1c2VkLCBhbmQgaWYgc28sIGNsZWFyIHRoZSBmb2N1cyBm aXJzdC4KKworICAgICAgICBNYWtpbmcgYSBsYXlvdXQgdGVzdCBmb3IgdGhpcyBpcyBub3QgcG9z c2libGUgYmVjYXVzZSBpdCByZXF1aXJlcyBzZXR0aW5nIGZvY3VzIHRvIHNvbWV0aGluZyBpbiB0 aGUgY2hyb21lLCBhbmQgdGhlbgorICAgICAgICBtb3ZpbmcgZm9jdXMgYmFjayB0byB0aGUgd2Vi cGFnZS4KKyAgICAgIAorICAgICAgICAqIGFjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eVJlbmRl ck9iamVjdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpBY2Nlc3NpYmlsaXR5UmVuZGVyT2JqZWN0 OjpzZXRGb2N1c2VkKToKKwogMjAxMi0xMS0wNiAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQu b3JnPgogCiAgICAgICAgIFNjcmlwdFdyYXBwYWJsZSBzaG91bGQgd29yayBmb3IgbW9yZSB0aGFu IGp1c3QgTm9kZQpJbmRleDogU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmls aXR5UmVuZGVyT2JqZWN0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9hY2Nlc3Np YmlsaXR5L0FjY2Vzc2liaWxpdHlSZW5kZXJPYmplY3QuY3BwCShyZXZpc2lvbiAxMzI0MjEpCisr KyBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlSZW5kZXJPYmplY3Qu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xNDg4LDEzICsxNDg4LDIxIEBACiAgICAgaWYgKCFjYW5T ZXRGb2N1c0F0dHJpYnV0ZSgpKQogICAgICAgICByZXR1cm47CiAgICAgCisgICAgRG9jdW1lbnQq IGRvY3VtZW50ID0gdGhpcy0+ZG9jdW1lbnQoKTsKICAgICBpZiAoIW9uKQotICAgICAgICBtX3Jl bmRlcmVyLT5kb2N1bWVudCgpLT5zZXRGb2N1c2VkTm9kZSgwKTsKKyAgICAgICAgZG9jdW1lbnQt PnNldEZvY3VzZWROb2RlKDApOwogICAgIGVsc2UgewotICAgICAgICBpZiAobV9yZW5kZXJlci0+ bm9kZSgpLT5pc0VsZW1lbnROb2RlKCkpCi0gICAgICAgICAgICBzdGF0aWNfY2FzdDxFbGVtZW50 Kj4obV9yZW5kZXJlci0+bm9kZSgpKS0+Zm9jdXMoKTsKKyAgICAgICAgTm9kZSogbm9kZSA9IHRo aXMtPm5vZGUoKTsKKyAgICAgICAgLy8gSWYgdGhpcyBub2RlIGlzIGFscmVhZHkgdGhlIGN1cnJl bnRseSBmb2N1c2VkIG5vZGUsIHRoZW4gY2FsbGluZyBmb2N1cygpIHdvbid0IGRvIGFueXRoaW5n LgorICAgICAgICAvLyBUaGF0IGlzIGEgcHJvYmxlbSB3aGVuIGZvY3VzIGlzIHJlbW92ZWQgZnJv bSB0aGUgd2VicGFnZSB0byBjaHJvbWUsIGFuZCB0aGVuIHJldHVybnMuCisgICAgICAgIC8vIElu IHRoZXNlIGNhc2VzLCB3ZSBuZWVkIHRvIGRvIHdoYXQga2V5Ym9hcmQgYW5kIG1vdXNlIGZvY3Vz IGRvLCB3aGljaCBpcyByZXNldCBmb2N1cyBmaXJzdC4KKyAgICAgICAgaWYgKGRvY3VtZW50LT5m b2N1c2VkTm9kZSgpID09IG5vZGUpCisgICAgICAgICAgICBkb2N1bWVudC0+c2V0Rm9jdXNlZE5v ZGUoMCk7CisgICAgICAgIAorICAgICAgICBpZiAobm9kZSAmJiBub2RlLT5pc0VsZW1lbnROb2Rl KCkpCisgICAgICAgICAgICB0b0VsZW1lbnQobm9kZSktPmZvY3VzKCk7CiAgICAgICAgIGVsc2UK LSAgICAgICAgICAgIG1fcmVuZGVyZXItPmRvY3VtZW50KCktPnNldEZvY3VzZWROb2RlKG1fcmVu ZGVyZXItPm5vZGUoKSk7CisgICAgICAgICAgICBkb2N1bWVudC0+c2V0Rm9jdXNlZE5vZGUobm9k ZSk7CiAgICAgfQogfQogCg== 172626 2012-11-06 11:39:30 -0800 2012-11-07 11:24:37 -0800 patch patch text/plain 2680 cfleizach SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMzYzNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDEyLTExLTA2ICBDaHJpcyBG bGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IFRleHRmaWVsZHMg ZG9uJ3QgZ2V0IGZvY3VzIHdoZW4gbmF2aWdhdGVkIHRvIGZyb20gJ3Nob3cgYWxsIHRhYnMnIGJ1 dHRvbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAx MzY5CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hl biBmb2N1cyBsZWF2ZXMgYSB3ZWJwYWdlLCB0aGUgd2VicGFnZSBkb2VzIG5vdCBub3JtYWxseSBj bGVhciBpdHMgZm9jdXNlZCBub2RlLiBXaGVuIGZvY3VzCisgICAgICAgIHJldHVybnMgKHRocm91 Z2ggdGFiLCBvciBtb3VzZSBjbGljayksIHRoZW4gZm9jdXMgd2lsbCBiZSBjbGVhcmVkIGFuZCBy ZXN0b3JlZC4KKworICAgICAgICBIb3dldmVyLCB3aGVuIGZvY3VzIGlzIHJldHVybmVkIHRocm91 Z2ggYWNjZXNzaWJpbGl0eSBtZWFucywgZm9jdXMgaXMgbm90IGNsZWFyZWQgZmlyc3QsIHdoaWNo CisgICAgICAgIG1lYW5zIHN1YnNlcXVlbnQgZm9jdXMgYWN0aW9ucyBkbyBub3RoaW5nLgorCisg ICAgICAgIFRoaXMgZml4IHdpbGwgY2hlY2sgaWYgdGhlIGZvY3VzZWQgbm9kZSBpcyB0aGUgc2Ft ZSBhcyB0aGUgbm9kZSBiZWluZyBmb2N1c2VkLCBhbmQgaWYgc28sIGNsZWFyIHRoZSBmb2N1cyBm aXJzdC4KKworICAgICAgICBNYWtpbmcgYSBsYXlvdXQgdGVzdCBmb3IgdGhpcyBpcyBub3QgcG9z c2libGUgYmVjYXVzZSBpdCByZXF1aXJlcyBzZXR0aW5nIGZvY3VzIHRvIHNvbWV0aGluZyBpbiB0 aGUgY2hyb21lLCBhbmQgdGhlbgorICAgICAgICBtb3ZpbmcgZm9jdXMgYmFjayB0byB0aGUgd2Vi cGFnZS4KKyAgICAgIAorICAgICAgICAqIGFjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eVJlbmRl ck9iamVjdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpBY2Nlc3NpYmlsaXR5UmVuZGVyT2JqZWN0 OjpzZXRGb2N1c2VkKToKKwogMjAxMi0xMS0wNiAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQu b3JnPgogCiAgICAgICAgIFNjcmlwdFdyYXBwYWJsZSBzaG91bGQgd29yayBmb3IgbW9yZSB0aGFu IGp1c3QgTm9kZQpJbmRleDogU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmls aXR5UmVuZGVyT2JqZWN0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9hY2Nlc3Np YmlsaXR5L0FjY2Vzc2liaWxpdHlSZW5kZXJPYmplY3QuY3BwCShyZXZpc2lvbiAxMzI0MjEpCisr KyBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlSZW5kZXJPYmplY3Qu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xNDg4LDEzICsxNDg4LDIxIEBACiAgICAgaWYgKCFjYW5T ZXRGb2N1c0F0dHJpYnV0ZSgpKQogICAgICAgICByZXR1cm47CiAgICAgCisgICAgRG9jdW1lbnQq IGRvY3VtZW50ID0gdGhpcy0+ZG9jdW1lbnQoKTsKICAgICBpZiAoIW9uKQotICAgICAgICBtX3Jl bmRlcmVyLT5kb2N1bWVudCgpLT5zZXRGb2N1c2VkTm9kZSgwKTsKKyAgICAgICAgZG9jdW1lbnQt PnNldEZvY3VzZWROb2RlKDApOwogICAgIGVsc2UgewotICAgICAgICBpZiAobV9yZW5kZXJlci0+ bm9kZSgpLT5pc0VsZW1lbnROb2RlKCkpCi0gICAgICAgICAgICBzdGF0aWNfY2FzdDxFbGVtZW50 Kj4obV9yZW5kZXJlci0+bm9kZSgpKS0+Zm9jdXMoKTsKLSAgICAgICAgZWxzZQotICAgICAgICAg ICAgbV9yZW5kZXJlci0+ZG9jdW1lbnQoKS0+c2V0Rm9jdXNlZE5vZGUobV9yZW5kZXJlci0+bm9k ZSgpKTsKKyAgICAgICAgTm9kZSogbm9kZSA9IHRoaXMtPm5vZGUoKTsKKyAgICAgICAgaWYgKG5v ZGUgJiYgbm9kZS0+aXNFbGVtZW50Tm9kZSgpKSB7CisgICAgICAgICAgICAvLyBJZiB0aGlzIG5v ZGUgaXMgYWxyZWFkeSB0aGUgY3VycmVudGx5IGZvY3VzZWQgbm9kZSwgdGhlbiBjYWxsaW5nIGZv Y3VzKCkgd29uJ3QgZG8gYW55dGhpbmcuCisgICAgICAgICAgICAvLyBUaGF0IGlzIGEgcHJvYmxl bSB3aGVuIGZvY3VzIGlzIHJlbW92ZWQgZnJvbSB0aGUgd2VicGFnZSB0byBjaHJvbWUsIGFuZCB0 aGVuIHJldHVybnMuCisgICAgICAgICAgICAvLyBJbiB0aGVzZSBjYXNlcywgd2UgbmVlZCB0byBk byB3aGF0IGtleWJvYXJkIGFuZCBtb3VzZSBmb2N1cyBkbywgd2hpY2ggaXMgcmVzZXQgZm9jdXMg Zmlyc3QuCisgICAgICAgICAgICBpZiAoZG9jdW1lbnQtPmZvY3VzZWROb2RlKCkgPT0gbm9kZSkK KyAgICAgICAgICAgICAgICBkb2N1bWVudC0+c2V0Rm9jdXNlZE5vZGUoMCk7CisgICAgICAgICAg ICAKKyAgICAgICAgICAgIHRvRWxlbWVudChub2RlKS0+Zm9jdXMoKTsKKyAgICAgICAgfSBlbHNl CisgICAgICAgICAgICBkb2N1bWVudC0+c2V0Rm9jdXNlZE5vZGUobm9kZSk7CiAgICAgfQogfQog Cg==