Bug 108222 – Touch disambiguation blacklist is not being queried properly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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

[Tien-Ren Chen]

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

[Tien-Ren Chen]

LGTM. Now we only need to update the tests.

Created attachment 185602 [details]
Patch

Created attachment 185604 [details]
Patch

[Tien-Ren Chen]

LGTM. Thank you!

[Adam Barth]

Comment on attachment 185604 [details]
Patch

Looks great!

[WebKit Review Bot]

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

[Adam Barth]

    The following ChangeLog files contain OOPS:

        trunk/Source/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.

[Adam Barth]

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...

[Alexandre Elias]

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

[Alexandre Elias]

LGTM, Adam?

[WebKit Review Bot]

Comment on attachment 185878 [details]
Patch

Clearing flags on attachment: 185878

Committed r141554: <http://trac.webkit.org/changeset/141554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.