Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
402603005:
Disable touch highlight when the touched node has no hand-cursor. (Closed)
DescriptionDisable touch highlight when the touched node has no hand-cursor.
Before these change, a missing hand-cursor at the touched node
highlighted the nearest ancestor node in DOM that has such a cursor.
This is not expected since the touched node may be visually detached
from the ancestor.
BUG=322323
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179863
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added checks for explicit non-hand cursor in an ancestor node. #Patch Set 3 : #Patch Set 4 : Tweaked the last heiristic for the test cases. #Patch Set 5 : #
Total comments: 2
Patch Set 6 : Added 2 new tests. #
Total comments: 14
Patch Set 7 : #Patch Set 8 : Fixed logic with all tests passing. #
Total comments: 18
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : Added another tests, handled image maps inside or outside of anchors. #
Total comments: 10
Patch Set 13 : #Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : Reverted back to last working logic #
Total comments: 2
Messages
Total messages: 32 (0 generated)
https://codereview.chromium.org/402603005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1197: // The heuristic we use is: show a highlight on tap only when we are at a node with a hand cursor I don't think this is quite what we want. If the node has a hand cursor, then we want to walk up the tree and find the node that invokes that hand cursor and use that for the highlight. So we still need to the treewalk, but we need to somehow exclude nodes where the cursor has been explicitly reset to something else.
lgtm with 1 nit https://codereview.chromium.org/402603005/diff/80001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/80001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1225: bestTouchNode = cursorDefiningAncestor; The logic may be correct, but the back-and-forth between bestTouchNode and cursorDefiningAncestor is confusing. Can you clean this up a bit?
Please also add a new layout test case that represents the issue you're fixing (i.e. it should fail without your change applied and pass with it applied).
On 2014/07/22 21:10:29, Rick Byers wrote: > Please also add a new layout test case that represents the issue you're fixing > (i.e. it should fail without your change applied and pass with it applied). Added two layout tests: the first one fails with the old heuristic, while the second one tests jumping out of a node w/o an explicit cursor.
https://codereview.chromium.org/402603005/diff/80001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/80001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1225: bestTouchNode = cursorDefiningAncestor; On 2014/07/22 20:34:12, wjmaclean wrote: > The logic may be correct, but the back-and-forth between bestTouchNode and > cursorDefiningAncestor is confusing. Can you clean this up a bit? Done.
https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html (right): https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:2: <html> Note that someday we should perhaps update all these link highlighting tests to match our layout test style (getting written down here: http://www.chromium.org/blink/coding-style/layout-test-style-guidelines), but for now I'm fine with new link highlight tests matching the style of the existing ones. https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:15: none All this explicit positioning makes this test hard to follow. I'd suggest simplifying it into something like this (see http://jsbin.com/docuq/1/edit): <style> #test { width: 150px; } #test, #test div { padding: 15px; border-style: solid; } </style> <div id=test class=opaqueHighlight style="cursor: pointer;"> <div style="cursor: pointer;"> <div style="cursor: default;"> <div id=targetDiv></div> </div> </div> </div> https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:20: <div style="position: absolute; left: 10px; top: 250px; -webkit-transform: translateZ(0);"> Does this div do anything? Can you just remove it? https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:25: var clientRect = document.getElementById('targetDiv').getBoundingClientRect(); Note that you should be careful to target the div you really want to click on. This looks like it works by your inner div being in the middle of your outer-most one, but better to put the targetDiv label on the innermost div. When I'm feeling paranoid I'll often also do a double-check in the test like: 'shouldBe('targetDiv', document.elementFromPoint(x, y).id)'. I've seen tests get checked in that pass unconditionally (rather than actually test what they intended to test) due to little bugs around stuff like this. Of course you should also run your tests on a build without your fix to verify it fails properly. https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1175: // If the current node has an attached renderer, check the cursor. Otherwise, go up. I'd omit this comment - the code says this very clearly. https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1178: if (cursor != CURSOR_AUTO || frame->eventHandler().useHandCursor(node, node->isLink())) I don't think this is right. If a node explicitly sets the cursor to 'auto' then that should also prevent highlight. Eg. try this test case http://jsbin.com/mojole/1/edit. I think I've led you astray on this CL - I'm sorry. I've just done a bunch of digging and understand this better now, let's talk offline.
https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html (right): https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:2: <html> On 2014/07/24 18:29:36, Rick Byers wrote: > Note that someday we should perhaps update all these link highlighting tests to > match our layout test style (getting written down here: > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines), but > for now I'm fine with new link highlight tests matching the style of the > existing ones. Acknowledged. https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:15: none On 2014/07/24 18:29:36, Rick Byers wrote: > All this explicit positioning makes this test hard to follow. I'd suggest > simplifying it into something like this (see http://jsbin.com/docuq/1/edit): > > <style> > #test { > width: 150px; > } > #test, #test div { > padding: 15px; > border-style: solid; > } > </style> > > <div id=test class=opaqueHighlight style="cursor: pointer;"> > <div style="cursor: pointer;"> > <div style="cursor: default;"> > <div id=targetDiv></div> > </div> > </div> > </div> Done. Thanks! https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:20: <div style="position: absolute; left: 10px; top: 250px; -webkit-transform: translateZ(0);"> On 2014/07/24 18:29:36, Rick Byers wrote: > Does this div do anything? Can you just remove it? I added it to be consistent with other tests here (which seem to "describe" the outcome). Nuked. https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:25: var clientRect = document.getElementById('targetDiv').getBoundingClientRect(); On 2014/07/24 18:29:36, Rick Byers wrote: > Note that you should be careful to target the div you really want to click on. > This looks like it works by your inner div being in the middle of your > outer-most one, but better to put the targetDiv label on the innermost div. > > When I'm feeling paranoid I'll often also do a double-check in the test like: > 'shouldBe('targetDiv', document.elementFromPoint(x, y).id)'. I've seen tests > get checked in that pass unconditionally (rather than actually test what they > intended to test) due to little bugs around stuff like this. Of course you > should also run your tests on a build without your fix to verify it fails > properly. Done. https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1175: // If the current node has an attached renderer, check the cursor. Otherwise, go up. On 2014/07/24 18:29:36, Rick Byers wrote: > I'd omit this comment - the code says this very clearly. Done. https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1178: if (cursor != CURSOR_AUTO || frame->eventHandler().useHandCursor(node, node->isLink())) After some experiment based on yours, it seems that node->renderer()->style()->cursor() produces not the "final" cursor but the one defined in this node's DOM. I'll show you a case offline. On 2014/07/24 18:29:36, Rick Byers wrote: > I don't think this is right. If a node explicitly sets the cursor to 'auto' > then that should also prevent highlight. Eg. try this test case > http://jsbin.com/mojole/1/edit. I think I've led you astray on this CL - I'm > sorry. I've just done a bunch of digging and understand this better now, let's > talk offline.
https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html (right): https://codereview.chromium.org/402603005/diff/100001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:20: <div style="position: absolute; left: 10px; top: 250px; -webkit-transform: translateZ(0);"> On 2014/07/29 18:34:50, mustaq wrote: > On 2014/07/24 18:29:36, Rick Byers wrote: > > Does this div do anything? Can you just remove it? > > I added it to be consistent with other tests here (which seem to "describe" the > outcome). Nuked. Once upon a time this used to be necessary to guarantee that the compositor was being invoked. This is now an historical artifact, and I don't think should be necessary. It should likely be removed from the other tests at some point, though in the short term likely causes no harm.
Thanks Mustaq. This is looking really good, but I think there's still one little issue. Rick https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html (right): https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor.html:17: <div class="opaqueHighlight" id="outerDiv" style="cursor: pointer"> Nice, this test looks great! https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html (right): https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html:9: <style> again, move to inside the <head> tag. https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2.html (right): https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2.html:8: <style> nit: <style> belongs inside of <head>. https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1200: if (!showsHandCursor(bestTouchNode, m_page->deprecatedLocalMainFrame(), isOverLink)) { nit: omit braces for single-line if (http://www.chromium.org/blink/coding-style#TOC-Braces) https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((parentNode = bestTouchNode->parentNode()) != 0) { style nit: don't explicitly compare pointers to 0, just rely on implicit conversion to bool: http://www.chromium.org/blink/coding-style#TOC-Null-false-and-0 https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((parentNode = bestTouchNode->parentNode()) != 0) { minor issue I should have mentioned earlier: please use NodeRenderingTraversal::parentElement() instead of Node::parentNode() here and above. There's technically a pre-existing bug here with handling subtle cases involving ShadowDOM. Might as well just fix that at the same time (you shouldn't notice any change in any of your tests). https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1207: if (bestTouchNode->isLink() && !parentNode->isLink()) Note the impact here on this case (which we should test explicitly): <div id=mybutton style="cursor: pointer"> Click me to go <a href=/>Home</a> </div> I originally argued that I expected tapping on "Home" to highlight all of mybutton here, but I realize now that's debatable (it'll ultimately be up to JavaScript code to decide if clicking here navigates to the link or doe something else). So I'm also OK with it highlighting just "Home" in this case. https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1209: if (!showsHandCursor(parentNode, toLocalFrame(m_page->mainFrame()), parentNode->isLink())) What about a case like this: <a> <span>My <b>link</b></span> </a> In that case tapping on the word "link" should cause a highlight around the whole anchor element, but I believe your code will bail out (highlighting just the "link") since the span node will have isLink=false. To fix this I thought you were going to use your 'isOverLink' variable here, instead of checking parentNode->isLink()? You 'break' above ensures that isOverLink always indicates whether bestTouchNode itself is inside of a link.
https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html (right): https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html:9: <style> On 2014/07/30 15:35:51, Rick Byers wrote: > again, move to inside the <head> tag. Done. https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2.html (right): https://codereview.chromium.org/402603005/diff/130001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2.html:8: <style> On 2014/07/30 15:35:51, Rick Byers wrote: > nit: <style> belongs inside of <head>. Done. https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1200: if (!showsHandCursor(bestTouchNode, m_page->deprecatedLocalMainFrame(), isOverLink)) { On 2014/07/30 15:35:51, Rick Byers wrote: > nit: omit braces for single-line if > (http://www.chromium.org/blink/coding-style#TOC-Braces) Done. https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((parentNode = bestTouchNode->parentNode()) != 0) { Missed this, sorry. I wanted to avoid double-parentheses to void the compiler warning :-/ On 2014/07/30 15:35:51, Rick Byers wrote: > style nit: don't explicitly compare pointers to 0, just rely on implicit > conversion to bool: > http://www.chromium.org/blink/coding-style#TOC-Null-false-and-0 https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((parentNode = bestTouchNode->parentNode()) != 0) { On 2014/07/30 15:35:52, Rick Byers wrote: > minor issue I should have mentioned earlier: please use > NodeRenderingTraversal::parentElement() instead of Node::parentNode() here and > above. There's technically a pre-existing bug here with handling subtle cases > involving ShadowDOM. Might as well just fix that at the same time (you > shouldn't notice any change in any of your tests). Done fixing this method. Not sure what other thing could be fixed in this CL---could you please clarify? Did you mean replacing all "node->parentNode()" in this file? https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1207: if (bestTouchNode->isLink() && !parentNode->isLink()) On 2014/07/30 15:35:51, Rick Byers wrote: > Note the impact here on this case (which we should test explicitly): > > <div id=mybutton style="cursor: pointer"> > Click me to go <a href=/>Home</a> > </div> > > I originally argued that I expected tapping on "Home" to highlight all of > mybutton here, but I realize now that's debatable (it'll ultimately be up to > JavaScript code to decide if clicking here navigates to the link or doe > something else). So I'm also OK with it highlighting just "Home" in this case. Acknowledged. https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1209: if (!showsHandCursor(parentNode, toLocalFrame(m_page->mainFrame()), parentNode->isLink())) Using isOverLink here instead of parentNode->isLink() causes one test failure (gesture-tapHighlight-imagemap.html), not sure why. I think we have to handle image maps separately, right? On 2014/07/30 15:35:51, Rick Byers wrote: > What about a case like this: > > <a> > <span>My <b>link</b></span> > </a> > > In that case tapping on the word "link" should cause a highlight around the > whole anchor element, but I believe your code will bail out (highlighting just > the "link") since the span node will have isLink=false. > > To fix this I thought you were going to use your 'isOverLink' variable here, > instead of checking parentNode->isLink()? You 'break' above ensures that > isOverLink always indicates whether bestTouchNode itself is inside of a link.
https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((parentNode = bestTouchNode->parentNode()) != 0) { On 2014/07/30 16:40:44, mustaq wrote: > On 2014/07/30 15:35:52, Rick Byers wrote: > > minor issue I should have mentioned earlier: please use > > NodeRenderingTraversal::parentElement() instead of Node::parentNode() here and > > above. There's technically a pre-existing bug here with handling subtle cases > > involving ShadowDOM. Might as well just fix that at the same time (you > > shouldn't notice any change in any of your tests). > > Done fixing this method. > > Not sure what other thing could be fixed in this CL---could you please clarify? > Did you mean replacing all "node->parentNode()" in this file? No, I just meant in this method (on this line, and in the "walk up the tree" block above). https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1209: if (!showsHandCursor(parentNode, toLocalFrame(m_page->mainFrame()), parentNode->isLink())) On 2014/07/30 16:40:45, mustaq wrote: > Using isOverLink here instead of parentNode->isLink() causes one test failure > (gesture-tapHighlight-imagemap.html), not sure why. I think we have to handle > image maps separately, right? > > On 2014/07/30 15:35:51, Rick Byers wrote: > > What about a case like this: > > > > <a> > > <span>My <b>link</b></span> > > </a> > > > > In that case tapping on the word "link" should cause a highlight around the > > whole anchor element, but I believe your code will bail out (highlighting just > > the "link") since the span node will have isLink=false. > > > > To fix this I thought you were going to use your 'isOverLink' variable here, > > instead of checking parentNode->isLink()? You 'break' above ensures that > > isOverLink always indicates whether bestTouchNode itself is inside of a link. > Oh, interesting. Note that there's a TODO in that test that says "make link highlighting work with image maps". I believe bokan@ added this test when he fixed a crash here, but it still wasn't doing the right thing. Perhaps your change makes it do something more reasonable now and so we should just update the expected output? You should probably play with a couple imagemap cases and make sure the change seems reasonable to you/bokan.
Added another test on the pointer-outside-a-link case. It was added even though it may seem like an overfitting because we want to be aware of any change in this behavior in future. https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/130001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1207: if (bestTouchNode->isLink() && !parentNode->isLink()) Added a test for this. On 2014/07/30 15:35:51, Rick Byers wrote: > Note the impact here on this case (which we should test explicitly): > > <div id=mybutton style="cursor: pointer"> > Click me to go <a href=/>Home</a> > </div> > > I originally argued that I expected tapping on "Home" to highlight all of > mybutton here, but I realize now that's debatable (it'll ultimately be up to > JavaScript code to decide if clicking here navigates to the link or doe > something else). So I'm also OK with it highlighting just "Home" in this case.
LGTM with nits and windows build fix https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((currentNode = NodeRenderingTraversal::parent(largestHandCursorNode))) { d'oh - looks like my suggestion to remove the '!= 0' caused a compiler error on Windows, sorry! I see both of the following styles elsewhere in the code: while (Node* currentNode = ...) { ... } and: Node* currentNode = ...; while(currentNode) { currentNode = ...; } https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1211: break; nit: rather than break and test below, you can simplify this by returning immediately (and then no need for the return statement below). https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1219: // Otherwise, bestTouchNode is an image map w/o an enclosing anchor tag. Ah, interesting. Doing another pass like this seems unfortunate but fine - it's rare we'll need it and I can see we won't necessarily know up front which case we're in (although HitTestResult::urlElement() might be able to tell you). Anyway good catch! https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1227: break; same as above https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1235: // Couldn't find a parent with a hand cursor nit: you mean 'without' not 'with', right?
https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1206: while ((currentNode = NodeRenderingTraversal::parent(largestHandCursorNode))) { On 2014/07/31 20:15:10, Rick Byers wrote: > d'oh - looks like my suggestion to remove the '!= 0' caused a compiler error on > Windows, sorry! I see both of the following styles elsewhere in the code: > > while (Node* currentNode = ...) { > ... > } > > and: > > Node* currentNode = ...; > while(currentNode) { > currentNode = ...; > } Yeah, the msvc failures plus the style requirement forced me to add a bit of redundancy in the code (which is IMO worse than comparing with null). https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1211: break; On 2014/07/31 20:15:10, Rick Byers wrote: > nit: rather than break and test below, you can simplify this by returning > immediately (and then no need for the return statement below). Done, thanks. https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1219: // Otherwise, bestTouchNode is an image map w/o an enclosing anchor tag. On 2014/07/31 20:15:10, Rick Byers wrote: > Ah, interesting. Doing another pass like this seems unfortunate but fine - it's > rare we'll need it and I can see we won't necessarily know up front which case > we're in (although HitTestResult::urlElement() might be able to tell you). > Anyway good catch! HitTestResult::isOverLink() returns true for both anchors and the map elements used in images, and this method already relies on (to be precise, performs an AND with) HitTestResult::m_innerURLElement. So I thought the two cases are indistinguishable for that class. Moreover, we can have an <a> inside a <map>, or vice versa. I think the url returned from hit test is ambiguous in either case, right? https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1227: break; On 2014/07/31 20:15:10, Rick Byers wrote: > same as above Done. https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1235: // Couldn't find a parent with a hand cursor On 2014/07/31 20:15:10, Rick Byers wrote: > nit: you mean 'without' not 'with', right? Good catch!
lgtm with nit https://codereview.chromium.org/402603005/diff/250001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html (right): https://codereview.chromium.org/402603005/diff/250001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html:4: <script src="../../resources/js-test.js"></script> nit. 4 spaces indent in blink
https://codereview.chromium.org/402603005/diff/250001/LayoutTests/compositing... File LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html (right): https://codereview.chromium.org/402603005/diff/250001/LayoutTests/compositing... LayoutTests/compositing/gestures/gesture-tapHighlight-nested-cursor2-expected.html:4: <script src="../../resources/js-test.js"></script> On 2014/08/01 07:53:14, jochen wrote: > nit. 4 spaces indent in blink Done.
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mustaq@chromium.org/402603005/270001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20155)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
On 2014/07/31 21:04:49, mustaq wrote: > https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1206: while ((currentNode = > NodeRenderingTraversal::parent(largestHandCursorNode))) { > On 2014/07/31 20:15:10, Rick Byers wrote: > > d'oh - looks like my suggestion to remove the '!= 0' caused a compiler error > on > > Windows, sorry! I see both of the following styles elsewhere in the code: > > > > while (Node* currentNode = ...) { > > ... > > } > > > > and: > > > > Node* currentNode = ...; > > while(currentNode) { > > currentNode = ...; > > } > > Yeah, the msvc failures plus the style requirement forced me to add a bit of > redundancy in the code (which is IMO worse than comparing with null). > > https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1211: break; > On 2014/07/31 20:15:10, Rick Byers wrote: > > nit: rather than break and test below, you can simplify this by returning > > immediately (and then no need for the return statement below). > > Done, thanks. > > https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1219: // Otherwise, bestTouchNode is an image map w/o > an enclosing anchor tag. > On 2014/07/31 20:15:10, Rick Byers wrote: > > Ah, interesting. Doing another pass like this seems unfortunate but fine - > it's > > rare we'll need it and I can see we won't necessarily know up front which case > > we're in (although HitTestResult::urlElement() might be able to tell you). > > Anyway good catch! > > HitTestResult::isOverLink() returns true for both anchors and the map elements > used in images, and this method already relies on (to be precise, performs an > AND with) HitTestResult::m_innerURLElement. So I thought the two cases are > indistinguishable for that class. > > Moreover, we can have an <a> inside a <map>, or vice versa. I think the url > returned from hit test is ambiguous in either case, right? Yeah that's exactly what I was wondering. Are you sure an <a> can be inside a <map>? I thought <map> was pretty special and didn't generally contain arbitrary dom element children. If it can only occur the other direction, then you might be able to distinguish the cases by looking at the URL element returned from the hit-test (depending on whether it picks the inner-most or outer-most - not sure offhand). But regardless I don't think it's worth digging into further - what you have here is good! > > https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1227: break; > On 2014/07/31 20:15:10, Rick Byers wrote: > > same as above > > Done. > > https://codereview.chromium.org/402603005/diff/210001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1235: // Couldn't find a parent with a hand cursor > On 2014/07/31 20:15:10, Rick Byers wrote: > > nit: you mean 'without' not 'with', right? > > Good catch!
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mustaq@chromium.org/402603005/290001
https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1178: if (cursor != CURSOR_AUTO || frame->eventHandler().useHandCursor(node, node->isLink())) After fixing the test failures caused by changes in drawing algorithm (https://code.google.com/p/chromium/issues/detail?id=399719), I found that all remaining test failures were caused by seemingly inconsistent value of hit-test-result->isOverLink. I have a demo to show you offline. At this moment, I have reverted back to my last working logic which makes all tests happy, and keeps the behavior above (http://jsbin.com/mojole/1/edit) unchanged. This is not the best solution, but we are not making this case worse at least. On 2014/07/29 18:34:50, mustaq wrote: > After some experiment based on yours, it seems that > node->renderer()->style()->cursor() produces not the "final" cursor but the one > defined in this node's DOM. I'll show you a case offline. > > On 2014/07/24 18:29:36, Rick Byers wrote: > > I don't think this is right. If a node explicitly sets the cursor to 'auto' > > then that should also prevent highlight. Eg. try this test case > > http://jsbin.com/mojole/1/edit. I think I've led you astray on this CL - I'm > > sorry. I've just done a bunch of digging and understand this better now, > let's > > talk offline. >
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21823)
Message was sent while issue was closed.
Change committed as 179863
Message was sent while issue was closed.
On 2014/08/08 18:24:53, mustaq wrote: > https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/402603005/diff/100001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1178: if (cursor != CURSOR_AUTO || > frame->eventHandler().useHandCursor(node, node->isLink())) > After fixing the test failures caused by changes in drawing algorithm > (https://code.google.com/p/chromium/issues/detail?id=399719), I found that all > remaining test failures were caused by seemingly inconsistent value of > hit-test-result->isOverLink. I have a demo to show you offline. > > At this moment, I have reverted back to my last working logic which makes all > tests happy, and keeps the behavior above (http://jsbin.com/mojole/1/edit) > unchanged. This is not the best solution, but we are not making this case worse > at least. > > On 2014/07/29 18:34:50, mustaq wrote: > > After some experiment based on yours, it seems that > > node->renderer()->style()->cursor() produces not the "final" cursor but the > one > > defined in this node's DOM. I'll show you a case offline. > > > > On 2014/07/24 18:29:36, Rick Byers wrote: > > > I don't think this is right. If a node explicitly sets the cursor to 'auto' > > > then that should also prevent highlight. Eg. try this test case > > > http://jsbin.com/mojole/1/edit. I think I've led you astray on this CL - > I'm > > > sorry. I've just done a bunch of digging and understand this better now, > > let's > > > talk offline. > > Created https://code.google.com/p/chromium/issues/detail?id=402583 to track the unexpected highlight case.
Message was sent while issue was closed.
https://codereview.chromium.org/402603005/diff/290001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1178: node = node->parentNode(); Note that you lost the NodeRenderingTraversal::parentNode() fix we discussed here. It just came up in this review: https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp.
Message was sent while issue was closed.
https://codereview.chromium.org/402603005/diff/290001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/402603005/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1178: node = node->parentNode(); Ooops, seems I accidentally reverted the suggestion while stepping back in the CL. I'll fix this now. |
