Bug 97216

Created attachment 164919 [details] Patch

Comment on attachment 164919 [details] Patch Wrong version uploaded.

Created attachment 164923 [details] Patch

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.

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

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

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

Allan, what is the state of this bug?

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

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.

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

Comment on attachment 164923 [details] Patch Marking this one as obsolete given r+ on the smaller alternative version :)

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