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

RESOLVED FIXED in Firefox 19

Status

()

Firefox
Menus
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: John Wayne Hill (UX Intern), Assigned: tetsuharu)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

7 years ago
(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.
This is a simple fix to nsContextMenu.js
Component: General → Menus
QA Contact: general → menus
Whiteboard: [good first bug]

Updated

7 years ago
Depends on: 429244

Comment 2

7 years ago
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

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

Comment 4

7 years ago
(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?
Created attachment 469626 [details] [diff] [review]
Fix for the bug.

This patch should fix the issue described here.
Attachment #469626 - Flags: review?(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?
(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

7 years ago
Three months of inactivity =(

Comment 9

7 years ago
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?
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

7 years ago
Can someone with permissions mark the current patch obsolete?
It's not necessarily obsolete.

Comment 13

6 years ago
Created attachment 515706 [details] [diff] [review]
Patch to nsContextMenu.js

This is my attempt at patching nsContextMenu.

Updated

6 years ago
Attachment #515706 - Flags: review?(gavin.sharp)
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.
Attachment #515706 - Flags: review?(gavin.sharp) → review-

Comment 15

6 years ago
(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

6 years ago
Created attachment 515998 [details] [diff] [review]
Patch updated.

This should fix both issues pointed out.
Attachment #515706 - Attachment is obsolete: true
Attachment #515998 - Flags: review?(gavin.sharp)

Comment 17

6 years ago
Could this still make it into Firefox 5 or are we past the deadline for that?
Duplicate of this bug: 648971

Comment 19

6 years ago
(In reply to comment #16)
> attachment.is_obsolete: 0 => 1; flag: review?(gavin.sharp@gmail.com)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 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?
Attachment #515998 - Flags: review?(gavin.sharp) → review-
Created attachment 528007 [details] [diff] [review]
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.
Attachment #469626 - Attachment is obsolete: true
Attachment #515998 - Attachment is obsolete: true
Attachment #469626 - Flags: review?(jst)
It'd be nice to have some test coverage for this too. Something along the lines of browser_bug417483.js, maybe.
Duplicate of this bug: 686200

Comment 24

6 years ago
Any news on the progress of this bug?
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?
I just haven't had time to followup. If you'd like to take this bug and write a test, that'd be great!
Created attachment 653760 [details] [diff] [review]
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?
Attachment #528007 - Attachment is obsolete: true
Attachment #653760 - Flags: review?(gavin.sharp)
Use a different access key? :) Are there no more available letters or something?
Created attachment 654051 [details] [diff] [review]
patch with test rev2

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #653760 - Attachment is obsolete: true
Attachment #653760 - Flags: review?(gavin.sharp)
Attachment #654051 - Flags: review?(gavin.sharp)
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.
Attachment #654051 - Flags: review?(gavin.sharp)
Created attachment 656489 [details] [diff] [review]
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
Attachment #654051 - Attachment is obsolete: true
Attachment #656489 - Flags: review?(gavin.sharp)
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?
Attachment #656489 - Flags: review?(gavin.sharp) → feedback+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Created attachment 656763 [details] [diff] [review]
patch rev4
Attachment #656489 - Attachment is obsolete: true
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"].
(Assignee)

Updated

5 years ago
Attachment #656763 - Flags: review?(gavin.sharp)
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.
Is it really desiable to send the password to Google even if some add-ons remove the mask?
(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"].
(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 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.
(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.
(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?
(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.
I don't think getBrowserSelection should return selected text from password fields.
Comment on attachment 656763 [details] [diff] [review]
patch rev4

Looks like there's a bunch of feedback here to address :)
Attachment #656763 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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 ?
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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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

5 years ago
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.
Created attachment 657237 [details] [diff] [review]
patch rev5

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

I change entity names.
Attachment #656763 - Attachment is obsolete: true
Attachment #656764 - Attachment is obsolete: true
Attachment #657237 - Flags: review?(gavin.sharp)
Created attachment 664899 [details] [diff] [review]
patch rev5.1

Rebased on latest mozilla-central.
Attachment #657237 - Attachment is obsolete: true
Attachment #657237 - Flags: review?(gavin.sharp)
Attachment #664899 - Flags: review?(gavin.sharp)
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.
Attachment #664899 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?
No; I think we should just file a bug about it.
I filed the new bug about mozmill test (bug 799149), and add "check-in needed" flag to this.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3ebc9d46ac
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5a3ebc9d46ac
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19

Updated

5 years ago
Depends on: 799596
Flags: in-qa-testsuite?(hskupin)

Updated

4 years ago
Duplicate of this bug: 360331
Removing my name from in-qa-testsuite flag for a better query.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.