695984 - Select clicked link or word when opening context menu on Mac

Status: NEW

(Firefox :: General, defect)

Description

[Frank Yan (:fryn)]

In Cocoa-based apps on OS X (Snow Leopard), it looks like it's standard to select the word on which the cursor was placed when opening the context menu.

For example, in Chrome, right-clicking or secondary click/tapping the word "Firefox" on a web page selects the word and opens a context menu with the item "Search Google for 'Firefox'". In Safari, the same happens, except with a menu item labelled "Search in Google". In TextEdit and Terminal, the same happens, except with a menu item labelled "Search in Spotlight".

Also, when right-clicking text inside a link, Chrome and Safari both select the link text and open a context menu with a similar searching item and also the menu item "Copy" to copy that link text.

Both of these are very useful, and I'd like to improve the platform integration and UI efficiency of Firefox by implementing them.

Comment 1

[Frank Yan (:fryn)]

Attachment: hacky patch that works perfectly

This is a hacky patch that I put together.
It works exactly as I would like the final patch to behave:
- Right-clicking inside an existing selection maintains that selection.
- Right-clicking an unselected area deselects any existing selection and does the following:
  - If text inside a link, select all of the link text.
  - If a word not inside a link, select the word.
  - If two words not inside a link, select either both words or nothing.
    (This patch selects both words, and I'm okay with that.)
  - If an area that is not text, do not select anything.

I've pushed it to the UX branch, and you can try it out in the latest UX nightlies (as of October 20, 2011):
https://hg.mozilla.org/projects/ux/rev/61fd918ea50b

Ehsan, could you provide feedback on my patch's implementation and help with an API to select the word as if I were double-clicking on the word without having the simulate a double-mousedown (which is what the hacky patch does now)? Thank you in advance!

Comment 2

[Frank Yan (:fryn)]

Correction:
  - If two words not inside a link, select either both words or nothing.
Should be:
  - If the click is in the space between two words not inside a link, select either both words or nothing.

Comment 3

[(no longer active)]

Comment on attachment 568317 [details] [diff] [review]
hacky patch that works perfectly

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

Overall this look good.  Please see my comments inline.

::: browser/base/content/nsContextMenu.js
@@ +1468,5 @@
>    }
>  };
> +
> +#ifdef XP_MACOSX
> +function selectContext(event) {

This method needs to be e10s aware.  :-)

@@ +1475,5 @@
> +  let x = event.screenX - box.screenX, y = event.screenY - box.screenY;
> +  let clickIsInside = function(rect)
> +    x >= rect.left && x <= rect.left + rect.width &&
> +    y >= rect.top && y <= rect.top + rect.height;
> +  let selection = document.commandDispatcher.focusedWindow.getSelection();

This will not work correctly when right-clicking on text controls, since they have their own selection objects.

@@ +1483,5 @@
> +  selection.removeAllRanges();
> +
> +  // If click is on text, select word(s).
> +  let node = document.popupNode, range = document.createRange();
> +  if (function clickIsOnText(node) {

Nit: please define the function before the if statement.

@@ +1493,5 @@
> +  }(node)) {
> +    // If click is on text in link, select link contents.
> +    do {
> +      if (node instanceof HTMLAnchorElement && node.href) {
> +        range.selectNodeContents(node);

If the anchor contains other things as well (for example an image) do we still want to select it?

@@ +1503,5 @@
> +      removeEventListener("mousedown", suppressHandlers, true);
> +      event.stopPropagation();
> +    }, true);
> +    QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).
> +    sendMouseEvent("mousedown", event.clientX, event.clientY, 0, 2, 0);

If the element has other mousedown event handlers attached to it, will this trigger them being selected as well?

Maybe it's best to implement this part on the Gecko side?

Comment 4

[(no longer active)]

See nsFrame::HandleMultiplePress <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2497> for the code in Gecko implementing word selection.  Specifically, you can use the nsFrame::PeekBackwardAndForward API to get the word selection behavior you need.

Comment 5

[matthias koplenig [:metasieben]]

wouldn't it make sense to have a hidden preference for this, since it might be useful for other platforms aswell?

Comment 6

[Frank Yan (:fryn)]

Unassigning myself, as I'm not actively working on this.

Comment 8

[Frank Yan (:fryn)]

Attachment: WIP v1

I got this working correctly when right-clicking directly on a word.
Unfortunately, it also selects text when right-clicking outside of text regions in the same cases that we select text when double-clicking in those locations.
What is the best way to detect whether the cursor is actually inside the rects of a text region?
In other words, SelectByTypeAtPoint in nsFrame does not provide the behavior that is desired here.

Also, ideally, the previous selection should be deselected when right-clicking outside of the region in which dragging the cursor would have produced that selection. I am not sure how to implement this either.

Comment 9

[(no longer active)]

Do you expect to get this behavior only in inputs and textareas, or in all editable fields?

Comment 10

[(no longer active)]

(Talking about contentEditable fields and documents in designMode.)

Comment 11

[Frank Yan (:fryn)]

(In reply to Ehsan Akhgari [:ehsan])
> Do you expect to get this behavior only in inputs and textareas, or in all
> editable fields?
> (Talking about contentEditable fields and documents in designMode.)

If by "this behavior" you mean "selecting clicked word when opening context menu", I'd like this behavior to work for any part of the web page that is selectable, including non-contentEditable text. That's how it works in almost all other Cocoa-based text fields on OS X, including in Safari and Chrome.

If by "this behavior" you mean "right-clicking on a word selecting the word and right-clicking in a blank area NOT selecting the nearest word", I'm mostly referring to non-editable fields, e.g. most body text of web pages.

Implementation-wise, I'm like to do the following:

1. Implement a function that returns when a mouse event was outside of the bounds of the current selection, so we can deselect the current selection on right-click if it returns true. (The "bounds of selection" does not always equal the union of the clientRects of the selection ranges. For example, select the second half of a paragraph by dragging beyond the last word of the paragraph. Now right-click. Even though the cursor is slightly outside of the highlighted region, in the desired behavior, we should continue to maintain the selection.)

2. Implement a variant of SelectByTypeAtPoint that only selects words when clicking directly on them. I think I can do this using the target of the nsGUIEvent and then walking down into the tree to check before selecting, but it seems overly convoluted and expensive. Is there a better way?

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to Ehsan Akhgari [:ehsan])
> > Do you expect to get this behavior only in inputs and textareas, or in all
> > editable fields?
> > (Talking about contentEditable fields and documents in designMode.)
> 
> If by "this behavior" you mean "selecting clicked word when opening context
> menu", I'd like this behavior to work for any part of the web page that is
> selectable, including non-contentEditable text. That's how it works in almost
> all other Cocoa-based text fields on OS X, including in Safari and Chrome.

Yes, that is what I meant.  If you wanna do that, you should hit-test and get the frame corresponding to the event position, and call nsIFrame::IsSelectable on it.

> If by "this behavior" you mean "right-clicking on a word selecting the word and
> right-clicking in a blank area NOT selecting the nearest word", I'm mostly
> referring to non-editable fields, e.g. most body text of web pages.

No, sorry, I was confused.  Please ignore my comment about the editability.

> Implementation-wise, I'm like to do the following:
> 
> 1. Implement a function that returns when a mouse event was outside of the
> bounds of the current selection, so we can deselect the current selection on
> right-click if it returns true. (The "bounds of selection" does not always
> equal the union of the clientRects of the selection ranges. For example, select
> the second half of a paragraph by dragging beyond the last word of the
> paragraph. Now right-click. Even though the cursor is slightly outside of the
> highlighted region, in the desired behavior, we should continue to maintain the
> selection.)

Hmm, isn't this just hit-testing?

> 2. Implement a variant of SelectByTypeAtPoint that only selects words when
> clicking directly on them. I think I can do this using the target of the
> nsGUIEvent and then walking down into the tree to check before selecting, but
> it seems overly convoluted and expensive. Is there a better way?

You need to ask roc about that.

Comment 13

[Frank Yan (:fryn)]

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > 1. Implement a function that returns when a mouse event was outside of the
> > bounds of the current selection, so we can deselect the current selection on
> > right-click if it returns true. (The "bounds of selection" does not always
> > equal the union of the clientRects of the selection ranges. For example, select
> > the second half of a paragraph by dragging beyond the last word of the
> > paragraph. Now right-click. Even though the cursor is slightly outside of the
> > highlighted region, in the desired behavior, we should continue to maintain the
> > selection.)
> 
> Hmm, isn't this just hit-testing?

Yes, but I don't know how to compute the region to hit-test against. As I explained above, the union of the clientRects of the selection ranges is often smaller than the correct region.

> > 2. Implement a variant of SelectByTypeAtPoint that only selects words when
> > clicking directly on them. I think I can do this using the target of the
> > nsGUIEvent and then walking down into the tree to check before selecting, but
> > it seems overly convoluted and expensive. Is there a better way?
> 
> You need to ask roc about that.

Roc, could you help me with these two problems?

Comment 14

[(no longer active)]

(In reply to comment #13)
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > > 1. Implement a function that returns when a mouse event was outside of the
> > > bounds of the current selection, so we can deselect the current selection on
> > > right-click if it returns true. (The "bounds of selection" does not always
> > > equal the union of the clientRects of the selection ranges. For example, select
> > > the second half of a paragraph by dragging beyond the last word of the
> > > paragraph. Now right-click. Even though the cursor is slightly outside of the
> > > highlighted region, in the desired behavior, we should continue to maintain the
> > > selection.)
> > 
> > Hmm, isn't this just hit-testing?
> 
> Yes, but I don't know how to compute the region to hit-test against. As I
> explained above, the union of the clientRects of the selection ranges is often
> smaller than the correct region.

I'm not sure why you need to do that.  Unless I'm missing something, you should just get a frame from the hit-testing, and see if it's selected or not.  You don't need to worry about the client rects of the selection range at all.

Comment 15

[(no longer active)]

(And by that, I mean calling the nsIFrame::IsSelected() function.)

Comment 16

[Frank Yan (:fryn)]

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> (And by that, I mean calling the nsIFrame::IsSelected() function.)

If you take a look at the patch, you'll see that I'm using it already, but it's not sufficient. If this is too confusing to follow in text form, I can show you via screen sharing on Vidyo or something.

Comment 17

[(no longer active)]

So, I'm not quite sure what you mean.  Can you please capture the bad behavior on a screencast?

I also tested chrome, and it seems like their behavior is a bit more complicated than this.  For example, right clicking on multi-word links selects the entire link.

Perhaps we should first get a better sense of what the desired behavior is.

Comment 18

[Frank Yan (:fryn)]

(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> Perhaps we should first get a better sense of what the desired behavior is.

I know exactly what the desired behavior is. It's just very difficult to recreate it, so, in the previous comments, I've been trying to break it down into pieces that we'd need to achieve it.
I'll make a screencast of it.

Comment 19

[matthias koplenig [:metasieben]]

while platform integration is great, will it still be possible to use >View Selection Source< after this has landed? neither chrome nor safari have this VERY useful feature.

correct me if i'm wrong, the patch that landed on the ux-branch did not allow that; it cleared the selection as soon as you right-clicked anywhere.

Comment 20

[(no longer active)]

Comment on attachment 660398 [details] [diff] [review]
WIP v1

Clearing the request for now.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

There are complains on Facebook so I dug a bit and find this bug.

(In reply to :Ehsan Akhgari from comment #4)
> See nsFrame::HandleMultiplePress
> <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.
> cpp#2497> for the code in Gecko implementing word selection.  Specifically,
> you can use the nsFrame::PeekBackwardAndForward API to get the word
> selection behavior you need.

Are these APIs scriptable? With e10s I don't think it make sense to do things as WIP does (trying to select the word from parent process through Selection API). IMHO the right place to do thing is here

http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/browser/base/content/content.js#121

but these methods has to be scriptable in order to be called here...

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #21)
> With e10s I don't think it make sense to do
> things as WIP does (trying to select the word from parent process through
> Selection API). 

I am sorry, I didn't look at the second WIP which do stuff in nsFrame.cpp directly in the content process.

Comment 23

[Asif Youssuff]

This was reported on https://www.reddit.com/r/firefox/comments/7mmvl6/select_word_with_secondary_click/ -- desired behavior is shown in: https://gfycat.com/FoolishOblongCob and the Firefox behavior is shown in https://gfycat.com/SecondhandJovialLeonberger.

I am also able to reproduce this behavior.