783531 - Implement a "select word at point" text selection routine in nsIDOMWindowUtils

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jim Mathies [:jimm]]

We currently hack this functionality in various ways, in Android and xul fennec we send mouse clicks to content to move the caret around and then use the nsISelectionController to (hopefully) expand selection to a word around that caret. Bug 695984 is another good example of where we want precise word selection but don't have it.

For metro we want very precise word selection on tap and hold. The mouse click/ selection controller hack isn't working reliably. When selecting text in a page we shouldn't be interacting with content through mouse clicks or any other event and we don't want to muck with focus either.

This bug is about implementing a helper in nsIDOMWindowUtils that does this right. 

Note we can't just select the closest nsIDOMNode, we'll need to identify the right node and the correct offsets within that node to generate a proper nsISelection the routine will return.

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2779

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelection.idl

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

needs testing and maybe a test or two.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 655355 [details] [diff] [review]
patch v.1

An api addition to nsIDOMWindowUtils that is incredibly useful in getting selection working properly on mobile devices.

Comment 4

[Jim Mathies [:jimm]]

(Added a uuid bump to my local patch.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 655355 [details] [diff] [review]
patch v.1

Review of attachment 655355 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.h
@@ +341,5 @@
> +                                 nsIWidget* aWidget,
> +                                 const nsIntPoint aPoint,
> +                                 nsSelectionAmount aBeginAmountType,
> +                                 nsSelectionAmount aEndAmountType,
> +                                 bool aAllowMultipleSelections);

This doesn't need to be XPCOM-ish does it? Make it non-virtual and return void.

Also, 'bool' parameters suck. make it a flags word.

Make callers do GetEventCoordinatesRelativeTo so this doesn't have to take aWidget, and aPoint should be in appunits (and relative to this frame).

Comment 6

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> ::: layout/generic/nsFrame.h
> @@ +341,5 @@
> > +                                 nsIWidget* aWidget,
> > +                                 const nsIntPoint aPoint,
> > +                                 nsSelectionAmount aBeginAmountType,
> > +                                 nsSelectionAmount aEndAmountType,
> > +                                 bool aAllowMultipleSelections);
> 
> This doesn't need to be XPCOM-ish does it? Make it non-virtual and return
> void.


I can ditch the nsresult, but I need the return result. bool return ok?


> Also, 'bool' parameters suck. make it a flags word.
>
> Make callers do GetEventCoordinatesRelativeTo so this doesn't have to take
> aWidget, and aPoint should be in appunits (and relative to this frame).

Ok, will do.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jim Mathies [:jimm] from comment #6)
> I can ditch the nsresult, but I need the return result. bool return ok?

If the return result means success/failure, keep using nsresult. But make the method non-virtual.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 9

[Jim Mathies [:jimm]]

Hmm, so it turns out our selection behavior isn't uniform across mac, linux, and windows. So a few of these tests will either have to be customized per platform or simply commented out.

Open question though, should our selection behavior be uniform? I'm guessing we tweak it so that it follows native os behavior.

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jim Mathies [:jimm] from comment #9)
> Open question though, should our selection behavior be uniform? I'm guessing
> we tweak it so that it follows native os behavior.

Right.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 658916 [details] [diff] [review]
patch v.2

Review of attachment 658916 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +932,5 @@
> +   * @param aSelectType The selection behavior requested.
> +   * @aAccumulate Accumulate with any existing selection. If true operation
> +   * will add a new range to existing selection. If false any existing
> +   * selection will be cleared first. This also affects caret position.
> +   * @return True if a selection occured, false otherwise.

Do we really need the accumulate parameter? Couldn't we always accumulate and have the caller clear the selection if necessary?

::: layout/generic/nsFrame.h
@@ +343,5 @@
> +  // Used by SelectByTypeAtPoint, designates whether selection
> +  // can be accumulated.
> +  enum SelectType {
> +    SELECT_SINGLE_SELECTION = 0,
> +    SELECT_ALLOW_MULTIPLE = 1

I think a flags word would be better. So a uint32_t parameter, defaulting to zero if you like, and here you just have one value, SELECT_ALLOW_MULTIPLE = 0x01.

@@ +371,5 @@
>                                      bool aJumpLines,
>                                      bool aMultipleSelection);
>  
> +  nsresult SelectByTypeAtPoint(nsPresContext* aPresContext,
> +                               const nsPoint aPoint,

const nsPoint& or just nsPoint

Comment 13

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 658916 [details] [diff] [review]
> patch v.2
> 
> Review of attachment 658916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/base/nsIDOMWindowUtils.idl
> @@ +932,5 @@
> > +   * @param aSelectType The selection behavior requested.
> > +   * @aAccumulate Accumulate with any existing selection. If true operation
> > +   * will add a new range to existing selection. If false any existing
> > +   * selection will be cleared first. This also affects caret position.
> > +   * @return True if a selection occured, false otherwise.
> 
> Do we really need the accumulate parameter? Couldn't we always accumulate
> and have the caller clear the selection if necessary?

I can live without it. Will update.

> 
> ::: layout/generic/nsFrame.h
> @@ +343,5 @@
> > +  // Used by SelectByTypeAtPoint, designates whether selection
> > +  // can be accumulated.
> > +  enum SelectType {
> > +    SELECT_SINGLE_SELECTION = 0,
> > +    SELECT_ALLOW_MULTIPLE = 1
> 
> I think a flags word would be better. So a uint32_t parameter, defaulting to
> zero if you like, and here you just have one value, SELECT_ALLOW_MULTIPLE =
> 0x01.

Ok, will do.

> 
> @@ +371,5 @@
> >                                      bool aJumpLines,
> >                                      bool aMultipleSelection);
> >  
> > +  nsresult SelectByTypeAtPoint(nsPresContext* aPresContext,
> > +                               const nsPoint aPoint,
> 
> const nsPoint& or just nsPoint

Oops.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jim Mathies [:jimm] from comment #13)
> > I think a flags word would be better. So a uint32_t parameter, defaulting to
> > zero if you like, and here you just have one value, SELECT_ALLOW_MULTIPLE =
> > 0x01.
> 
> Ok, will do.

You don't actually need it if we get rid of the accumulate parameter, do you?

Comment 15

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> (In reply to Jim Mathies [:jimm] from comment #13)
> > > I think a flags word would be better. So a uint32_t parameter, defaulting to
> > > zero if you like, and here you just have one value, SELECT_ALLOW_MULTIPLE =
> > > 0x01.
> > 
> > Ok, will do.
> 
> You don't actually need it if we get rid of the accumulate parameter, do you?

Yes because HandleMultiplePress has |bool aControlHeld|, which currently gets converted into a SelectType for SelectByTypeAtPoint, which then gets converted back into a bool for PeekBackwardAndForward's aMultipleSelection. :) Maybe PeekBackwardAndForward should use the uint32_t paramter as well to simplify this. But HandleMultiplePress definitely wants to be able to flip between an accumulate and single selection mode.

Comment 16

[Jim Mathies [:jimm]]

In looking it over it looks like we have to keep the accumulate parameter if we want to reuse SelectByTypeAtPoint in HandleMultiplePress. SelectByTypeAtPoint mostly came out of HandleMultiplePress.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, but the flag should be called SELECT_ACCUMULATE instead of SELECT_ALLOW_MULTIPLE, since just "allowing multiple" doesn't necessarily imply that it should accumulate as well.

Comment 18

[Jim Mathies [:jimm]]

Attachment: patch v.3

If you like I can propagate the aSelectFlags flags down into HandleClick as well.

Comment 19

[Jim Mathies [:jimm]]

Note I removed the cheesy nsFrame.h comments too. Those have been in there for thirteen years.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 659231 [details] [diff] [review]
patch v.3

Review of attachment 659231 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMWindowUtils.cpp
@@ +2622,5 @@
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }
> +
> +  nsSelectionAmount amount;
> +  switch(aSelectBehavior) {

Space before (

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +920,5 @@
> +  const unsigned long SELECT_BEHAVIOR_LINE        = 3;
> +  const unsigned long SELECT_BEHAVIOR_BEGINLINE   = 4;
> +  const unsigned long SELECT_BEHAVIOR_ENDLINE     = 5;
> +  const unsigned long SELECT_BEHAVIOR_PARAGRAPH   = 6;
> +  const unsigned long SELECT_BEHAVIOR_WORDNOSPACE = 7;

Remove the _BEHAVIOR? Seems unnecessary.

::: dom/tests/mochitest/chrome/selectAtPoint.html
@@ +59,5 @@
> +    let text = selection.toString();
> +
> +    // Test
> +    ok(text == aExpectedSelectionText, aSelectTypeStr + " selection text matches?");
> +    dumpLn(aSelectTypeStr + " selection text:", "[" + text + "]");

probably worth having a debug variable to control these dumps. (Default to 'off')

::: layout/generic/nsFrame.h
@@ +100,5 @@
> + */
> +
> +// Designates that selection is accumulated (added) to existing
> +// selection.
> +#define SELECT_ACCUMULATE   0x01

Put enum { SELECT_ACCUMULATE = 0x01 }; in nsFrame right before PeekBackwardAndForward.

@@ +366,5 @@
>                             nsGUIEvent *    aEvent,
>                             nsEventStatus*  aEventStatus);
>  
>    NS_IMETHOD PeekBackwardAndForward(nsSelectionAmount aAmountBack,
>                                      nsSelectionAmount aAmountForward,

Why is PeekBackwardAndForward virtual anyway? You could fix that while you're here if you want to :-).

Comment 21

[Jim Mathies [:jimm]]

Updated per comments. One difference, the dump output is useful when the test fails, so I commented out the other non-critical dumpLn, and placed a condition on dumping the selected text for when the test fails.

I'm a little nervous this will randomly orange, so I want the text output there in the event that it does.

Comment 22

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70429c07246a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70429c07246a

Comment 24

[Jim Mathies [:jimm]]

We have mdn docs that need to expose this - 
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDOMWindowUtils