Closed Bug 780847 Opened 9 years ago Closed 9 years ago

fluff out touch click targets

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-basecamp +

People

(Reporter: gal, Assigned: roc)

References

Details

Attachments

(4 files, 7 obsolete files)

patch
13.45 KB, patch
Details | Diff | Splinter Review
Add support for checking for event listeners with an nsIAtom* event name
2.05 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Fix nsLayoutUtils::IsProperAncestorFrame(CrossDoc) v2
2.39 KB, patch
mats
: review+
Details | Diff | Splinter Review
main patch v5
24.66 KB, patch
mats
: review+
Details | Diff | Splinter Review 
We currently use some code in the frontend code in fennec to fluff out click targets. We should do this in the event handling code proper.

The fennec code is in ElementTouchHelper and can be ported pretty easily (JS), and should probably eventually be C++.

smaug pointed out that nsLayoutUtils::GetFrameForPoint might be the right place to hook this into.
blocking-basecamp: --- → +
If we think this is hard, we can take a shortcut and port over the fennec code 1:1 for now.
You actually want PresShell::HandleEvent to call nsLayoutUtils::GetFramesForArea instead of GetFrameForPoint.

The code in ElementTouchHelper could be ported to a helper method in PresShell pretty easily.
also see bug 547997.
Attached patch patch (obsolete) — — Splinter Review
Attached patch patch — — Splinter Review
Attachment #649814 - Attachment is obsolete: true
Comment on attachment 649815 [details] [diff] [review]
patch

>diff --git a/dom/base/TouchManager.js b/dom/base/TouchManager.js
Please keep this somewhere in b2g land, or at least not in dom/base
Andreas, I think it would be cleaner to go straight to C++. I can do that now. Would you like me to?
<3
Assignee: nobody → roc
roc, I think the fennec code does roughly the right thing, but maybe not really at the right level. I think its good to look at active click targets (links, things with event handlers listening), but we probably want to look at frames, not the DOM.
roc, dougt is suggesting to pull in wesj if you need any help with the fennec code, since he wrote it
Attached patch WIP (obsolete) — — Splinter Review 
Complete as far as I know, but totally untested.
I'd like to think about this a little bit more and add some better documentation. However, it seems to work OK, at least it passes the mochitests I've created.
Comment on attachment 652757 [details] [diff] [review]
Add support for checking for event listeners with an nsIAtom* event name

Code looks ok (although I'd put the NS_ASSERTION inside ifdef debug), but
I don't quite understand why this is needed.
I think you'd want something which checks if there are
js implemented event listeners in default group.
ListenerStruct has mListenerType.
Attachment #652757 - Flags: review?(bugs) → review-
Comment on attachment 652758 [details] [diff] [review]
Fix nsLayoutUtils::IsProperAncestorFrame(CrossDoc) to handle cases where aFrame or aAncestorFrame are the root

> Bug 780847. Fix nsLayoutUtils::IsProperAncestorFrame(CrossDoc) to handle
> cases where aFrame or aAncestorFrame are the root.

I can't tell what you mean by "Fix".

The documentation (and name) of IsProperAncestorFrame says that
it returns false when aAncestorFrame == aFrame.  In other words,
IsProperAncestorFrame(x,x) is false for all x.

This patch makes IsProperAncestorFrame(root,root) return true.

r- if that's your intention, because I think it's the wrong result.
(you should probably just add a new method in this case)

r- if that's not your intention - new patch expected with explanation
of what it is you're fixing exactly
Attachment #652758 - Flags: review?(matspal) → review-
(In reply to Olli Pettay [:smaug] from comment #16)
> Code looks ok (although I'd put the NS_ASSERTION inside ifdef debug), but
> I don't quite understand why this is needed.
> I think you'd want something which checks if there are
> js implemented event listeners in default group.
> ListenerStruct has mListenerType.

I'm just refactoring the existing code used by Fennec's ElementTouchHelper. But I also think that checking all event listeners is the right way to go. We want to select the element as a good touch target if it will respond to the event in any way.
EventListenerManager check doesn't include all the default handling
But I guess that is why there is the manual check for certain elements.
(the check should check namespace)
Attached patch Main patch v2 (obsolete) — — Splinter Review 
Added namespace check and some comments. Everything's green on try.
Attachment #652759 - Attachment is obsolete: true
Attachment #653222 - Flags: review?(bugs)
Comment on attachment 653209 [details] [diff] [review]
Fix nsLayoutUtils::IsProperAncestorFrame(CrossDoc) v2

I don't see anything wrong with the old code in the case where
aFrame or aAncestorFrame are the root (as the commit message
claims), can you point out the bug please?
Attachment #652757 - Flags: review- → review+
(In reply to Mats Palmgren [:mats] from comment #24)
> I don't see anything wrong with the old code in the case where
> aFrame or aAncestorFrame are the root (as the commit message
> claims), can you point out the bug please?

Consider aAncestorFrame == aCommonAncestor == aFrame->GetParent(). The current implementation of IsProperAncestorFrameCrossDoc returns false, which is incorrect.
Comment on attachment 653209 [details] [diff] [review]
Fix nsLayoutUtils::IsProperAncestorFrame(CrossDoc) v2

OK, got it.  I was confused by "root" in the commit message.
Please update it to better describe the problem.
Attachment #653209 - Flags: review?(matspal) → review+
Comment on attachment 653222 [details] [diff] [review]
Main patch v2


>+static nsIFrame*
>+GetClosest(nsIFrame* aRoot, const nsPoint& aPointRelativeToRootFrame,
>+           const char* aPrefBranch, nsIFrame* aRestrictToDescendants,
>+           nsTArray<nsIFrame*>& aCandidates)
>+{
>+  nsIFrame* bestTarget = nullptr;
>+  // Lower is better; distance is in appunits
>+  float bestDistance = 1e6f;
>+  for (PRUint32 i = 0; i < aCandidates.Length(); ++i) {
>+    nsIFrame* f = aCandidates[i];
>+    if (!IsElementClickable(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))
>+      continue;
>+    if (!nsLayoutUtils::IsAncestorFrameCrossDoc(aRestrictToDescendants, f, aRoot))
>+      continue;
This misses some {}


>+FindFrameTargetedByInputEvent(PRUint8 aEventStructType,
>+                              nsIFrame* aRootFrame,
>+                              const nsPoint& aPointRelativeToRootFrame,
>+                              PRUint32 aFlags)
>+{
>+  bool ignoreRootScrollFrame = (aFlags & INPUT_IGNORE_ROOT_SCROLL_FRAME) != 0;
>+  const char* prefBranch = nullptr;
>+  if (aEventStructType == NS_MOZTOUCH_EVENT ||
>+      aEventStructType == NS_TOUCH_EVENT) {
>+    prefBranch = "touch";
>+  } else if (aEventStructType == NS_MOUSE_EVENT) {
>+    // Mostly for testing purposes
>+    prefBranch = "mouse";
>+  }
>+  bool useSearchRadius = false;
>+  if (prefBranch) {
>+    nsPrintfCString prefName("ui.%s.radius.enabled", prefBranch);
>+    useSearchRadius = Preferences::GetBool(prefName.get());
>+  }
I think we should cache the pref values, especially because 
Preferences::Add*VarCache is just super simple to use.


>+  // If the exact target is non-null, only consider candidate targets in the same
>+  // document as the exact target. Otherwise, if an ancestor document has
>+  // a mouse event handler for example, targets that are !IsElementClickable can
>+  // never be targeted --- something nsSubDocumentFrame in an ancestor document
>+  // would be targeted instead.
>+  nsIFrame* restrictToDescendants = target
>+      ? target->PresContext()->PresShell()->GetRootFrame() : aRootFrame;
? should be in the previous line and 2 space indentation.
Attachment #653222 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #27)
> I think we should cache the pref values, especially because 
> Preferences::Add*VarCache is just super simple to use.

I can do that, of course, but it seems a bit pointless to me. Do you think that this could actually ever show up on a profile?
It might (especially if we end up using the code for other things too on mobile), 
and since it is trivial to cache the pref values and not cause code to
jump to totally different place, I'd prefer caching the values.
Attached patch main patch v3 (obsolete) — — Splinter Review
Attachment #655514 - Flags: review?(bugs)
Comment on attachment 655514 [details] [diff] [review]
main patch v3


>+ * Any descendant (in the same document) of a clickable element is also
>+ * deemed clickable since events will bubble to the clickable element from its
>+ * descendant.
s/bubble/propagate/


>+struct EventRadiusPrefs {
{ goes to next line.
>+static bool
>+IsElementClickable(nsIFrame* aFrame)
>+{
>+  // Input events bubble 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->GetParent()) {
Nit, missing one space before content

>+    } else if (content->IsSVG()) {
>+      nsIAtom* tag = content->Tag();
>+      if (tag == nsGkAtoms::a) {
>+        return true;
>+      }
>+    }

] else if (content->IsSVG(nsGkAtoms::a)) {
  return true;
}

>+GetAppUnitsFromMMPref(nsIFrame* aFrame, uint32_t aMM, bool aVertical)
>+{
>+  nsPresContext* pc = aFrame->PresContext();
>+  nsIPresShell* presShell = pc->PresShell();
>+  float result = float(aMM)*
space before *

>+    (pc->DeviceContext()->AppUnitsPerPhysicalInch()/25.4f)*
space before and after / and before *
25.4f needs some explanation.
>+      distance *= aPrefs->mVisitedWeight/100.0f;
Nit, space before and after /


>+namespace mozilla {
>+
>+enum {
>+  INPUT_IGNORE_ROOT_SCROLL_FRAME = 0x01
>+};
Enums should be in form eValueName



>+      uint32_t aFlags = 0;
aFoo is form is reserved for parameters. Just flags


>+      if (aEvent->eventStructType == NS_MOUSE_EVENT &&
>+          static_cast<nsMouseEvent*>(aEvent)->ignoreRootScrollFrame) {
>+        aFlags |= INPUT_IGNORE_ROOT_SCROLL_FRAME;
Not sure I like assigning enum values to uint32_t, but ok.


Mats should also review this.
Attachment #655514 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #31)
> >+namespace mozilla {
> >+
> >+enum {
> >+  INPUT_IGNORE_ROOT_SCROLL_FRAME = 0x01
> >+};
> Enums should be in form eValueName

We have loads of enums that aren't, especially enums that are actually defining bit flags. So I think it's actually more consistent to use A_B style here.

> >+      uint32_t aFlags = 0;
> aFoo is form is reserved for parameters. Just flags

aFlags is a parameter. I don't understand.

> >+      if (aEvent->eventStructType == NS_MOUSE_EVENT &&
> >+          static_cast<nsMouseEvent*>(aEvent)->ignoreRootScrollFrame) {
> >+        aFlags |= INPUT_IGNORE_ROOT_SCROLL_FRAME;
> Not sure I like assigning enum values to uint32_t, but ok.

We use enums as a portable way to define constants inline in classes, all over the place. It's not strictly needed here since we're not in a class scope, but it seems to me we might as well keep doing it.
Attached patch main patch v4 (obsolete) — — Splinter Review 
Asking for re-review because I didn't take all the suggested changes.
Attachment #655514 - Attachment is obsolete: true
Attachment #656644 - Flags: review?(bugs)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)

> > >+      uint32_t aFlags = 0;
> > aFoo is form is reserved for parameters. Just flags
> 
> aFlags is a parameter. I don't understand.

It is not. You define a local variable called aFlags.
Local variables should not be in form aFoo.
Comment on attachment 656644 [details] [diff] [review]
main patch v4

Well, ok for the enum (I still think we should use enums consistently, but we aren't), but fix the aFlags
Attachment #656644 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #34)
> It is not. You define a local variable called aFlags.
> Local variables should not be in form aFoo.

Oh, *that* aFlags. Absolutely, will fix, sorry :-).
Comment on attachment 656644 [details] [diff] [review]
main patch v4

> layout/base/PositionedEventTargeting.cpp

Kudos for the elaborate overview!

> + * [snip] For visited links we multiply the distance
> + * weight be a specified constants; [snip]

huh?

> +  uint32_t mVisitedWeight;

"// in percent" for clarity

> +  uint32_t mSideRadii[4]; // TRBL order

"// TRBL order, in millimeters" for clarity

>+IsElementClickable(nsIFrame* aFrame)
>...
>+      if (tag == nsGkAtoms::a ||

I'm not sure a plain <a> without href should be in this list.
Isn't the IsLink test that comes later better?

I actually tested this with a <a>Chapter 1</a> near a link, and it didn't
feel right that it should influence the radius of the link.
Can you explain why you think it should?

While testing (Fx on Linux) I also noted that clicking on the lower part
of the File menu button actuated the Back button below it.  I'm guessing
this feature is meant for Fennec without XUL so it's not important, but
perhaps you could add an early return for IsXUL somewhere anyway?

>+static nscoord
>+GetAppUnitsFromMMPref(nsIFrame* aFrame, uint32_t aMM, bool aVertical)

Remove the Get prefix.  The Pref suffix also seems unnecessary.

> +  static const float MM_PER_INCH = 25.4f;

Maybe you can use this one instead:
./gfx/src/nsCoord.h:#define MM_PER_INCH_FLOAT             25.4f

> modules/libpref/src/init/all.js

You have put the prefs in the "#ifdef XP_WIN" section.
Please move them to the end of the general section (just above
"#ifdef XP_WIN" as it happens).


r=mats with those addressed
Attachment #656644 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren [:mats] from comment #37)
> I'm not sure a plain <a> without href should be in this list.
> Isn't the IsLink test that comes later better?

I copied that from the Fennec code I was adapting. You're right. I'll remove <a> from that list.

> While testing (Fx on Linux) I also noted that clicking on the lower part
> of the File menu button actuated the Back button below it.  I'm guessing
> this feature is meant for Fennec without XUL so it's not important, but
> perhaps you could add an early return for IsXUL somewhere anyway?

I've added some basic support for XUL elements. I've also modified the loop up the DOM tree to use GetFlattenedTreeParent instead of just GetParent to try to support XBL a bit better. But it's true this is for Web content, B2G and Fennec, so XUL and XBL aren't very important and I don't think we should try hard to make them work well, at this stage.
Attached patch main patch v5 — — Splinter Review
Attachment #656644 - Attachment is obsolete: true
Attachment #657233 - Flags: review?(matspal)
Comment on attachment 657233 [details] [diff] [review]
main patch v5

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> ... I'll remove <a> from that list.

OK, please also remove the <svg:a>, it has IsLink:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGAElement.cpp#191

> I've added some basic support for XUL elements.

Thanks, that fixed the File menu problem and UI seems to work reasonably
well in general.  Clicking on popup menus still have the same problem,
e.g. Preferences->When Nightly Starts.  Adding menulist to the list of
XUL tags makes it work.

r=mats
Attachment #657233 - Flags: review?(matspal) → review+
Blocks: 788109
Depends on: 832101
Depends on: 835175
Depends on: 837242
No longer depends on: 835175
Depends on: 901036
You need to log in before you can comment on or make changes to this bug.