Bug 108222 - Touch disambiguation blacklist is not being queried properly
Summary: Touch disambiguation blacklist is not being queried properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: dfalcantara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-29 13:28 PST by dfalcantara
Modified: 2013-02-01 00:49 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.49 KB, patch)
2013-01-29 13:35 PST, dfalcantara
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2013-01-29 17:04 PST, dfalcantara
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2013-01-30 15:52 PST, dfalcantara
no flags Details | Formatted Diff | Diff
Patch (5.98 KB, patch)
2013-01-30 15:54 PST, dfalcantara
no flags Details | Formatted Diff | Diff
Patch (6.23 KB, patch)
2013-01-31 13:09 PST, dfalcantara
no flags Details | Formatted Diff | Diff
Patch (8.19 KB, patch)
2013-01-31 15:17 PST, dfalcantara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dfalcantara 2013-01-29 13:28:33 PST
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))
Comment 1 dfalcantara 2013-01-29 13:35:36 PST
Created attachment 185299 [details]
Patch
Comment 2 Tien-Ren Chen 2013-01-29 14:26:59 PST
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;
        }
    }
Comment 3 dfalcantara 2013-01-29 17:04:46 PST
Created attachment 185349 [details]
Patch
Comment 4 Tien-Ren Chen 2013-01-29 17:11:27 PST
LGTM. Now we only need to update the tests.
Comment 5 dfalcantara 2013-01-30 15:52:00 PST
Created attachment 185602 [details]
Patch
Comment 6 dfalcantara 2013-01-30 15:54:48 PST
Created attachment 185604 [details]
Patch
Comment 7 Tien-Ren Chen 2013-01-30 16:30:42 PST
LGTM. Thank you!
Comment 8 Adam Barth 2013-01-31 11:16:48 PST
Comment on attachment 185604 [details]
Patch

Looks great!
Comment 9 WebKit Review Bot 2013-01-31 11:32:47 PST
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
Comment 10 Adam Barth 2013-01-31 12:21:20 PST
    The following ChangeLog files contain OOPS:

        trunk/Source/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
Comment 11 Adam Barth 2013-01-31 12:22:01 PST
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.)
Comment 12 dfalcantara 2013-01-31 13:09:21 PST
Created attachment 185845 [details]
Patch
Comment 13 dfalcantara 2013-01-31 13:12:36 PST
Comment on attachment 185845 [details]
Patch

Take 2...
Comment 14 Alexandre Elias 2013-01-31 14:48:44 PST
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.
Comment 15 dfalcantara 2013-01-31 15:17:48 PST
Created attachment 185878 [details]
Patch
Comment 16 Alexandre Elias 2013-01-31 15:21:24 PST
LGTM, Adam?
Comment 17 WebKit Review Bot 2013-02-01 00:49:39 PST
Comment on attachment 185878 [details]
Patch

Clearing flags on attachment: 185878

Committed r141554: <http://trac.webkit.org/changeset/141554>
Comment 18 WebKit Review Bot 2013-02-01 00:49:43 PST
All reviewed patches have been landed.  Closing bug.