Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137123009:
Add hittest mode for Touch-action which ignore inline elements and svg elements (Closed)
|
Created:
6 years, 11 months ago by gnana Modified:
6 years, 9 months ago CC:
blink-reviews, krit, bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, f(malita), jchaffraix+rendering, pdr, Stephen Chennney, rwlbuis, leviw_travelin_and_unemployed Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd hittest mode for Touch-action which ignore inline elements and svg elements
Intoducing new HitTestRequest::TouchAction hit test type, which will be used
in Special hit testing for computing effective touch-action in eventhandler.
BUG=247396, 319479
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168443
Patch Set 1 : #
Total comments: 14
Patch Set 2 : incorporated review comments #
Total comments: 12
Patch Set 3 : incoporated review comments #
Total comments: 10
Patch Set 4 : incorporated review comments #
Total comments: 3
Patch Set 5 : incorporated review comments #
Total comments: 2
Patch Set 6 : rebase to trunk and Incorporated review comments #
Messages
Total messages: 31 (0 generated)
Hi Rick, It will be helpful if you can give review comments. With the current changes inline-block are also ignored. i have made a test page to verify inline-block behaviour in IE here http://jsfiddle.net/gnanasekar/vTKW3/1/ Can you check the result of this test case in your spare time. I will add more testcase to this unit test. if you have any suggestion on test case please let me know. https://chromiumcodereview.appspot.com/137123009/diff/130001/Source/web/tests... File Source/web/tests/data/touch-action-hittest.html (right): https://chromiumcodereview.appspot.com/137123009/diff/130001/Source/web/tests... Source/web/tests/data/touch-action-hittest.html:14: <span style='display: inline-block' expected-hittest='3' hittest='3'>HitTestRequest::TouchAction should not ignore inline block elements This case hittest fails. Need to check if inline-blocks should not be ignored in hittest.
Thanks Gnanasekar, this is a good start! https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/H... File Source/core/rendering/HitTestRequest.h (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/H... Source/core/rendering/HitTestRequest.h:44: TouchAction = 1 << 13 Please add a brief comment describing this: eg. "Hit testing for touch-action considers only blocks-level elements": http://www.w3.org/TR/pointerevents/#the-touch-action-css-property. Note that there's more detail about this in the working draft - https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touc... - but that'll be published at some point. https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... File Source/core/rendering/RootInlineBox.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... Source/core/rendering/RootInlineBox.cpp:180: if (request.touchAction()) Does this need to be on a inline-box by inline-box basis, as opposed to just on the appropriate RenderObject objects? Eg. I was hoping we could just tie it to RenderBlock (or maybe RenderBox - probably need to test the non-block RenderBoxes to see what we want). It's probably better to opt-in to touch-action hit testing then to opt-out (just to reduce the risk of having bugs down the line where CC and main-thread hit-testing disagree). I think this really belongs inside of visibleToHitTestRequest (for any other consumers of that API) as opposed to directly in nodeAtPoint. You can either expand visibleToHitTestRequest to look for specific types of renderers, or make it virtual and override it in the appropriate RenderObject subclass(es). https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... File Source/core/rendering/svg/RenderSVGRoot.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGRoot.cpp:401: if (!request.touchAction() && contentBoxRect().contains(pointInBorderBox)) { RenderSVGRoot is a RenderBox so whatever we decide for RenderBox will probably handle the SVG case. The individual SVG elements are all RenderSVGModelObject (not a RenderBox) so won't get touch-action hit testing applied to them if we opt-in in RenderBox/RenderBlock. https://codereview.chromium.org/137123009/diff/130001/Source/web/tests/TouchA... File Source/web/tests/TouchActionTest.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:298: void TouchActionTest::runHitTestOnTree(WebCore::Node* root, WebView* webView, TouchActionTrackingWebViewClient& client) Is there reason you need to add a new type of test? I think the existing 'runTestOnTree' infrastructure should work perfectly for this (and test more of the codepath than what you're testing here). Just create some elements with the desired stype and apply touch-action: none, and verify you get an expected-action of none. Eg. see the reference to bug 319479 in touch-action-simple.html - it's already testing the main case you're fixing here (we just need to change the expected result). Sorry I didn't make this clear earlier. https://codereview.chromium.org/137123009/diff/130001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-hittest.html (right): https://codereview.chromium.org/137123009/diff/130001/Source/web/tests/data/t... Source/web/tests/data/touch-action-hittest.html:14: <span style='display: inline-block' expected-hittest='3' hittest='3'>HitTestRequest::TouchAction should not ignore inline block elements On 2014/01/16 14:36:02, gnana wrote: > This case hittest fails. Need to check if inline-blocks should not be ignored in > hittest. In IE inline-block DOES respect touch-action, so we should as well. I'll add this to the WG e-mail thread you started.
Thanks Rick for review comments. I have few queries which i have put in reply to your comments. Please have a look. https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... File Source/core/rendering/RootInlineBox.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... Source/core/rendering/RootInlineBox.cpp:180: if (request.touchAction()) On 2014/01/17 16:12:46, Rick Byers wrote: > Does this need to be on a inline-box by inline-box basis, as opposed to just on > the appropriate RenderObject objects? Eg. I was hoping we could just tie it to > RenderBlock (or maybe RenderBox - probably need to test the non-block > RenderBoxes to see what we want). > > It's probably better to opt-in to touch-action hit testing then to opt-out (just > to reduce the risk of having bugs down the line where CC and main-thread > hit-testing disagree). Ok. i will apply Opt-in way. > > I think this really belongs inside of visibleToHitTestRequest (for any other > consumers of that API) as opposed to directly in nodeAtPoint. You can either > expand visibleToHitTestRequest to look for specific types of renderers, or make > it virtual and override it in the appropriate RenderObject subclass(es). ok, i too feel this belongs to visibleToHitTestRequest. But only my concern was if it was called in all cases. I m checking on visibleToHitTestRequest approach. I felt instead of renderer we can check for display property like this bool visibleToHitTestRequest(const HitTestRequest& request) const { bool touchAction = request.TouchAction() ? style()->display() == BLOCK || style()->display() == INLINE_BLOCK : true; bool pointerEvent = request.ignorePointerEventsNone() || style()->pointerEvents() != PE_NONE; return style()->visibility() == VISIBLE && touchAction && pointerEvent && !isInert(); } and for SVG case we can still use RenderSvgRoot change. Also I feel other way of making virtual and overriding with opt-in touchaction may not work, as hit test will still happen for non-block elements. https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... File Source/core/rendering/svg/RenderSVGRoot.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGRoot.cpp:401: if (!request.touchAction() && contentBoxRect().contains(pointInBorderBox)) { On 2014/01/17 16:12:46, Rick Byers wrote: > RenderSVGRoot is a RenderBox so whatever we decide for RenderBox will probably > handle the SVG case. The individual SVG elements are all RenderSVGModelObject > (not a RenderBox) so won't get touch-action hit testing applied to them if we > opt-in in RenderBox/RenderBlock. commented reply below https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGRoot.cpp:415: if (hitTestAction == HitTestBlockBackground && visibleToHitTestRequest(request)) { Yes, but visibleToHitTestRequest is called only for container <SVG>element. So hit test will happen on the other svg elements before we reach here. https://codereview.chromium.org/137123009/diff/130001/Source/web/tests/TouchA... File Source/web/tests/TouchActionTest.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:298: void TouchActionTest::runHitTestOnTree(WebCore::Node* root, WebView* webView, TouchActionTrackingWebViewClient& client) On 2014/01/17 16:12:46, Rick Byers wrote: > Is there reason you need to add a new type of test? I think the existing > 'runTestOnTree' infrastructure should work perfectly for this (and test more of > the codepath than what you're testing here). Just create some elements with the > desired stype and apply touch-action: none, and verify you get an > expected-action of none. > > Eg. see the reference to bug 319479 in touch-action-simple.html - it's already > testing the main case you're fixing here (we just need to change the expected > result). Sorry I didn't make this clear earlier. Ok. i understood. i will apply this in next patch.
https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... File Source/core/rendering/RootInlineBox.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... Source/core/rendering/RootInlineBox.cpp:180: if (request.touchAction()) On 2014/01/21 14:00:16, gnana wrote: > On 2014/01/17 16:12:46, Rick Byers wrote: > > Does this need to be on a inline-box by inline-box basis, as opposed to just > on > > the appropriate RenderObject objects? Eg. I was hoping we could just tie it > to > > RenderBlock (or maybe RenderBox - probably need to test the non-block > > RenderBoxes to see what we want). > > > > It's probably better to opt-in to touch-action hit testing then to opt-out > (just > > to reduce the risk of having bugs down the line where CC and main-thread > > hit-testing disagree). > Ok. i will apply Opt-in way. > > > > I think this really belongs inside of visibleToHitTestRequest (for any other > > consumers of that API) as opposed to directly in nodeAtPoint. You can either > > expand visibleToHitTestRequest to look for specific types of renderers, or > make > > it virtual and override it in the appropriate RenderObject subclass(es). > > ok, i too feel this belongs to visibleToHitTestRequest. But only my concern was > if it was called in all cases. > I m checking on visibleToHitTestRequest approach. I felt instead of renderer we > can check for display property like this > bool visibleToHitTestRequest(const HitTestRequest& request) const > { > bool touchAction = request.TouchAction() ? style()->display() == BLOCK || > style()->display() == INLINE_BLOCK : true; > bool pointerEvent = request.ignorePointerEventsNone() || > style()->pointerEvents() != PE_NONE; > > return style()->visibility() == VISIBLE && touchAction && pointerEvent && > !isInert(); > } This might be OK. We'd probably need to look at each of the EDisplay values and decide which it should apply for (I'm guessing it should for flex, and many of the table ones). My gut instinct is that using the RenderObject type is going to be more reliable, but I don't know this well enough. I've pinged leviw@ (who will probably do the OWNERS review of this CL) to ask him to add his opinion. > and for SVG case we can still use RenderSvgRoot change. > > Also I feel other way of making virtual and overriding with opt-in touchaction > may not work, as hit test will still happen for non-block elements. Right, sorry - what I meant was that the base class would return false for all TouchAction-type hit tests, then sub-classes could opt-in. https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... File Source/core/rendering/svg/RenderSVGRoot.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGRoot.cpp:415: if (hitTestAction == HitTestBlockBackground && visibleToHitTestRequest(request)) { On 2014/01/21 14:00:16, gnana wrote: > Yes, but visibleToHitTestRequest is called only for container <SVG>element. So > hit test will happen on the other svg elements before we reach here. Ah, I see! Is there any good reason not to call visibleToHitTestRequest above (in fact, I wonder if not doing so is responsible for some subtle bugs)? Regardless I think it's OK explicitly exclude touchAction above.
https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... File Source/core/rendering/RootInlineBox.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/R... Source/core/rendering/RootInlineBox.cpp:180: if (request.touchAction()) On 2014/01/21 15:24:20, Rick Byers wrote: > On 2014/01/21 14:00:16, gnana wrote: > > On 2014/01/17 16:12:46, Rick Byers wrote: > > > Does this need to be on a inline-box by inline-box basis, as opposed to just > > on > > > the appropriate RenderObject objects? Eg. I was hoping we could just tie it > > to > > > RenderBlock (or maybe RenderBox - probably need to test the non-block > > > RenderBoxes to see what we want). > > > > > > It's probably better to opt-in to touch-action hit testing then to opt-out > > (just > > > to reduce the risk of having bugs down the line where CC and main-thread > > > hit-testing disagree). > > Ok. i will apply Opt-in way. > > > > > > I think this really belongs inside of visibleToHitTestRequest (for any other > > > consumers of that API) as opposed to directly in nodeAtPoint. You can > either > > > expand visibleToHitTestRequest to look for specific types of renderers, or > > make > > > it virtual and override it in the appropriate RenderObject subclass(es). > > > > ok, i too feel this belongs to visibleToHitTestRequest. But only my concern > was > > if it was called in all cases. > > I m checking on visibleToHitTestRequest approach. I felt instead of renderer > we > > can check for display property like this > > bool visibleToHitTestRequest(const HitTestRequest& request) const > > { > > bool touchAction = request.TouchAction() ? style()->display() == BLOCK || > > style()->display() == INLINE_BLOCK : true; > > bool pointerEvent = request.ignorePointerEventsNone() || > > style()->pointerEvents() != PE_NONE; > > > > return style()->visibility() == VISIBLE && touchAction && pointerEvent && > > !isInert(); > > } > > This might be OK. We'd probably need to look at each of the EDisplay values and > decide which it should apply for (I'm guessing it should for flex, and many of > the table ones). My gut instinct is that using the RenderObject type is going > to be more reliable, but I don't know this well enough. I've pinged leviw@ (who > will probably do the OWNERS review of this CL) to ask him to add his opinion. Generally speaking, I agree that what you really want here is to look at the type of renderer. If you want it to apply for blocks, I'd add a virtual method that only RenderBlock (the base class for all block-type renderers) returns true for. Otherwise if new block-level display types come around, they get the expected behavior. Either way, this isn't the right place to do this if, as you seem to specify, you want inline-blocks. You'll never get to them if you bail here. > > > and for SVG case we can still use RenderSvgRoot change. > > > > Also I feel other way of making virtual and overriding with opt-in touchaction > > may not work, as hit test will still happen for non-block elements. > > Right, sorry - what I meant was that the base class would return false for all > TouchAction-type hit tests, then sub-classes could opt-in. https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... File Source/core/rendering/svg/RenderSVGRoot.cpp (right): https://codereview.chromium.org/137123009/diff/130001/Source/core/rendering/s... Source/core/rendering/svg/RenderSVGRoot.cpp:415: if (hitTestAction == HitTestBlockBackground && visibleToHitTestRequest(request)) { On 2014/01/21 15:24:20, Rick Byers wrote: > On 2014/01/21 14:00:16, gnana wrote: > > Yes, but visibleToHitTestRequest is called only for container <SVG>element. So > > hit test will happen on the other svg elements before we reach here. > > Ah, I see! Is there any good reason not to call visibleToHitTestRequest above > (in fact, I wonder if not doing so is responsible for some subtle bugs)? This matches what we do in RenderBlock. > Regardless I think it's OK explicitly exclude touchAction above.
Please check this
Thanks Gnana, this is looking close to me. Rick https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3677: HitTestResult taResult = hitTestResultAtPoint(pagePoint, hitType | HitTestRequest::TouchAction); It's probably worth moving the hit test into computeEffectiveTouchAction (i.e. have it take a point instead of a Node - I don't think it needs to take the hitType, a constant should be fine). https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3679: if (taNode == node) { You'll need to do this even when the hit test returns a different node than 'node'. Eg. touching on an inline that contained inside of a block - the touch-action of the block needs to be applied still. https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/H... File Source/core/rendering/HitTestRequest.h (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/H... Source/core/rendering/HitTestRequest.h:44: TouchAction = 1 << 13, // Hit testing for touch-action considers only blocks-level elements nit: s/blocks/block/ https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.h (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/R... Source/core/rendering/RenderBlock.h:121: return style()->visibility() == VISIBLE && style()->pointerEvents() != PE_NONE && !isInert(); The general approach of updating visibleToHitTestRequest and making it vritual seems good to me, thanks! But it's unfortunate to have to duplicate the normal (not-touch-action-related) hit test logic in RenderObject, RenderBlock, and RenderSVGRoot. How about having the derived functions pass a modified HitTestRequest to the base that clears the touch action flag? I.e. // Touch action hit tests on a RenderBlock are done exactly like normal hit tests if (request.touchAction()) RenderObject::visibleToHitTestRequest(HitTestRequest(request.type() & ~TouchAction); What do you think? https://codereview.chromium.org/137123009/diff/250001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-simple.html (right): https://codereview.chromium.org/137123009/diff/250001/Source/web/tests/data/t... Source/web/tests/data/touch-action-simple.html:29: <p>Below case is broken, should be AUTO - <a href='http://crbug.com/319479'>Bug 319479</a></p> Remove this line (no longer a bug here). https://codereview.chromium.org/137123009/diff/250001/Source/web/tests/data/t... Source/web/tests/data/touch-action-simple.html:30: <div class='ta-none' style='height: 0; margin-bottom: 100px'> We should add a similar test case for SVG (and probably also inline-block, table-cell, etc.). If there's more than a couple, then it's probably worth grouping them into a new file.
https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3676: // FIXME(rbyers): Should really be doing a second hit test that ignores inline elements - crbug.com/319479. Oh, and you can remove this FIXME now of course.
Please have a look. https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3676: // FIXME(rbyers): Should really be doing a second hit test that ignores inline elements - crbug.com/319479. On 2014/02/17 14:44:24, Rick Byers wrote: > Oh, and you can remove this FIXME now of course. Done. https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3677: HitTestResult taResult = hitTestResultAtPoint(pagePoint, hitType | HitTestRequest::TouchAction); On 2014/02/17 14:43:52, Rick Byers wrote: > It's probably worth moving the hit test into computeEffectiveTouchAction (i.e. > have it take a point instead of a Node - I don't think it needs to take the > hitType, a constant should be fine). Done. https://codereview.chromium.org/137123009/diff/250001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3679: if (taNode == node) { On 2014/02/17 14:43:52, Rick Byers wrote: > You'll need to do this even when the hit test returns a different node than > 'node'. Eg. touching on an inline that contained inside of a block - the > touch-action of the block needs to be applied still. Done. https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/H... File Source/core/rendering/HitTestRequest.h (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/H... Source/core/rendering/HitTestRequest.h:44: TouchAction = 1 << 13, // Hit testing for touch-action considers only blocks-level elements On 2014/02/17 14:43:52, Rick Byers wrote: > nit: s/blocks/block/ Done. https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.h (right): https://codereview.chromium.org/137123009/diff/250001/Source/core/rendering/R... Source/core/rendering/RenderBlock.h:121: return style()->visibility() == VISIBLE && style()->pointerEvents() != PE_NONE && !isInert(); On 2014/02/17 14:43:52, Rick Byers wrote: > The general approach of updating visibleToHitTestRequest and making it vritual > seems good to me, thanks! But it's unfortunate to have to duplicate the normal > (not-touch-action-related) hit test logic in RenderObject, RenderBlock, and > RenderSVGRoot. > > How about having the derived functions pass a modified HitTestRequest to the > base that clears the touch action flag? I.e. > > // Touch action hit tests on a RenderBlock are done exactly like normal hit > tests > if (request.touchAction()) > RenderObject::visibleToHitTestRequest(HitTestRequest(request.type() & > ~TouchAction); > > What do you think? This looks better. modified Done. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... File Source/web/tests/TouchActionTest.cpp (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:172: webView->resize(WebSize(800, 1200)); I changed this to fix the unit test failure for <div class='ta-none' style='height: 0; margin-bottom: 50px'> <span expected-action='none' style='display:inline-block'> touch-action should be inherited by inline-block elements </span> </div> error: Test point not contained in visible area Let me know your opnion on this. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:262: if (taResult.innerElement() == result.innerElement()) { I have added this check for cases similar to this <div class='ta-none' style='height: 0; margin-bottom: 100px'> <span expected-action='auto'> touch-action should not be inherited by inline elements <div expected-action='none'>But is inherited by any block descendants of them</div> </span> </div> the below two checks fail, because the count is set for the div and touch action of div is none. Please suggest me how i can fix it in better way. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-simple.html (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/data/t... Source/web/tests/data/touch-action-simple.html:51: </svg> while running unit test, it crashes for SVG while accessing height() RefPtr<WebCore::ClientRectList> rects = element->getClientRects(); ASSERT_GE(rects->length(), 0u) << failureContext; RefPtr<WebCore::ClientRect> r = rects->item(0); WebCore::FloatRect clientFloatRect = WebCore::FloatRect(r->left(), r->top(), r->width(), r->height()); need to check this. any suggestion will be helpful
Looking good! Unfortunately it seems your test has uncovered a real bug we'll want to track down :-) https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... File Source/web/tests/TouchActionTest.cpp (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:172: webView->resize(WebSize(800, 1200)); On 2014/02/24 18:59:50, gnana wrote: > I changed this to fix the unit test failure for > > <div class='ta-none' style='height: 0; margin-bottom: 50px'> > <span expected-action='none' style='display:inline-block'> > touch-action should be inherited by inline-block elements > </span> > </div> > error: Test point not contained in visible area > > Let me know your opnion on this. Yeah, that should be fine. At some point we may want to try to compress things a bit to make them fit better, or split into multiple test cases. But as long as we have a descriptive failure like this I think we're fine. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:262: if (taResult.innerElement() == result.innerElement()) { On 2014/02/24 18:59:50, gnana wrote: > I have added this check for cases similar to this > > <div class='ta-none' style='height: 0; margin-bottom: 100px'> > <span expected-action='auto'> > touch-action should not be inherited by inline elements > <div expected-action='none'>But is inherited by any block descendants of > them</div> > </span> > </div> > > the below two checks fail, because the count is set for the div and touch > action of div is none. > > Please suggest me how i can fix it in better way. > Oh! Something is wrong with the hit testing code then. I expected the hit test to go through the span and not consider it's parent div since it's outside it's bounds, and so ultimately land in the body (at least that's how IE appears to behave). But it looks like the hit test against the parent div is still being satisfied, even though the point is outside it's bounds. Here's a similar test case involving mouse events and pointer-events:none: http://jsbin.com/bosad/1/edit. I would have expected clicking on the overflow text NOT to trigger the event handler, but it does. Perhaps this is a pre-existing hit testing bug? This repro is consistent with IE behavior though, so I don't think we necessarily want to change that. Can you debug and see where exactly in the hit testing code we're deciding that the hit does actually reach the div? Perhaps there's an issue in RenderBlock::hitTestContents, maybe to do with HitTestBlockBackgrounds? (I don't know what that's doing). Perhaps there's just a step we can skip for touch-action hit tests. This is really getting at the crux of the CL here. We don't want an object's inline children to influence the hit test result at all (should behave the same as if the child was display: none). Here it still appears to be influencing the hit test somehow. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-simple.html (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/data/t... Source/web/tests/data/touch-action-simple.html:51: </svg> On 2014/02/24 18:59:50, gnana wrote: > while running unit test, it crashes for SVG while accessing height() > > RefPtr<WebCore::ClientRectList> rects = element->getClientRects(); > ASSERT_GE(rects->length(), 0u) << failureContext; > RefPtr<WebCore::ClientRect> r = rects->item(0); > WebCore::FloatRect clientFloatRect = WebCore::FloatRect(r->left(), > r->top(), r->width(), r->height()); > > need to check this. any suggestion will be helpful Oh, looks like I should have had an ASSERT_GT instead of an ASSERT_GE here - sorry about that. Presumably ClientRectList::length is 0 in this case (I didn't expect to have expected-action on SVG elements). Once you switch to ASSERT_GT it'll just fail the ASSERT instead of crashing. I think it's fine not to test this case, just test the SVG root case similar to your table-cell case (i.e. the expected-action is on the <svg> element, and the class='ta-none' is on a 0-size parent div).
I feel the patch is ready for owner review. Please check. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... File Source/web/tests/TouchActionTest.cpp (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:172: webView->resize(WebSize(800, 1200)); On 2014/02/25 22:52:03, Rick Byers wrote: > On 2014/02/24 18:59:50, gnana wrote: > > I changed this to fix the unit test failure for > > > > <div class='ta-none' style='height: 0; margin-bottom: 50px'> > > <span expected-action='none' style='display:inline-block'> > > touch-action should be inherited by inline-block elements > > </span> > > </div> > > error: Test point not contained in visible area > > > > Let me know your opnion on this. > > Yeah, that should be fine. At some point we may want to try to compress things > a bit to make them fit better, or split into multiple test cases. But as long > as we have a descriptive failure like this I think we're fine. Done. https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:262: if (taResult.innerElement() == result.innerElement()) { On 2014/02/25 22:52:03, Rick Byers wrote: > On 2014/02/24 18:59:50, gnana wrote: > > I have added this check for cases similar to this > > > > <div class='ta-none' style='height: 0; margin-bottom: 100px'> > > <span expected-action='auto'> > > touch-action should not be inherited by inline elements > > <div expected-action='none'>But is inherited by any block descendants of > > them</div> > > </span> > > </div> > > > > the below two checks fail, because the count is set for the div and touch > > action of div is none. > > > > Please suggest me how i can fix it in better way. > > > > Oh! Something is wrong with the hit testing code then. I expected the hit test > to go through the span and not consider it's parent div since it's outside it's > bounds, and so ultimately land in the body (at least that's how IE appears to > behave). But it looks like the hit test against the parent div is still being > satisfied, even though the point is outside it's bounds. Here's a similar test > case involving mouse events and pointer-events:none: > http://jsbin.com/bosad/1/edit. I would have expected clicking on the overflow > text NOT to trigger the event handler, but it does. Perhaps this is a > pre-existing hit testing bug? This repro is consistent with IE behavior though, > so I don't think we necessarily want to change that. > > Can you debug and see where exactly in the hit testing code we're deciding that > the hit does actually reach the div? Perhaps there's an issue in > RenderBlock::hitTestContents, maybe to do with HitTestBlockBackgrounds? (I don't > know what that's doing). Perhaps there's just a step we can skip for > touch-action hit tests. > > This is really getting at the crux of the CL here. We don't want an object's > inline children to influence the hit test result at all (should behave the same > as if the child was display: none). Here it still appears to be influencing the > hit test somehow. Debugged and found that this issue caused in inlineflowbox::nodeatpoint. i commented more details in inlineflowbox.cpp https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-simple.html (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/data/t... Source/web/tests/data/touch-action-simple.html:51: </svg> On 2014/02/25 22:52:03, Rick Byers wrote: > On 2014/02/24 18:59:50, gnana wrote: > > while running unit test, it crashes for SVG while accessing height() > > > > RefPtr<WebCore::ClientRectList> rects = element->getClientRects(); > > ASSERT_GE(rects->length(), 0u) << failureContext; > > RefPtr<WebCore::ClientRect> r = rects->item(0); > > WebCore::FloatRect clientFloatRect = WebCore::FloatRect(r->left(), > > r->top(), r->width(), r->height()); > > > > need to check this. any suggestion will be helpful > > Oh, looks like I should have had an ASSERT_GT instead of an ASSERT_GE here - > sorry about that. Presumably ClientRectList::length is 0 in this case (I didn't > expect to have expected-action on SVG elements). Once you switch to ASSERT_GT > it'll just fail the ASSERT instead of crashing. I think it's fine not to test > this case, just test the SVG root case similar to your table-cell case (i.e. the > expected-action is on the <svg> element, and the class='ta-none' is on a 0-size > parent div). Added test for svg root alone. Done. https://codereview.chromium.org/137123009/diff/480001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/137123009/diff/480001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1059: if (visibleToHitTestRequest(request) && locationInContainer.intersects(rect)) { i feel this rect should have been computed from renderer->size(), then the locationInContainer.intersects(rect) will fail. But its computed from InlineFlowBox::roundedFrameRect(). I dont understand why this is done. I have added a hittestrequest::touchaction check to fix the bug. Now body element is return for the test case when touch on span element. <div class='ta-none' style='height: 0; margin-bottom: 50px'> <span expected-action='auto'> touch-action should not be inherited by inline elements </span> </div>
LGTM modulo the one tiny suggestion. Levi, can you please take a look? https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... File Source/web/tests/TouchActionTest.cpp (right): https://codereview.chromium.org/137123009/diff/360001/Source/web/tests/TouchA... Source/web/tests/TouchActionTest.cpp:262: if (taResult.innerElement() == result.innerElement()) { On 2014/02/27 13:26:59, gnana wrote: > On 2014/02/25 22:52:03, Rick Byers wrote: > > On 2014/02/24 18:59:50, gnana wrote: > > > I have added this check for cases similar to this > > > > > > <div class='ta-none' style='height: 0; margin-bottom: 100px'> > > > <span expected-action='auto'> > > > touch-action should not be inherited by inline elements > > > <div expected-action='none'>But is inherited by any block descendants of > > > them</div> > > > </span> > > > </div> > > > > > > the below two checks fail, because the count is set for the div and touch > > > action of div is none. > > > > > > Please suggest me how i can fix it in better way. > > > > > > > Oh! Something is wrong with the hit testing code then. I expected the hit > test > > to go through the span and not consider it's parent div since it's outside > it's > > bounds, and so ultimately land in the body (at least that's how IE appears to > > behave). But it looks like the hit test against the parent div is still being > > satisfied, even though the point is outside it's bounds. Here's a similar > test > > case involving mouse events and pointer-events:none: > > http://jsbin.com/bosad/1/edit. I would have expected clicking on the overflow > > text NOT to trigger the event handler, but it does. Perhaps this is a > > pre-existing hit testing bug? This repro is consistent with IE behavior > though, > > so I don't think we necessarily want to change that. > > > > Can you debug and see where exactly in the hit testing code we're deciding > that > > the hit does actually reach the div? Perhaps there's an issue in > > RenderBlock::hitTestContents, maybe to do with HitTestBlockBackgrounds? (I > don't > > know what that's doing). Perhaps there's just a step we can skip for > > touch-action hit tests. > > > > This is really getting at the crux of the CL here. We don't want an object's > > inline children to influence the hit test result at all (should behave the > same > > as if the child was display: none). Here it still appears to be influencing > the > > hit test somehow. > > Debugged and found that this issue caused in inlineflowbox::nodeatpoint. i > commented more details in inlineflowbox.cpp Good work! https://codereview.chromium.org/137123009/diff/480001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/137123009/diff/480001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1059: if (visibleToHitTestRequest(request) && locationInContainer.intersects(rect)) { On 2014/02/27 13:26:59, gnana wrote: > i feel this rect should have been computed from renderer->size(), then the > locationInContainer.intersects(rect) will fail. > But its computed from InlineFlowBox::roundedFrameRect(). I dont understand why > this is done. > > I have added a hittestrequest::touchaction check to fix the bug. Now body > element is return for the test case when touch on span element. > > <div class='ta-none' style='height: 0; margin-bottom: 50px'> > <span expected-action='auto'> > touch-action should not be inherited by inline elements > </span> > </div> Ah, this makes sense. The RenderBlock has a bunch of InlineFlowBoxes, and InlineBox::visibleToHitTestRequest is calling into RenderBlock::visibleToHitTestRequest here. I think this is the right thing to do, except that the touchAction check probably belongs inside of InlineBox::visibleToHitTestRequest instead of here. That way any other uses of InlineBox know that touch-action hit tests are never satisfied by them.
Thanks Rick. https://codereview.chromium.org/137123009/diff/480001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/137123009/diff/480001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1059: if (visibleToHitTestRequest(request) && locationInContainer.intersects(rect)) { On 2014/02/27 14:53:11, Rick Byers wrote: > On 2014/02/27 13:26:59, gnana wrote: > > i feel this rect should have been computed from renderer->size(), then the > > locationInContainer.intersects(rect) will fail. > > But its computed from InlineFlowBox::roundedFrameRect(). I dont understand why > > this is done. > > > > I have added a hittestrequest::touchaction check to fix the bug. Now body > > element is return for the test case when touch on span element. > > > > <div class='ta-none' style='height: 0; margin-bottom: 50px'> > > <span expected-action='auto'> > > touch-action should not be inherited by inline elements > > </span> > > </div> > > Ah, this makes sense. The RenderBlock has a bunch of InlineFlowBoxes, and > InlineBox::visibleToHitTestRequest is calling into > RenderBlock::visibleToHitTestRequest here. I think this is the right thing to > do, except that the touchAction check probably belongs inside of > InlineBox::visibleToHitTestRequest instead of here. That way any other uses of > InlineBox know that touch-action hit tests are never satisfied by them. Moved the change to InlineBox Done.
https://codereview.chromium.org/137123009/diff/500001/Source/core/rendering/R... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/137123009/diff/500001/Source/core/rendering/R... Source/core/rendering/RenderObject.h:927: if (request.touchAction()) Instead of making this virtual and adding a branch for touchAction to every implementation, why not keep the single action and add a method like "visibleForTouchAction"? Then you just need 4 virtual methods that just return true or false with no logic, and the logic here would just be "if (request.touchAction() && visibleForTouchAction())" That seems way clearer than multipl visibleToHitTestRequest methods with logic in them.
Thanks Levi. Please have a look once again. https://codereview.chromium.org/137123009/diff/500001/Source/core/rendering/R... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/137123009/diff/500001/Source/core/rendering/R... Source/core/rendering/RenderObject.h:927: if (request.touchAction()) On 2014/02/28 18:14:39, Levi wrote: > Instead of making this virtual and adding a branch for touchAction to every > implementation, why not keep the single action and add a method like > "visibleForTouchAction"? Then you just need 4 virtual methods that just return > true or false with no logic, and the logic here would just be "if > (request.touchAction() && visibleForTouchAction())" That seems way clearer than > multipl visibleToHitTestRequest methods with logic in them. Done.
This is exactly what I was looking for. Thanks! LGTM.
The CQ bit was checked by gnanasekar.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/137123009/560001
The CQ bit was unchecked by gnanasekar.s@samsung.com
Need trivial LGTM for Below Presubmit Errors
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
Source/web/tests/TouchActionTest.cpp
Source/web/tests/data/touch-action-simple.html
Need trivial LGTM for Below Presubmit Errors
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
Source/web/tests/TouchActionTest.cpp
Source/web/tests/data/touch-action-simple.html
Source/web/tests rslgtm
The CQ bit was checked by abarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/137123009/560001
The CQ bit was unchecked by gnanasekar.s@samsung.com
The CQ bit was checked by gnanasekar.s@samsung.com
The CQ bit was unchecked by gnanasekar.s@samsung.com
The CQ bit was checked by gnanasekar.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/137123009/560001
Message was sent while issue was closed.
Change committed as 168443 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
