Bug 94355

OK, we may need this to use on Android in the short term, as page scale is currently a transform. What are the cases where scale is not applied correctly?

(In reply to comment #1) > OK, we may need this to use on Android in the short term, as page scale is currently a transform. What are the cases where scale is not applied correctly? This patch will land shortly after 84487 ... we just need to update so we transform quad points instead of a rect. Right now applying sclaeX or scaleY may cause the highlight to be slightly off in it's positioning.

OK, thanks. It's fine to land the other one first, just so long as we follow up on this.

(In reply to comment #3) > OK, thanks. It's fine to land the other one first, just so long as we follow up on this. This should be done *very soon* after the first one lands :-)

Created attachment 160783 [details] Patch This CL extends the current link highlighter to account for transformed targets. Transformed links will be represented by a rectilinear bounding box (at present), but transformed divs will be highlighted according to their borders. This CL also improves path specificity by using the quad list inside the target, as opposed to the target's bounding box.

Comment on attachment 160783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160783&action=review Close - I think it needs a bit more polish, though. > Source/WebKit/chromium/src/LinkHighlight.cpp:144 > +static void convertPointsTargetToCompositedLayer(const Vector<IntPoint>& vector, RenderObject* targetRenderer, RenderObject* compositedRenderer, Vector<FloatPoint>& outVector) vector / outVector aren't great variable names - can you name them for the data they represent? > Source/WebKit/chromium/src/LinkHighlight.cpp:161 > +static void addQuadToPath(const Vector<FloatPoint>&quad, Path& path) space between & and quad > Source/WebKit/chromium/src/LinkHighlight.cpp:163 > + // FIXME: Make this create rounded rectangles, just like the axis-aligned case. Path has an addPathForRoundedRect, if it's useful > Source/WebKit/chromium/src/LinkHighlight.cpp:176 > + // FIXME: find way to compare highlight paths, or get rid of the return value for this function. SkPath has an operator==. I think you should resolve this fixme one way or the other in this patch - there's no real value in leaving this in such a weird state when all changes will be internal to this file. > Source/WebKit/chromium/src/LinkHighlight.cpp:192 > + boundingQuad.append(roundedIntPoint(quads[quadIndex].p1())); rounded points aren't necessarily bounding, so either the code or the variable name is wrong here. Do you want FloatQuad::enclosingBoundingBox(), perhaps?

Created attachment 163237 [details] Patch (In reply to comment #6) > (From update of attachment 160783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160783&action=review > > Close - I think it needs a bit more polish, though. > > > Source/WebKit/chromium/src/LinkHighlight.cpp:144 > > +static void convertPointsTargetToCompositedLayer(const Vector<IntPoint>& vector, RenderObject* targetRenderer, RenderObject* compositedRenderer, Vector<FloatPoint>& outVector) > > vector / outVector aren't great variable names - can you name them for the data they represent? Fixed. > > Source/WebKit/chromium/src/LinkHighlight.cpp:161 > > +static void addQuadToPath(const Vector<FloatPoint>&quad, Path& path) > > space between & and quad Fixed. > > Source/WebKit/chromium/src/LinkHighlight.cpp:163 > > + // FIXME: Make this create rounded rectangles, just like the axis-aligned case. > > Path has an addPathForRoundedRect, if it's useful Hmmm, I don't think it helps ... it takes a rect as input, and not a quad, so it seems it only helps with axis-aligned quads. > > Source/WebKit/chromium/src/LinkHighlight.cpp:176 > > + // FIXME: find way to compare highlight paths, or get rid of the return value for this function. > > SkPath has an operator==. I think you should resolve this fixme one way or the other in this patch - there's no real value in leaving this in such a weird state when all changes will be internal to this file. Had to wait for a Skia patch to land before I could follow up with this patch. The == operator is used within skia, and based on initial testing seems to do what we want. > > Source/WebKit/chromium/src/LinkHighlight.cpp:192 > > + boundingQuad.append(roundedIntPoint(quads[quadIndex].p1())); > > rounded points aren't necessarily bounding, so either the code or the variable name is wrong here. Do you want FloatQuad::enclosingBoundingBox(), perhaps? It's not critical that it be a bounding box, and it keeps the code simpler to just use rounded points, so for now I'm just using the rounded points and have renamed the variable. I've also remove the ZIndex test, as it seems that applying a 2-D rotation kills the auto zIndex property of the target, thus rendering rotated divs non-highlightable.

Comment on attachment 163237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163237&action=review > Source/WebKit/chromium/src/LinkHighlight.cpp:146 > +static void convertPointsTargetToCompositedLayer(const Vector<IntPoint>& targetSpacePoints, RenderObject* targetRenderer, RenderObject* compositedRenderer, Vector<FloatPoint>& compositedSpacePoints) Do you need to use so many vectors here? Why not input a FloatQuad (and round it here) and output a FloatQuad? > Source/WebKit/chromium/src/LinkHighlight.cpp:207 > + if (quads.size() == 1 && quad.isRectilinear()) { > + FloatSize rectRoundingRadii(3, 3); > + newPath.addRoundedRect(quad.boundingBox(), rectRoundingRadii); > + } else > + addQuadToPath(transformedQuad, newPath); Do the existing tests hit both of these cases? > Source/WebKit/chromium/src/WebViewImpl.cpp:1166 > // If the document has click handlers installed, we don't want to default to applying the highlight to the entire RenderView, or the > // entire body. Also, if the node has non-auto Z-index, we cannot be sure of it's ordering with respect to other possible target nodes. > RenderObject* touchNodeRenderer = bestTouchNode ? bestTouchNode->renderer() : 0; > - if (bestTouchNode && (!touchNodeRenderer || touchNodeRenderer->isRenderView() || touchNodeRenderer->isBody() || !touchNodeRenderer->style()->hasAutoZIndex())) > + if (bestTouchNode && (!touchNodeRenderer || touchNodeRenderer->isRenderView() || touchNodeRenderer->isBody())) I don't understand this change or why the comment has been left unchanged. Also, s/it's/its/.

Comment on attachment 163237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163237&action=review >> Source/WebKit/chromium/src/LinkHighlight.cpp:146 >> +static void convertPointsTargetToCompositedLayer(const Vector<IntPoint>& targetSpacePoints, RenderObject* targetRenderer, RenderObject* compositedRenderer, Vector<FloatPoint>& compositedSpacePoints) > > Do you need to use so many vectors here? Why not input a FloatQuad (and round it here) and output a FloatQuad? Originally I had expected we'd need to transform arbitrary polygons, and there might be some benefit to doing it this way. Also, it irks me to no end that FloatPoint doesn't have an index operator ;-) Vectors removed. >> Source/WebKit/chromium/src/LinkHighlight.cpp:207 >> + addQuadToPath(transformedQuad, newPath); > > Do the existing tests hit both of these cases? Sorry, no; will add a multi-quad test in new patch. >> Source/WebKit/chromium/src/WebViewImpl.cpp:1166 >> + if (bestTouchNode && (!touchNodeRenderer || touchNodeRenderer->isRenderView() || touchNodeRenderer->isBody())) > > I don't understand this change or why the comment has been left unchanged. Also, s/it's/its/. The comment should have been updated (strange about "it's", I don't usually mix those up ;-) ). We had originally opted out of highlighting any node with non-auto Z-index, primarily to match what Clank is doing. However, it seems this is not necessary, and it interferes with properly highlighting rotated elements.

Created attachment 166988 [details] Patch

Comment on attachment 166988 [details] Patch Why are all the tests in platform/chromium-linux?

(In reply to comment #11) > (From update of attachment 166988 [details]) > Why are all the tests in platform/chromium-linux? At present this is only turned on for ChromeOS. The plan is to get the behaviour right, then turn it on for Win8 as well, at which time we will re-structure the same tests to run on both. Right now, this is disabled by default on ChromeOS and enabled via a flag.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 166988 [details] [details]) > > Why are all the tests in platform/chromium-linux? > > At present this is only turned on for ChromeOS. The plan is to get the behaviour right, then turn it on for Win8 as well, at which time we will re-structure the same tests to run on both. > > Right now, this is disabled by default on ChromeOS and enabled via a flag. Huh. Where is this flag set? Why would this test not run on other platforms?

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 166988 [details] [details] [details]) > > > Why are all the tests in platform/chromium-linux? > > > > At present this is only turned on for ChromeOS. The plan is to get the behaviour right, then turn it on for Win8 as well, at which time we will re-structure the same tests to run on both. > > > > Right now, this is disabled by default on ChromeOS and enabled via a flag. > > Huh. Where is this flag set? Why would this test not run on other platforms? It's set via compiler define in WebViewImpl::handleGestureEvent(), and also requires --enable-touch-events (since otherwise GestureTapDown won't be sent). The tests should in theory run elsewhere, but during the dogfood phase we chose to just run them on Linux. The question of where the tests run is a little moot at present, since they got turned off (temporarily) in https://bugs.webkit.org/show_bug.cgi?id=96951, although the root cause doesn't seem to be the highlighting code based on our investigations thus far.

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 166988 [details] [details] [details]) > > > Why are all the tests in platform/chromium-linux? > > > > At present this is only turned on for ChromeOS. The plan is to get the behaviour right, then turn it on for Win8 as well, at which time we will re-structure the same tests to run on both. > > > > Right now, this is disabled by default on ChromeOS and enabled via a flag. > > Huh. Where is this flag set? Why would this test not run on other platforms? Oh, and it requires --enable-gesture-tap-highlight too ...

(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #12) > > > (In reply to comment #11) > > > > (From update of attachment 166988 [details] [details] [details] [details]) > > > > Why are all the tests in platform/chromium-linux? > > > > > > At present this is only turned on for ChromeOS. The plan is to get the behaviour right, then turn it on for Win8 as well, at which time we will re-structure the same tests to run on both. > > > > > > Right now, this is disabled by default on ChromeOS and enabled via a flag. > > > > Huh. Where is this flag set? Why would this test not run on other platforms? > > Oh, and it requires --enable-gesture-tap-highlight too ... Where is this set for the tests? I guess it's just weird for me to see something that's not really platform-specific have tests in a platform-specific directory.

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > (In reply to comment #12) > > > > (In reply to comment #11) > > > > > (From update of attachment 166988 [details] [details] [details] [details] [details]) > > > > > Why are all the tests in platform/chromium-linux? > > > > > > > > At present this is only turned on for ChromeOS. The plan is to get the behaviour right, then turn it on for Win8 as well, at which time we will re-structure the same tests to run on both. > > > > > > > > Right now, this is disabled by default on ChromeOS and enabled via a flag. > > > > > > Huh. Where is this flag set? Why would this test not run on other platforms? > > > > Oh, and it requires --enable-gesture-tap-highlight too ... > > Where is this set for the tests? Oh, sorry ... thought we were talking about the end-user. The tests only run on Linux due to the conditional compile on the call to enableTouchHighlight from WebViewImpl::handleGestureEvent(). WebKit turns on highlights by default, but the chrome browser overrides this and turns them off by default. So for DRT they are on by default (but would fail on non-Linux bots due to enableTouchHighlight not being called). > I guess it's just weird for me to see something that's not really platform-specific have tests in a platform-specific directory. It is a little weird, and once we're past dogfood I would like to broaden the tests so all platforms run them, though I guess I need to get them re-enabled on Linux first :-)

Comment on attachment 166988 [details] Patch R=me. Ok, sure.

Created attachment 167363 [details] Patch for landing

Comment on attachment 167363 [details] Patch for landing Clearing flags on attachment: 167363 Committed r130551: <http://trac.webkit.org/changeset/130551>

All reviewed patches have been landed. Closing bug.