Summary: | Touch disambiguation blacklist is not being queried properly | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | dfalcantara | ||||||||||||||
Component: | WebCore Misc. | Assignee: | dfalcantara | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, aelias, trchen, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
dfalcantara
2013-01-29 13:28:33 PST
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. |