In TouchDisambiguation.cpp, a blacklist is built to prevent containers from counting as legitimate touch targets. However, it is not being queried properly. I believe the issue is in this block of code: 121 HashMap<Node*, TouchTargetData> touchTargets; 122 float bestScore = 0; 123 for (ListHashSet<RefPtr<Node> >::const_iterator it = hitResults.begin(); it != hitResults.end(); ++it) { 124 for (Node* node = it->get(); node; node = node->parentNode()) { 125 if (blackList.contains(it->get())) 126 continue; 127 if (node->isDocumentNode() || node->hasTagName(HTMLNames::htmlTag) || node->hasTagName(HTMLNames::bodyTag)) 128 break; 129 if (node->willRespondToMouseClickEvents()) { 130 TouchTargetData& targetData = touchTargets.add(node, TouchTargetData()).iterator->value; 131 targetData.windowBoundingBox = boundingBoxForEventNodes(node); 132 targetData.score = scoreTouchTarget(touchPoint, touchPointPadding, targetData.windowBoundingBox); 133 bestScore = max(bestScore, targetData.score); 134 break; 135 } 136 } 137 } Line 125 should be checking if the current node is part of the blacklist, not its parent: if (blackList.contains(node))
Created attachment 185299 [details] Patch
Good catch! Great thanks for figuring out the mess. I think we need to also change the blacklisting loop to handle this case: <div onclick="foo" class="outer"> <div onclick="foo" class="middle"><div class="inner">Link1</div></div> <div onclick="foo" class="middle"><div class="inner">Link2</div></div> </div> In this case the hit test will return both the inner <div> block, and we'll end up blacklist the <div> in the middle. HashSet<Node*> blackList; for (ListHashSet<RefPtr<Node> >::const_iterator it = hitResults.begin(); it != hitResults.end(); ++it) { RenderObject* renderer = it->get()->renderer(); if (!renderer) continue; >>>> Should traverse up until we find the actual touch target before we start to blacklist stuff. for (RenderBlock* container = renderer->containingBlock(); container; container = container->containingBlock()) { Node* containerNode = container->node(); if (!containerNode) continue; if (!blackList.add(containerNode).isNewEntry) break; } }
Created attachment 185349 [details] Patch
LGTM. Now we only need to update the tests.
Created attachment 185602 [details] Patch
Created attachment 185604 [details] Patch
LGTM. Thank you!
Comment on attachment 185604 [details] Patch Looks great!
Comment on attachment 185604 [details] Patch Rejecting attachment 185604 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 185604, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: mit-queue/Source/WebKit/chromium/third_party/skia/src --revision 7453 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 51>At revision 7453. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/16272316
The following ChangeLog files contain OOPS: trunk/Source/WebCore/ChangeLog Please don't ever say "OOPS" in a ChangeLog file.
Comment on attachment 185604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185604&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). You should fill out this line to list the tests. (The OOPS on the "reviewed by" line is fine---the bots will fill that one out for you.)
Created attachment 185845 [details] Patch
Comment on attachment 185845 [details] Patch Take 2...
Comment on attachment 185845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185845&action=review > Source/WebKit/chromium/tests/WebFrameTest.cpp:1911 > + webViewImpl->resize(WebSize(viewportWidth, viewportHeight)); Please add setApplyPageScaleFactorInCompositor and setApplyDeviceScaleFactorInCompositor here like the other tests, and verify it still works.
Created attachment 185878 [details] Patch
LGTM, Adam?
Comment on attachment 185878 [details] Patch Clearing flags on attachment: 185878 Committed r141554: <http://trac.webkit.org/changeset/141554>
All reviewed patches have been landed. Closing bug.