565717 - "Search Google for" context menu entry should be in textareas/inputs too

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[John Wayne Hill (UX Intern)]

(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!)

To reproduce:
1. open a page with a form
2. select some text on the page that isn't in a text input
3. notice the "Search Google for"
4. select some text within a text input
5. notice the "Search Google for" is missing

Recommendation:
Add the "Search Google for" context menu when selecting text within a text input and text area.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This is a simple fix to nsContextMenu.js

Comment 2

[David Kaplan]

I tried implementing the fix as an intro to the firefox codebase.
Only issue is I couldn't find the necessary equivalent to getBrowserSelection() for the text input fields (that function only seems to work on content selection text).

Unless I've completely approached this wrong...

Comment 3

[David Kaplan]

I even tried document.commandDispatcher.focusedWindow.getSelection().toString(); but that only works for the normal content as well (not text/input fields)...

Comment 4

[imradyurrad]

(In reply to comment #3)
> I even tried
> document.commandDispatcher.focusedWindow.getSelection().toString(); but that
> only works for the normal content as well (not text/input fields)...

Did you ever figure it out?

Comment 5

[Steve Glardon (Glards)]

Attachment: Fix for the bug.

This patch should fix the issue described here.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 469626 [details] [diff] [review]
Fix for the bug.

Would this patch not also potentially open us up to a security bug where this could return the selection object for another window, which could potentially be from a different domain?

Comment 7

[Steve Glardon (Glards)]

(In reply to comment #6)
> Comment on attachment 469626 [details] [diff] [review]
> Fix for the bug.
> 
> Would this patch not also potentially open us up to a security bug where this
> could return the selection object for another window, which could potentially
> be from a different domain?

I did some tests and the security manager does not allow to access the window.getSelection() for a different domain.

I made a document with 2 frames. In each frame there was a text box. Both frame had a javascript called every second to output the window.getSelection(). I selected the text in the textbox and each frame was writing the selection of its own textbox.

Then I made a test to get the selection of the other frame, it was working. I tried with the other frame being www.google.com and that's where the security manager didn't allow to get the selection.

If you think of some other way I can test this, let me know.

Comment 8

[imradyurrad]

Three months of inactivity =(

Comment 9

[Paul [pwd]]

This would be a huge win, especially if it's as little change as described and/or implemented in the current patch that's up for review. Is there a chance that this could make it into final? Will we need to reassign this to a new reviewer?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

That patch affects the result of window.getSelection, which is a web-exposed API - we're quite unlikely to take that change at this point, because of the compatibility concerns.

A more targeted fix would be to just adjust the nsContextMenu code to check both selections.

Comment 11

[Paul [pwd]]

Can someone with permissions mark the current patch obsolete?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

It's not necessarily obsolete.

Comment 13

[Joshua Jerome]

Attachment: Patch to nsContextMenu.js

This is my attempt at patching nsContextMenu.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 515706 [details] [diff] [review]
Patch to nsContextMenu.js

Thanks for the patch! A couple of issues:

getTextSelection should probably use the same condition as setTarget does for inline spell checking: (this.onTextInput && !this.target.readOnly &&
 this.target.type != "password").

This seems to lose the 16-char limit argument to getBrowserSelection for the isTextSelection path, which seems wrong.

Comment 15

[Joshua Jerome]

(In reply to comment #14)
> Comment on attachment 515706 [details] [diff] [review]
> Patch to nsContextMenu.js
> 
> Thanks for the patch! A couple of issues:
> 
> getTextSelection should probably use the same condition as setTarget does for
> inline spell checking: (this.onTextInput && !this.target.readOnly &&
>  this.target.type != "password").

Oops, sorry. 

> This seems to lose the 16-char limit argument to getBrowserSelection for the
> isTextSelection path, which seems wrong.

Yeah I noticed this, but I wasn't sure how serious the change was since the string is shortened later on in isTextSelection.

Additionally the access key for searching and spell check are the same. Should I try to address that in the same patch or is that another bug?

Comment 16

[Joshua Jerome]

Attachment: Patch updated.

This should fix both issues pointed out.

Comment 17

[Paul [pwd]]

Could this still make it into Firefox 5 or are we past the deadline for that?

Comment 19

[Paul [pwd]]

(In reply to comment #16)
> attachment.is_obsolete: 0 => 1; flag: review?([e-mail])Created attachment 515998 [details] [diff] [review]
> Patch updated.
> 
> This should fix both issues pointed out.

Having read http://blog.mozilla.com/jorendorff/2011/04/20/how-to-fix-a-bug-episode-434494-part-2/ you might want to give Gavin a bit of a prod:
"       <heycam> in general I have no idea how long I should be letting my
                patches sit in someones queue before bugging them about it :)
   <jorendorff> At least 3 days. But never 2 weeks."

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 515998 [details] [diff] [review]
Patch updated.

Sorry for the delay. I looked at this the other day, I have a couple of comments:
- I was wrong to suggest using exactly the same condition as spellcheck: unlike spellchecking, it's still useful to be able to search for selections from readOnly text fields, so I don't think we want that condition.
- You can simplify getTextSelection slightly by avoiding the temporary and just returning values directly.
- getTextSelection needs to enforce the length limit for selections returned from inputs as well, otherwise you could end up with a huge context menu.

Apart from that, I think this patch is good. I'm going to attach a patch that attempts to address these comments. Joshua, can you take a look at it and confirm that it looks good to you?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This adds basic support for truncating the length. Don't think we need to be as fancy as getBrowserSelection (re: only using "relevant" characters, or trimming whitespace), since text field selections are more likely to be "simple" text.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

It'd be nice to have some test coverage for this too. Something along the lines of browser_bug417483.js, maybe.

Comment 24

[eviatarbach]

Any news on the progress of this bug?

Comment 25

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Gavin, your patch works good on latest-mozilla-central.
(the patch need to rebase, but it's a easy work.)

Why doesn't this patch land?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

I just haven't had time to followup. If you'd like to take this bug and write a test, that'd be great!

Comment 27

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch with test

I add tests.

But there is a problem.
On <input>, #context-search's accesskeys conflicts with #spell-check-enabled's one.
How do we resolve it?

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Use a different access key? :) Are there no more available letters or something?

Comment 29

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch with test rev2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #28)
> Use a different access key? :) Are there no more available letters or
> something?

I change the accesskey of #spell-check-enabled.
Please review this.

Comment 30

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 654051 [details] [diff] [review]
patch with test rev2

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

I tried tests, this patch failed on some tests. I need to fix them before reviewing.

BTW, Gavin, Who should be developer of this patch? I took over from an original developer.

Comment 31

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev3

Update the patch.
I change the approach to get a selected text in a text edit area.
(As a result, this does not have parts which I took over. So I sign my name)

And also, this passes tests which this patch relates ;)
https://tbpl.mozilla.org/?tree=Try&rev=1c2de47eac78

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 656489 [details] [diff] [review]
patch rev3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function getBrowserSelection(aCharLen) {

>+    let isOnTextInput = function isOnTextInput(aElm) {
>+      if (aElm instanceof HTMLInputElement) {
>+        return aElm.mozIsTextField(false);
>+      }
>+      else {
>+        return (aElm instanceof HTMLTextAreaElement);
>+      }
>+    };

Just use:
function isOnTextInput(elem) {
  return elem instanceof HTMLTextAreaElement ||
         ((elem instanceof HTMLInputElement) && elem.mozIsTextField(true));
}

>+    if (isOnTextInput(elm) && elm.type !== "password") {

Then you don't need to check type!=="password" here.

>+      selection = elm.QueryInterface(Ci.nsIDOMNSEditableElement).

Can this QI fail?

Comment 33

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #32)
> > if (isOnTextInput(elm) && elm.type !== "password") {
> 
> Then you don't need to check type!=="password" here.

I don't think it. If we remove to check type!=="password", getBrowserSelection() gets "●●●●●" when input[type="password"] has focus. I seems that this is meaningless.
And as a result, #context-search is shown with label like "Search for '●●●●●'". I seem this is tedious. e.g. Chromium does not show "search for google" on context-menu input[type="password"].

However, if we think that getBrowserSelection() should be designed as getting whatever browser selected text, we need to remove to check type!=="password" from getBrowserSelection() and I'll modify nsContextMenu.js to not showing #context-search on input[type="password"].

Which should I do?


> > selection = elm.QueryInterface(Ci.nsIDOMNSEditableElement).
> 
> Can this QI fail?

No. We can't access |elm.editor| if we delete this QI.

Comment 34

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev4

Comment 35

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: update nsContextMenu for patch rev4

my proposal.
-getBrowserSelection() should be designed as getting whatever browser selected text.
-not showing #context-search on input[type="password"].

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

If you need to check if the editor is password field mode strictly, you need to check if nsIEditor::flags has nsIPlaintextEditor::eEditorPasswordMask. E.g.:

> var isPasswordEditor =
>   !!(inputElement.QueryInterface(Ci.nsIDOMNSEditableElement).editor.flags &
>        Ci.nsIPlaintextEditor.eEditorPasswordMask);

Although, I don't know if the flag changed editors work fine. If some add-ons change the flag for canceling the mask in password field, this check may be better. If we need to make this feature for such add-ons, we should use this check. But I'm not sure if it's worth.

Comment 37

[Masatoshi Kimura [:emk]]

Is it really desiable to send the password to Google even if some add-ons remove the mask?

Comment 38

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #35)
> Created attachment 656764 [details] [diff] [review]
> update nsContextMenu for patch rev4
> 
> my proposal.
> -getBrowserSelection() should be designed as getting whatever browser
> selected text.
> -not showing #context-search on input[type="password"].

My explaining may misread a reader.
At patch rev4, #context-searchselect is not shown on input[type="password"] because the return value of getBrowserSelection() decides gContextMenu.isTextSelected and showing #context-searchselect (This is currently implementation).

This proposal patch aims is following:
1. Change getBrowserSelection() to get simply all browser selected text even if user selects a text in input[type="password"].
2. By effect (1), Move the part of handling to decide gContextMenu.isTextSelected & showing #context-searchselect to nsContextMenu.getTextSelection().(In reply to Masatoshi Kimura [:emk] from comment #37)
> Is it really desiable to send the password to Google even if some add-ons
> remove the mask?

(In reply to Masatoshi Kimura [:emk] from comment #37)
> Is it really desiable to send the password to Google even if some add-ons
> remove the mask?

I think that #context-searchselect should not be shown in input[type="password"].
So I'll think to implement this bug so that users cannot command "Search Google for" in input[type="password"].

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #38)
> (In reply to Masatoshi Kimura [:emk] from comment #37)
> > Is it really desiable to send the password to Google even if some add-ons
> > remove the mask?
> 
> I think that #context-searchselect should not be shown in
> input[type="password"].
> So I'll think to implement this bug so that users cannot command "Search
> Google for" in input[type="password"].

Good point. It makes sense. Although, input[type="text"] can be password field. But we must never meet such case.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 656763 [details] [diff] [review]
patch rev4

>+      return (elem instanceof HTMLTextAreaElement) ||
>+             ( (elem instanceof HTMLInputElement) && elem.mozIsTextField(false) );

The parentheses around elem instanceof Foo don't seem useful.

>+      selection = element.QueryInterface(Ci.nsIDOMNSEditableElement).
>+                  editor.selection.toString();

Skimming this patch made me look for a variable named "editor". Please make this more readable by moving the trailing dot from the first line to the beginning of the second line:

      selection = element.QueryInterface(Ci.nsIDOMNSEditableElement)
                         .editor.selection.toString();

>-<!ENTITY spellCheckEnable.accesskey "S">
>+<!ENTITY spellCheckEnable.accesskey "g">

This doesn't look like a change that's necessarily limited to en-US, which means that you need to change the entity name.

Comment 41

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39)
> Although, input[type="text"] can be password field. But we must never meet such case.

I think it may be very edge case. There is no the end point if we start to catch them...


(In reply to Dão Gottwald [:dao] from comment #40)
> Comment on attachment 656763 [details] [diff] [review]
> patch rev4
>
> >-<!ENTITY spellCheckEnable.accesskey "S">
> >+<!ENTITY spellCheckEnable.accesskey "g">
> 
> This doesn't look like a change that's necessarily limited to en-US, which
> means that you need to change the entity name.

Umm.... Do you have any suitable entity name...? I don't find it.

BTW, I don't know well about L10n. If we change a entity, Is finding it very hard? I seem that it will appear in Mercurial's changelog.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #40)
> >-<!ENTITY spellCheckEnable.accesskey "S">
> >+<!ENTITY spellCheckEnable.accesskey "g">
> 
> This doesn't look like a change that's necessarily limited to en-US, which
> means that you need to change the entity name.

How would this affect other locales?

Comment 43

[:Gavin Sharp [email: [e-mail]]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #33)
> I don't think it. If we remove to check type!=="password",
> getBrowserSelection() gets "●●●●●" when input[type="password"] has focus

You also need to pass true to mozIsTextField, which filters out password elements.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

I don't think getBrowserSelection should return selected text from password fields.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 656763 [details] [diff] [review]
patch rev4

Looks like there's a bunch of feedback here to address :)

Comment 46

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #42)
> (In reply to Dão Gottwald [:dao] from comment #40)
> > >-<!ENTITY spellCheckEnable.accesskey "S">
> > >+<!ENTITY spellCheckEnable.accesskey "g">
> > 
> > This doesn't look like a change that's necessarily limited to en-US, which
> > means that you need to change the entity name.
> 
> How would this affect other locales?

The same way it affects en-US: a new combination of menu items opening the door to new conflicts.

Comment 47

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Dão Gottwald [:dao] from comment #46)
> >-<!ENTITY spellCheckEnable.accesskey "S">
> >+<!ENTITY spellCheckEnable.accesskey "g">
> 
> The same way it affects en-US: a new combination of menu items opening the
> door to new conflicts.

Do you mean that the above new accesskey cannot resolve to conflict because the access key of #context-searchselect will conflict to the one of #context-showonlythisframe ?

Comment 48

[:Gavin Sharp [email: [e-mail]]]

He's saying that we're changing when menu items appear, so we're creating the potential for new conflicts, and that applies to all locales. He's right, but simply changing the entity name seems unlikely to be sufficient to alert localizers to this new potential conflict. I'm not sure how carefully they tend to look out for accesskey conflicts in context menus, given the numerous different possible combinations.

I guess the best option is to just change the name of contextMenuSearchText.* (contextMenuSearch.*?) and spellCheckEnable.* (spellCheckToggle.*?), and comment here accordingly so that those who check blame notice.

Comment 49

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #48)
> He's saying that we're changing when menu items appear, so we're creating
> the potential for new conflicts, and that applies to all locales. He's
> right, but simply changing the entity name seems unlikely to be sufficient
> to alert localizers to this new potential conflict. I'm not sure how
> carefully they tend to look out for accesskey conflicts in context menus,
> given the numerous different possible combinations.
> 
> I guess the best option is to just change the name of
> contextMenuSearchText.* (contextMenuSearch.*?) and spellCheckEnable.*
> (spellCheckToggle.*?), and comment here accordingly so that those who check
> blame notice.

Thank you for your extensive explanation. I understand.
I'll change entity names in the next patch.

Comment 50

[Axel Hecht [:Pike]]

Not sure actually.

Can we create a variant of our mozmill accesskey conflict check instead? I'm thinking of a dummy page with the typical constructs, fire the context menu programmatically, and see if the menu items have conflicting accesskeys.

Then they could report along with the tests for the pref dialog etc on http://mozmill-ci.blargon7.com/#/l10n/reports.

If I compare the existing accesskeys, http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=contextMenuSearchText.accesskey&find=browser&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-mozilla-aurora vs http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=spellCheckEnable.accesskey&find=toolkit&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-mozilla-aurora, it's 50/50 on where we introduce a regression.

Comment 51

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev5

try: https://tbpl.mozilla.org/?tree=Try&rev=9bea1aa9eb6f

I change entity names.

Comment 52

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev5.1

Rebased on latest mozilla-central.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 664899 [details] [diff] [review]
patch rev5.1

Sorry for the delay here - we should file a separate bug for creating that mozmill test to ensure that there are no conflicts in context menu access keys.

Comment 54

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #53)
> Comment on attachment 664899 [details] [diff] [review]
> patch rev5.1
> 
> Sorry for the delay here - we should file a separate bug for creating that
> mozmill test to ensure that there are no conflicts in context menu access
> keys.

Thank you for reviewing.
So,  Do we need create a new mozmill test before checking-in this patch?

Comment 55

[:Gavin Sharp [email: [e-mail]]]

No; I think we should just file a bug about it.

Comment 56

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I filed the new bug about mozmill test (bug 799149), and add "check-in needed" flag to this.

Comment 57

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3ebc9d46ac

Comment 58

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5a3ebc9d46ac

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Removing my name from in-qa-testsuite flag for a better query.