[Qt] Large areas highlighted on touch

[Allan Sandfeld Jensen]

In some situations we can still end up highlighting a large area if the document has an onclick handler. 

To avoid all these issues I suggest putting a area limit on non-link, non-form elements with click eventhandlers that can be highlighted.

[Allan Sandfeld Jensen]

Created attachment 164919 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 164919 [details]
Patch

Wrong version uploaded.

[Allan Sandfeld Jensen]

Created attachment 164923 [details]
Patch

[Andras Becsi]

Comment on attachment 164923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164923&action=review

Thanks for fixing, this was really annoying.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1588
> +                if (boxSize.area() > 100000
> +                    && (boxSize.width() > 200 && boxSize.height() > 200))
> +                    activationNode = 0;

Hmm, 100000 feels a bit big to me. Wouldn't this still highlight narrow but long areas? Let's say 200 * 500?
I wonder how to better decide this threshold.

[Kenneth Rohde Christiansen]

Comment on attachment 164923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164923&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1581
> +        if (activationNode && !activationNode->isMouseFocusable() && !activationNode->isContentEditable() && !activationNode->isLink()) {

How to make sure this "list" is always up to date?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1587
> +                if (boxSize.area() > 100000
> +                    && (boxSize.width() > 200 && boxSize.height() > 200))

could be one line or the style guide says to add braces

[Allan Sandfeld Jensen]

(In reply to comment #4)
> (From update of attachment 164923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164923&action=review
> 
> Thanks for fixing, this was really annoying.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1588
> > +                if (boxSize.area() > 100000
> > +                    && (boxSize.width() > 200 && boxSize.height() > 200))
> > +                    activationNode = 0;
> 
> Hmm, 100000 feels a bit big to me. Wouldn't this still highlight narrow but long areas? Let's say 200 * 500?
> I wonder how to better decide this threshold.

I made it quite big on purpose, and the cases it is designed to stop are actually much larger. 100000 does seem big, but it is just 333x333, which is 1x1 inch on a high dpi screen.

(In reply to comment #5)
> (From update of attachment 164923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164923&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1581
> > +        if (activationNode && !activationNode->isMouseFocusable() && !activationNode->isContentEditable() && !activationNode->isLink()) {
> 
> How to make sure this "list" is always up to date?
> 
Hopefully it wouldn't need to be updated. New special shadow elements can be added, but I can't think of any that we would like to highlight, that would be large and neither focusable or a link.

[Kenneth Rohde Christiansen]

Comment on attachment 164923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164923&action=review

>>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1588
>>> +                    activationNode = 0;
>> 
>> Hmm, 100000 feels a bit big to me. Wouldn't this still highlight narrow but long areas? Let's say 200 * 500?
>> I wonder how to better decide this threshold.
> 
> I made it quite big on purpose, and the cases it is designed to stop are actually much larger. 100000 does seem big, but it is just 333x333, which is 1x1 inch on a high dpi screen.
> 
> (In reply to comment #5)

Then tht is a good comment to add. I actually think that one inch is a good size, but you should maybe make the value dependent on device-pixel-ratio.

int oneInchArea...

[Andras Becsi]

Allan, what is the state of this bug?

[Allan Sandfeld Jensen]

(In reply to comment #8)
> Allan, what is the state of this bug?

I haven't updated it to use DPI yet. Mostly because I am not sure 1"x1" is any less arbitrary than 300x300.

[Allan Sandfeld Jensen]

Created attachment 169386 [details]
Alternative Patch

This is an alternative solution to the problem. This restricts the highlighting of scripted event-handlers to inline elements. In theory big areas could still to be highlighted if someone puts the scripted event-handler on an inline-block element that is actually used as normal block element, but that seems extremely unlike.

[Andras Becsi]

(In reply to comment #10)
> Created an attachment (id=169386) [details]
> Alternative Patch
> 
> This is an alternative solution to the problem. This restricts the highlighting of scripted event-handlers to inline elements. In theory big areas could still to be highlighted if someone puts the scripted event-handler on an inline-block element that is actually used as normal block element, but that seems extremely unlike.

This approach works for all the sites I noticed the issue on, and I couldn't reproduce the problem with the patch.
I also like this better because it seems to do the right thing instead of introducing fishy heuristics based on area size.

Looks good to me.

[Simon Hausmann]

Comment on attachment 164923 [details]
Patch

Marking this one as obsolete given r+ on the smaller alternative version :)

[Allan Sandfeld Jensen]

Committed r135280: <http://trac.webkit.org/changeset/135280>