Bug 1181763 - Allow the target fluffing code to fluff even when directly hitting something clickable. r=roc
authorKartikaya Gupta <kgupta@mozilla.com>
Fri, 17 Jul 2015 08:36:00 -0400
changeset 253513 285d33e0631d057568a1c5df7c6f92ef139196b5
parent 253512 a1e66a69688ff99cd16d9c9ed008d494a2dddf70
child 253514 498153aa50a7c5473d784b244fc9018f34c612b2
push id29067
push userkwierso@gmail.com
push dateSat, 18 Jul 2015 00:57:04 +0000
treeherdermozilla-central@e2f2eb9ecca0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs1181763
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1181763 - Allow the target fluffing code to fluff even when directly hitting something clickable. r=roc There is a common pattern on the web where a click listener is registered on a container element high up in the DOM tree, and based on the target of the click events, it performs the appropriate action. In such cases, our existing fluffing code was not getting activated anywhere inside the container, because the entire container was considered clickable. However, this is not user-friendly because often the actual targets inside the container are small and hard to hit. Also, the fluffing code will often take the container element itself as the target, even if the user actually hit something inside the container. This patch changes this behaviour so when an event hits inside a clickable container, fluffing still occurs, but is restricted to DOM descendants of the container. This allows fluffing to work in the above scenarios, and since the events will bubble up to the container, the listeners on the container are guaranteed to still trigger.
layout/base/PositionedEventTargeting.cpp
layout/base/tests/test_event_target_radius.html
--- a/layout/base/PositionedEventTargeting.cpp
+++ b/layout/base/PositionedEventTargeting.cpp
@@ -168,16 +168,31 @@ HasTouchListener(nsIContent* aContent)
     return false;
   }
 
   return elm->HasListenersFor(nsGkAtoms::ontouchstart) ||
          elm->HasListenersFor(nsGkAtoms::ontouchend);
 }
 
 static bool
+IsDescendant(nsIFrame* aFrame, nsIContent* aAncestor, nsAutoString* aLabelTargetId)
+{
+  for (nsIContent* content = aFrame->GetContent(); content;
+       content = content->GetFlattenedTreeParent()) {
+    if (aLabelTargetId && content->IsHTMLElement(nsGkAtoms::label)) {
+      content->GetAttr(kNameSpaceID_None, nsGkAtoms::_for, *aLabelTargetId);
+    }
+    if (content == aAncestor) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static bool
 IsElementClickable(nsIFrame* aFrame, nsIAtom* stopAt = nullptr, nsAutoString* aLabelTargetId = nullptr)
 {
   // Input events propagate up the content tree so we'll follow the content
   // ancestors to look for elements accepting the click.
   for (nsIContent* content = aFrame->GetContent(); content;
        content = content->GetFlattenedTreeParent()) {
     if (stopAt && content->IsHTMLElement(stopAt)) {
       break;
@@ -339,18 +354,18 @@ static bool IsElementPresent(nsTArray<ns
     }
   }
   return false;
 }
 
 static nsIFrame*
 GetClosest(nsIFrame* aRoot, const nsPoint& aPointRelativeToRootFrame,
            const nsRect& aTargetRect, const EventRadiusPrefs* aPrefs,
-           nsIFrame* aRestrictToDescendants, nsTArray<nsIFrame*>& aCandidates,
-           int32_t* aElementsInCluster)
+           nsIFrame* aRestrictToDescendants, nsIContent* aClickableAncestor,
+           nsTArray<nsIFrame*>& aCandidates, int32_t* aElementsInCluster)
 {
   nsIFrame* bestTarget = nullptr;
   // Lower is better; distance is in appunits
   float bestDistance = 1e6f;
   nsRegion exposedRegion(aTargetRect);
   for (uint32_t i = 0; i < aCandidates.Length(); ++i) {
     nsIFrame* f = aCandidates[i];
     PET_LOG("Checking candidate %p\n", f);
@@ -368,17 +383,22 @@ GetClosest(nsIFrame* aRoot, const nsPoin
 
     if (preservesAxisAlignedRectangles) {
       // Subtract from the exposed region if we have a transform that won't make
       // the bounds include a bunch of area that we don't actually cover.
       SubtractFromExposedRegion(&exposedRegion, region);
     }
 
     nsAutoString labelTargetId;
-    if (!IsElementClickable(f, nsGkAtoms::body, &labelTargetId)) {
+    if (aClickableAncestor) {
+      if (!IsDescendant(f, aClickableAncestor, &labelTargetId)) {
+        PET_LOG("  candidate %p is not a descendant of required ancestor\n", f);
+        continue;
+      }
+    } else if (!IsElementClickable(f, nsGkAtoms::body, &labelTargetId)) {
       PET_LOG("  candidate %p was not clickable\n", f);
       continue;
     }
     // If our current closest frame is a descendant of 'f', skip 'f' (prefer
     // the nested frame).
     if (bestTarget && nsLayoutUtils::IsProperAncestorFrameCrossDoc(f, bestTarget, aRoot)) {
       PET_LOG("  candidate %p was ancestor for bestTarget %p\n", f, bestTarget);
       continue;
@@ -498,22 +518,23 @@ FindFrameTargetedByInputEvent(WidgetGUIE
              (aEvent->mClass == eTouchEventClass ? "touch" : "other")),
     mozilla::layers::Stringify(aPointRelativeToRootFrame).c_str(), aRootFrame);
 
   const EventRadiusPrefs* prefs = GetPrefsFor(aEvent->mClass);
   if (!prefs || !prefs->mEnabled) {
     PET_LOG("Retargeting disabled\n");
     return target;
   }
+  nsIContent* clickableAncestor = nullptr;
   if (target && IsElementClickable(target, nsGkAtoms::body)) {
     if (!IsElementClickableAndReadable(target, aEvent, prefs)) {
       aEvent->AsMouseEventBase()->hitCluster = true;
     }
     PET_LOG("Target %p is clickable\n", target);
-    return target;
+    clickableAncestor = target->GetContent();
   }
 
   // Do not modify targeting for actual mouse hardware; only for mouse
   // events generated by touch-screen hardware.
   if (aEvent->mClass == eMouseEventClass &&
       prefs->mTouchOnly &&
       aEvent->AsMouseEvent()->inputSource !=
         nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
@@ -538,17 +559,18 @@ FindFrameTargetedByInputEvent(WidgetGUIE
   if (NS_FAILED(rv)) {
     return target;
   }
 
   int32_t elementsInCluster = 0;
 
   nsIFrame* closestClickable =
     GetClosest(aRootFrame, aPointRelativeToRootFrame, targetRect, prefs,
-               restrictToDescendants, candidates, &elementsInCluster);
+               restrictToDescendants, clickableAncestor, candidates,
+               &elementsInCluster);
   if (closestClickable) {
     if ((!prefs->mTouchClusterDetectionDisabled && elementsInCluster > 1) ||
         (!IsElementClickableAndReadable(closestClickable, aEvent, prefs))) {
       if (aEvent->mClass == eMouseEventClass) {
         WidgetMouseEventBase* mouseEventBase = aEvent->AsMouseEventBase();
         mouseEventBase->hitCluster = true;
       }
     }
--- a/layout/base/tests/test_event_target_radius.html
+++ b/layout/base/tests/test_event_target_radius.html
@@ -33,18 +33,19 @@ https://bugzilla.mozilla.org/show_bug.cg
 
   <div class="target" style="transform:translate(-80px,0);" id="t4" onmousedown="x=1" hidden></div>
 
   <div class="target" style="left:0; z-index:1" id="t5_left" onmousedown="x=1" hidden></div>
   <div class="target" style="left:106px;" id="t5_right" onmousedown="x=1" hidden></div>
   <div class="target" style="left:0; top:210px;" id="t5_below" onmousedown="x=1" hidden></div>
 
   <div class="target" id="t6" onmousedown="x=1" hidden>
-    <div id="t6_inner" style="position:absolute; left:-20px; top:20px; width:60px; height:60px; background:yellow;"></div>
+    <div id="t6_inner" style="position:absolute; left:-40px; top:20px; width:60px; height:60px; background:yellow;"></div>
   </div>
+  <div id="t6_outer" style="position:absolute; left:160px; top:120px; width:60px; height:60px; background:green;" onmousedown="x=1" hidden></div>
 
   <div class="target" id="t7" onmousedown="x=1" hidden></div>
   <div class="target" id="t7_over" hidden></div>
 
   <div id="t8" contenteditable="true" class="target" hidden></div>
 
   <div id="t9" class="target" ontouchend="x=1" hidden></div>
 
@@ -182,23 +183,30 @@ function test3() {
   testMouseClick("t5_left", 10, 106, "t5_below", "closest DIV is below");
   setShowing("t5_left", false);
   setShowing("t5_right", false);
   setShowing("t5_below", false);
 
   // Test behavior of nested elements.
   // The following behaviors are questionable and may need to be changed.
   setShowing("t6", true);
+  setShowing("t6_outer", true);
   testMouseClick("t6_inner", -1, 10, "t6_inner",
     "inner element is clickable because its parent is, even when it sticks outside parent");
-  testMouseClick("t6_inner", 19, -1, "t6_inner",
+  testMouseClick("t6_inner", 39, -1, "t6_inner",
     "when outside both inner and parent, but in range of both, the inner is selected");
-  testMouseClick("t6_inner", 25, -1, "t6",
-    "clicking in clickable parent close to inner activates parent, not inner");
+  testMouseClick("t6_inner", 45, -1, "t6_inner",
+    "clicking in clickable parent close to inner activates inner, not parent");
+  ok(13*mm < 80, "no point inside t6 that's not within radius of t6_inner; adjust layout of t6/inner/outer as needed");
+  testMouseClick("t6_outer", -40 + 13*mm, -1, "t6",
+    "clicking in clickable container close to outer activates parent, not outer");
+  testMouseClick("t6_outer", 1, 1, "t6_outer",
+    "clicking directly on the outer activates it");
   setShowing("t6", false);
+  setShowing("t6_outer", false);
 
   setShowing("t7", true);
   setShowing("t7_over", true);
   testMouseClick("t7", 100 + 11*mm, 10, "body", "covered div is not clickable");
   testMouseClick("t7", 10, 10, "t7_over", "covered div is not clickable even within its bounds");
   setShowing("t7", false);
   setShowing("t7_over", false);