New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 617436 link

Starred by 4 users
Status: Fixed
Owner:
█ spqc...@chromium.org
Last visit > 30 days ago
Closed: Mar 2018
Cc:
ellyjo...@chromium.org
█ shrike@chromium.org
mac-bugs-priority@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocking:
issue 168501
issue 603386



Sign in to add a comment

MacViews textfields use generic context menu?

Project Member Reported by shrike@chromium.org, Jun 5 2016 
Version: 53.0.2758.0 Canary
OS: 10.11

What steps will reproduce the problem?
(1) With webui MacView dialogs enabled, navigate to http://apted.net/chat
(2) In the dialog that appears, type some text in the Name textfield and control-click it

What is the expected output?
I expect to see the "text" contextual menu, which contains special contextual menu commands and menus like "Spelling and Grammar"

What do you see instead?
I see the generic contextual menu.

NSText or NSTextView puts together this enhanced contextual menu.

Note that the "Search with Google" command should search using Google Chrome.
StandardContextualMenu.png
92.0 KB View Download
TextContextualMenu.png
143 KB View Download

Comment 1 by spqc...@chromium.org, Jun 15 2016

Owner: spqc...@chromium.org

Comment 2 by spqc...@chromium.org, Jun 15 2016

Status: Assigned (was: Available)

Comment 3 by tapted@chromium.org, Jun 16 2016 

Yeah this is a good thing to look at next. I explored a couple of things in https://codereview.chromium.org/2072653002, but really only to map out a possible solution.

I think a good first step is to get parity with renderer context menus -- this is the stuff in chrome/browser/ui/cocoa/renderer_context_menu. It needs to be pulled out into a component with no dependencies on src/content -- it can probably live in ui/base/cocoa. RenderViewContextMenuMac probably still needs to exist, but speech_submenu_model_ and bidi_submenu_model_ (and probably other things -- everything that doesn't need src/content basically) should be owned by a new class in ui/base/cocoa, say TextContextMenuMac, which is constructed with a TextContextMenuMacDelegate (which RenderViewContextMenuMac and a new class in ui/views/cocoa inherit from). Then, on mac, Textfield::UpdateContextMenu() in textfield.cc should gather extra items from the speech and bibdi menu models on a TextContextMenuMac.

TextContextMenuMac should try to implement as much of the functionality as it can using interfaces on ui::TextInputClient -- textfield.cc is a TextInputClient, TextContextMenuMacDelegate just needs a GetInputClient method which, for MacViews, is a views::Textfield. Things like startSpeaking are just [NSApp speakString:base::SysUTF8ToNSString(delegate_->GetInputClient()->GetTextFromRange(...) )];


Once we have parity with renderer_context_menu we can then do really cool stuff like actually implementing text substitutions for the WebContent area (without duplicating effort), which is a 6-year old feature request -- Issue 42434.


I explored a couple of possible shortcuts in https://codereview.chromium.org/2072653002, but I think they are dead-ends. E.g. AppKit provides an `NSText` virtual interface with some tantalizing methods that look like they could give nice integration without a full-blown NSTextField, but it seems to be pretty useless.

There's also a possibility of populating the menu model with things from `[NSTextView defaultMenu]` on a semi-automatic basis -- we could then just hook up the responder messages appropriately in BridgedContentView, (or possibly a shim view that implements NSText). We might even be able to steal a few localized strings this way. But I don't think it will work for renderer_context_menu and ultimately we want to consolidate these two codepaths so that the WebContent gets all the benefits that its textfields currently lack in this respect.
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 7 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by spqc...@chromium.org, Jul 28 2016 

Based on what's currently available on context menu for the web content.
I'm going to add the following items for now:
- Spelling & Grammar
- Speech
- Lookup
- Search With Google

Once that's in, I might explore to see how to add other items like "Substitution" and "Transformation", on both the web and MacViews context menus

I noticed that there are currently issues with the Spelling Suggestions. What is the status for those?

Comment 6 by tapted@chromium.org, Jul 29 2016 

What are the issues? I've seen one that points to some potential flakiness with the 'Ask Google For Suggestions' option, but otherwise things seem ok.

Comment 7 by spqc...@chromium.org, Jul 29 2016 

 Issue 627533 , but looking into it, I don't think it should impact the MacViews work

Comment 8 by tapted@chromium.org, Dec 8 2016

Blocking: 603386
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 7 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/375ddabe9cdc45673e112c5b1b06cc754aa45759

commit 375ddabe9cdc45673e112c5b1b06cc754aa45759
Author: spqchan <spqchan@chromium.org>
Date: Tue Feb 07 02:41:29 2017

[MacViews] Implemented text context menu

- Added Speech, Writing Direction and Look Up
- Introduced text_services_context_menu to create and handle the items
- Moved the the code from render_view_context_menu_mac into text_services_context_menu
- Created views_text_context_menu to bridge Views with text_services_context_menu

BUG= 617436 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2164483006
Cr-Commit-Position: refs/heads/master@{#448526}

[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/chrome/app/chrome_command_ids.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/chrome/app/generated_resources.grd
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/frame_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/test/test_render_view_host.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/content/test/test_render_view_host.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/base/BUILD.gn
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/base/cocoa/text_services_context_menu.cc
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/base/cocoa/text_services_context_menu.h
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/base/cocoa/text_services_context_menu_unittest.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/gfx/BUILD.gn
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/gfx/decorated_text_mac.h
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/gfx/decorated_text_mac.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/gfx/render_text.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/gfx/render_text.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/strings/ui_strings.grd
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/BUILD.gn
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/label.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/label.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/textfield/textfield_test_api.h
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/textfield/textfield_unittest.cc
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/views_text_services_context_menu.cc
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/views_text_services_context_menu.h
[add] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/controls/views_text_services_context_menu_mac.mm
[modify] https://crrev.com/375ddabe9cdc45673e112c5b1b06cc754aa45759/ui/views/word_lookup_client.h

Comment 10 by spqc...@chromium.org, Feb 9 2017

Status: Fixed (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 10 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87

commit 6dcf90cf6c047e222f9da8d2567b60d66fa2fe87
Author: jonross <jonross@chromium.org>
Date: Fri Feb 10 17:15:37 2017

Revert [MacViews] Implemented text context menu

This change reverts the implementation of TextField context menus.

The original feature was targetting MacViews, however this breaks menus on non
Mac platforms.

Textfield::OnCaretBoundsChanged deletes the MenuModel while the MenuController
is still active. Subsequent UI events attempt to access the now destroyed model.

This is causing two major crashes on dev channel. So I am reverting to unblock.

TBR=sky@chromium.org, nasko@chromium.org

Revert "[MacViews] Implemented text context menu"

This reverts commit 375ddabe9cdc45673e112c5b1b06cc754aa45759.
Original review: https://codereview.chromium.org/2164483006

BUG= 617436 , 690097, 690443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2688883002
Cr-Commit-Position: refs/heads/master@{#449645}

[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/chrome/app/chrome_command_ids.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/chrome/app/generated_resources.grd
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/frame_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/test/test_render_view_host.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/content/test/test_render_view_host.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/base/BUILD.gn
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/base/cocoa/text_services_context_menu.cc
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/base/cocoa/text_services_context_menu.h
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/base/cocoa/text_services_context_menu_unittest.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/gfx/BUILD.gn
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/gfx/decorated_text_mac.h
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/gfx/decorated_text_mac.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/gfx/render_text.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/gfx/render_text.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/strings/ui_strings.grd
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/BUILD.gn
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/controls/label.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/controls/label.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/controls/textfield/textfield_test_api.h
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/controls/textfield/textfield_unittest.cc
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/views/controls/views_text_services_context_menu.cc
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/views/controls/views_text_services_context_menu.h
[delete] https://crrev.com/bba2078bfd6da7ee4df5cd3b3974ff37a39d46e6/ui/views/controls/views_text_services_context_menu_mac.mm
[modify] https://crrev.com/6dcf90cf6c047e222f9da8d2567b60d66fa2fe87/ui/views/word_lookup_client.h
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/671ec2e35979d2656757bc3771af54c6190efb91

commit 671ec2e35979d2656757bc3771af54c6190efb91
Author: tapted <tapted@chromium.org>
Date: Mon Feb 13 00:02:45 2017

nit: Use Title Case for "Select All" in views::Textfield context menus on Mac.

It currently isn't (and there's now "Look Up", which is).

BUG= 617436 

Review-Url: https://codereview.chromium.org/2687223002
Cr-Commit-Position: refs/heads/master@{#449900}

[modify] https://crrev.com/671ec2e35979d2656757bc3771af54c6190efb91/ui/strings/ui_strings.grd

Comment 13 by shrike@chromium.org, Apr 3 2017

Labels: -Pri-2 -M-54 M-60 OS-Mac Pri-1
Status: Started (was: Fixed)
Reopening (change was reverted without the bug being reopened). Marking P1 because we want for shipping MacViews.

Does this change add a "Search in Google" item? It should, and it should search using Chrome, not Safari.

Also, does this change apply to the menus an uneditable-but-selectable textfield displays? Assuming yes for now (if not, let me know and I will file a new bug for that, because currently those menus are also generic).

Comment 14 by shrike@chromium.org, Apr 3 2017

Blocking: 168501

Comment 15 by spqc...@chromium.org, May 8 2017 

Issues on why this was reverted:

crbug.com/690443
 crbug.com/690331 
 crbug.com/690586 

Putting them here for reference

Comment 16 by shrike@chromium.org, May 22 2017

Labels: -M-60 M-61

Comment 17 by ellyjo...@chromium.org, Feb 14 2018

Labels: MacViews-Controls
spqchan: what's up with this?

Comment 18 by spqc...@chromium.org, Feb 14 2018 

I landed a CL but it got reverted since it was causing some major issues. I have yet to reland this with the issues fixed

Comment 19 by ellyjo...@chromium.org, Feb 14 2018 

Okay - are you likely to have a chance to look at this soon? The branch point for M66 is March 1.

Comment 20 by spqc...@chromium.org, Feb 14 2018 

From an offline chat: I'm looking into that now
Project Member

Comment 21 by bugdroid1@chromium.org, Feb 21 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e29cccfc7674503a948faeed4d7a9ab15574b80

commit 6e29cccfc7674503a948faeed4d7a9ab15574b80
Author: spqchan <spqchan@chromium.org>
Date: Wed Feb 21 18:40:17 2018

[MacViews] Add Speech to the Textfield Context Menu

- Added Speech items to the MacViews textfield context menu
- Created views_text_context_menu to bridge Views with
  text_services_context_menu

Overview CL:
https://chromium-review.googlesource.com/c/chromium/src/+/923548

This is a reland of https://codereview.chromium.org/2164483006

Bug:  617436 
Change-Id: I0c9be96455ac34702caddf6ac8d0d93a36e69abd
Reviewed-on: https://chromium-review.googlesource.com/926962
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538158}
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/chrome/app/generated_resources.grd
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/base/BUILD.gn
[add] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/base/cocoa/text_services_context_menu.cc
[add] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/base/cocoa/text_services_context_menu.h
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/strings/ui_strings.grd
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/views/BUILD.gn
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/views/controls/textfield/textfield.h
[add] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/views/controls/views_text_services_context_menu.cc
[add] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/views/controls/views_text_services_context_menu.h
[add] https://crrev.com/6e29cccfc7674503a948faeed4d7a9ab15574b80/ui/views/controls/views_text_services_context_menu_mac.mm
Project Member

Comment 22 by bugdroid1@chromium.org, Feb 28 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f869e9ea375f53b1be7f30485404110f9e4b0776

commit f869e9ea375f53b1be7f30485404110f9e4b0776
Author: spqchan <spqchan@chromium.org>
Date: Wed Feb 28 02:38:20 2018

[MacViews] Add BiDi to the Textfield Context Menu

Added the Bidirection submenu to the MacViews textfield context menu

Overview CL:
https://chromium-review.googlesource.com/c/chromium/src/+/923548

This is a reland of https://codereview.chromium.org/2164483006

Bug:  617436 
Change-Id: I8de831e72373de6583161df792472eeb29f37a5b
Reviewed-on: https://chromium-review.googlesource.com/929668
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539670}
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/chrome/app/generated_resources.grd
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/base/cocoa/text_services_context_menu.cc
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/base/cocoa/text_services_context_menu.h
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/strings/ui_strings.grd
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/views/controls/textfield/textfield_test_api.cc
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/views/controls/textfield/textfield_test_api.h
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/views/controls/textfield/textfield_unittest.cc
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/views/controls/views_text_services_context_menu.cc
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/views/controls/views_text_services_context_menu.h
[modify] https://crrev.com/f869e9ea375f53b1be7f30485404110f9e4b0776/ui/views/controls/views_text_services_context_menu_mac.mm
Project Member

Comment 23 by bugdroid1@chromium.org, Mar 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/941ff581649afe136b7f4db6c1a9e82672a966b8

commit 941ff581649afe136b7f4db6c1a9e82672a966b8
Author: spqchan <spqchan@chromium.org>
Date: Fri Mar 09 02:02:37 2018

[MacViews] Add Lookup in the Textfield Context Menu

Added the Lookup menu item to the textfield context menu.

Overview CL:
https://chromium-review.googlesource.com/c/chromium/src/+/923548

This is a reland of https://codereview.chromium.org/2164483006

Bug:  617436 
Change-Id: I6a84f8f5611de48bee05160b719cb2cc8c040a87
Reviewed-on: https://chromium-review.googlesource.com/941733
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541988}
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/gfx/BUILD.gn
[add] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/gfx/decorated_text_mac.h
[add] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/gfx/decorated_text_mac.mm
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/gfx/render_text.cc
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/gfx/render_text.h
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/label.cc
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/label.h
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/textfield/textfield_unittest.cc
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/views_text_services_context_menu.h
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/controls/views_text_services_context_menu_mac.mm
[modify] https://crrev.com/941ff581649afe136b7f4db6c1a9e82672a966b8/ui/views/word_lookup_client.h

Comment 24 by spqc...@chromium.org, Mar 12 2018

Labels: Merge-Request-66
Merge request for the CL in #23. The CL is low risk and has been verified on Canary
Project Member

Comment 25 by sheriffbot@chromium.org, Mar 12 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: There is .grd file changes and we are only 35 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 26 by gov...@chromium.org, Mar 12 2018 

Thank you spqchan@. Could you pls provide merge justification why it is needed?

Comment 27 by spqc...@chromium.org, Mar 12 2018 

This is a feature that needs to be shipped with the MacViews browser. Note: the CL in #23 doesn't actually change the .grd file.

Comment 28 by gov...@chromium.org, Mar 12 2018

Labels: -Merge-Review-66 Merge-Approved-66
Approving merge for cl listed at #23 to M66 branch 3359 based on comment #24 and #27. Please merge ASAP.
Project Member

Comment 29 by bugdroid1@chromium.org, Mar 13 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5

commit 645bd3e8c31c73258b236e37a3f8eea41ba2d3d5
Author: spqchan <spqchan@chromium.org>
Date: Tue Mar 13 00:40:08 2018

[MacViews] Add Lookup in the Textfield Context Menu

Added the Lookup menu item to the textfield context menu.

Overview CL:
https://chromium-review.googlesource.com/c/chromium/src/+/923548

This is a reland of https://codereview.chromium.org/2164483006

(cherry picked from commit 941ff581649afe136b7f4db6c1a9e82672a966b8)

Bug:  617436 
Change-Id: I6a84f8f5611de48bee05160b719cb2cc8c040a87
Reviewed-on: https://chromium-review.googlesource.com/941733
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#541988}
Reviewed-on: https://chromium-review.googlesource.com/959702
Reviewed-by: Sarah Chan <spqchan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#185}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/gfx/BUILD.gn
[add] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/gfx/decorated_text_mac.h
[add] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/gfx/decorated_text_mac.mm
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/gfx/render_text.cc
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/gfx/render_text.h
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/label.cc
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/label.h
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/textfield/textfield_unittest.cc
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/views_text_services_context_menu.h
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/controls/views_text_services_context_menu_mac.mm
[modify] https://crrev.com/645bd3e8c31c73258b236e37a3f8eea41ba2d3d5/ui/views/word_lookup_client.h
Project Member

Comment 30 by bugdroid1@chromium.org, Mar 14 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd02d61c7a2eb98543da651173025d84515d61ee

commit fd02d61c7a2eb98543da651173025d84515d61ee
Author: spqchan <spqchan@chromium.org>
Date: Wed Mar 14 18:34:26 2018

Refactor RenderViewContextMenuMac

Remove the Speech and BiDI logic in RenderWidgetHostViewMac and
use the logic TextServiceContextMenu instead.

Overview CL:
https://chromium-review.googlesource.com/c/chromium/src/+/923548

This is a reland of https://codereview.chromium.org/2164483006

BUG:  617436 
Change-Id: I56b7427342951fea5c7f9ef25cb3b6c98e8addd8
Reviewed-on: https://chromium-review.googlesource.com/957869
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543137}
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/chrome/app/chrome_command_ids.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/browser/renderer_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/test/test_render_view_host.cc
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/content/test/test_render_view_host.h
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/ui/base/cocoa/text_services_context_menu.cc
[modify] https://crrev.com/fd02d61c7a2eb98543da651173025d84515d61ee/ui/base/cocoa/text_services_context_menu.h

Comment 31 by ellyjo...@chromium.org, Mar 23 2018

Labels: -Hotlist-Merge-Review -Phase2 -MovedFrom-53 -M-61 -merge-merged-3359 Target-67
spqchan: where are we on this? Let's target getting the rest of this done in M67.

Comment 32 by spqc...@chromium.org, Mar 23 2018

Status: Fixed (was: Started)

Comment 33 by susan.boorgula@chromium.org, Apr 6 2018

Labels: Needs-Feedback
Unable to reproduce the issue with steps mentioned in comment #0 as we don't have the credentials to login to the site http://apted.net/chat.
Hence unable to verify the fix on latest canary 67.0.3390.0. 

Attaching the screen shot for reference.

spqchan@ Request you to help in verifying the fix on the latest M-67 build.

Thanks..
617436.png
111 KB View Download

Comment 34 by spqc...@chromium.org, Apr 8 2018 

You don't need to log in the website. Anyway, I can verify that the fix is working
►

Sign in to add a comment