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

Issue 640502 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocked on:
issue 671055


Show other hotlists

Hotlists containing this issue:
MacViews-Task-Queue


Sign in to add a comment

MacViews: Support Mac dictionary popup.

Project Member Reported by karandeepb@chromium.org, Aug 24 2016

Issue description

Version: 54.0.2838.0
OS: Mac

What steps will reproduce the problem? 
(1) Enable chrome://flags/#mac-views-native-dialogs
(2) Open bookmark bubble
(3) Hover over the "Name" label.
(4) Press Ctrl+Command+D.
(5) Dictionary popup should open for the word "Name", but it doesn't.
(6) In the Name textfield, enter the words "hello bye"
(7) Place the cursor at "bye". 
(8) Press Ctrl+Command+D.
(9) Dictionary popup should show for "bye", instead it shows up for "hello". Also, its location is incorrect.

Should be possible to do this for textfields. Not sure how labels can be supported.
 

Comment 1 by pal...@gmail.com, Aug 24 2016

Just tried in chrome 53.0.2785.70 beta 64bit and cannot reproduce it, seems to be a dev specific bug.
You'll need to enable chrome://flags/#mac-views-native-dialogs and restart the browser to reproduce.
Owner: karandeepb@chromium.org
Status: Assigned (was: Available)
Investigated this a bit. Its easy to support for Textfield currently, but for labels it's not that trivial. 

One way is to implement characterIndexForPoint:. But that's not the best way since, Cocoa will then call firstRectForCharacterRange: to display the popup. So we'll need to maintain accounting of the target View used in characterIndexForPoint.

A better way is to use quickLookWithEvent: along with showDefinitionForAttributedString:atPoint: (This is how it's done in the Chrome renderer), since this way the popup display logic won't be spread across multiple functions. Then we won't need to implement characterIndexForPoint:. Also, labels will need to support operations like getWordAtPoint.

Open question- what else uses characterIndexForPoint?
Cc: rsesek@chromium.org pinkerton@chromium.org
How do we handle supporting this for web content? Couldn't we do the same thing?

+rsesek who I think did a lot of that work. 
Labels: OS-Mac
Yeah we can do the same thing as we do for Web contents, as far as the interaction with Cocoa is concerned. But for MacViews, we'll have to retrieve the textual information from the underlying "View" and the support for that has to be added. Also, for Views, everything is synchronous, which should make the things a bit easier than the Web contents implementation.
Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 6 2016

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

commit a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Oct 06 03:22:51 2016

MacViews: Implement Force Touch/Mac dictionary lookup for Textfields.

This CL adds support to MacViews for Force touch and dictionary lookups. This is
achieved by implementing quickLookWithEvent: on the BridgedContentView. A new
interface called WordLookupClient is added which is to be implemented by views
which support word lookups. Currently only views::Textfield implements this
interface. A virtual method GetWordLookupClient is added to views::View to
retrieve the WordLookupClient associated with a given View. Support is added to
the RenderText class to retreive the word displayed at a given point along with
its styling information. A DecoratedText class is also introduced to encapsulate
the styling information for some given text.

Note, this CL only adds support for dictionary lookups to Views::Textfields.

Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-vns/edit?usp=sharing.
BUG= 640502 
TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble.
Enter some text in the name textfield. Hover over the text and press
Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of
the word, the mouse was hovering on.

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

[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/BUILD.gn
[add] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/decorated_text.cc
[add] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/decorated_text.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_mac.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_mac.mm
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/BUILD.gn
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/controls/textfield/textfield_model.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/view.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/view.h
[add] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/word_lookup_client.h

Labels: TE-Verified-M55 TE-Verified-55.0.2883.6
Verified this issue on Mac OS 10.12 using chrome latest Dev #55.0.2883.6 by following steps mentioned in the original comment. By pressing Ctrl+Command+D observed Dictionary popup shown only for bye as expected. Hence adding TE-Verified label.
640502.mp4
1.0 MB View Download
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a

commit a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Oct 06 03:22:51 2016

MacViews: Implement Force Touch/Mac dictionary lookup for Textfields.

This CL adds support to MacViews for Force touch and dictionary lookups. This is
achieved by implementing quickLookWithEvent: on the BridgedContentView. A new
interface called WordLookupClient is added which is to be implemented by views
which support word lookups. Currently only views::Textfield implements this
interface. A virtual method GetWordLookupClient is added to views::View to
retrieve the WordLookupClient associated with a given View. Support is added to
the RenderText class to retreive the word displayed at a given point along with
its styling information. A DecoratedText class is also introduced to encapsulate
the styling information for some given text.

Note, this CL only adds support for dictionary lookups to Views::Textfields.

Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-vns/edit?usp=sharing.
BUG= 640502 
TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble.
Enter some text in the name textfield. Hover over the text and press
Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of
the word, the mouse was hovering on.

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

[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/BUILD.gn
[add] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/decorated_text.cc
[add] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/decorated_text.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_mac.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_mac.mm
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/BUILD.gn
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/controls/textfield/textfield_model.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/view.cc
[modify] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/view.h
[add] https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a/ui/views/word_lookup_client.h

Comment 12 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
Blocking: 603386
Cc: spqc...@chromium.org
Labels: -Pri-2 Pri-3
This may have regressed at some point. Maybe selection controller stuff.

Also we should highlight the word under the cursor when right-clicking (I think karan was already thinking about that).

Note it's unlikely this is actually used much in the UI :/.

Also... is this broken in Canary for the Cocoa browser? I just tried on 57.0.2948.0 and it wasn't working in the omnibox, but it did work in 56.0.2914.3.

(QuickLook is full of bugs :| looking at you Issue 668694...)
>> Also we should highlight the word under the cursor when right-clicking (I think karan was already thinking about that).

There is  issue 657556  for that.

>>Also... is this broken in Canary for the Cocoa browser? I just tried on 57.0.2948.0 and it wasn't working in the omnibox, but it did work in 56.0.2914.3.

But the omnibox will be a Cocoa NSText{Field/View} right? It works correctly for me in both the omnibox and for Views::Textfield (like the bookmark bubble) on Chrome 57.0.2948.0.

>>>> Also... is this broken in Canary for the Cocoa browser? I just tried on 57.0.2948.0 and it wasn't working in the omnibox, but it did work in 56.0.2914.3.

>> But the omnibox will be a Cocoa NSText{Field/View} right? It works correctly for me in both the omnibox and for Views::Textfield (like the bookmark bubble) on Chrome 57.0.2948.0.

So for me... it's weird. Cmd+Cmd+d works everywhere (m56, m57, omnibox, views::TextField, Cocoa, Views).

Cocoa+Omnibox: Force touch works in Chrome/Official/Dev. Does NOT work in any Canary/Chromium build.  [I think we disabled it in the omnibox at some point? Dunno why it still works for me :/ -> aha - r356086 is "Disable Force Touch lookup in the Omnibox." - that's m48! ]

Cocoa+NSTextField: Force touch works.

views::TextField: Ctrl+Cmd+d always works, but force-touch never does.

We probably just need to wire up that mouse event, similar to how WebContents does it. (but also check in mac_views_browser whether we have/want it in the omnibox - it was disabled because of: {crashes, other reasons} which may or may not be a problem - see  Issue 544250 )
For MacViews at least, I remember it worked earlier. It seems there has been some regression. Will take a look.
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 19 2016

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

commit 8de9c34b9a48e07607bed42939e32ab2f4b79558
Author: karandeepb <karandeepb@chromium.org>
Date: Mon Dec 19 06:09:16 2016

MacViews: Perform word lookup on force touch.

r368481 added support for force touch to the BaseView and r423419 implemented
dictionary lookup for views::Textfield on MacViews. This CL implements the
method forceTouchEvent: on the BridgedContentView. As a result, force touching
on a views::Textfield now performs dictionary lookup for the word under the
cursor as per the user's system preferences.

BUG= 640502 
TEST= Enable chrome://flags/#secondary-ui-md. Open Bookmark Bubble. Enter some
text in the name textfield. Force touch on one of the words in the text. Verify
that a dictionary popup appears showing the definition of the word under the
cursor.

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

[modify] https://crrev.com/8de9c34b9a48e07607bed42939e32ab2f4b79558/ui/views/cocoa/bridged_content_view.mm

Status: Force touch should now work. Remaining: Enable word lookup for labels and make it support multiline.
Blockedon: 671055
Components: -UI>OSIntegration Internals>PlatformIntegration
Deprecating UI>OSIntegration in favor of the more generic Internals>PlatformIntegration
Project Member

Comment 21 by bugdroid1@chromium.org, Jan 23 2017

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

commit 666e4f0298298427d2026c2896466951f199f5bf
Author: karandeepb <karandeepb@chromium.org>
Date: Mon Jan 23 01:57:27 2017

MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG= 640502 , 671055 

TEST=Run out/Default/views_examples_with_content_exe with the flag --enable-
harfbuzz-rendertext. Go to the Label section. Ensure word lookup works correctly
on pressing Ctrl+Command+D for selectable labels.

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

[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/gfx/render_text.cc
[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/gfx/render_text.h
[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/views/controls/label.cc
[modify] https://crrev.com/666e4f0298298427d2026c2896466951f199f5bf/ui/views/controls/label.h

Blocking: -603386
Owner: ----
Status: Available (was: Started)
These are now supported for Textfields and selectable labels (with RenderTextHarfBuzz). In the future we can look to support them for TreeView etc. as well. 

This should not block any issue now.
Labels: -TE-Verified-M55 -TE-Verified-55.0.2883.6 MacViews-Controls
Labels: M-64
This bug is still live, and should be fixed before the MacViews launch.
Cc: shrike@chromium.org
ellyjones@ - I was just poking around the remaining MacViews Harmony bugs. What part of this bug is still live? I'm not on my laptop but it seems like dictionary lookup is working: if I Control-Cmd D in the bookmarks textfield the word under my mouse gets looked up. Unfortunately, though, pressing Esc to dismiss the dictionary window also dismisses the bookmarks dialog.

Cc: ellyjo...@chromium.org
+ellyjones@, to see c#25.
Status: Fixed (was: Available)
#25: hm, according to the comment in #22, it doesn't work in TreeViews, and we have an editable TreeView in the bookmarks editor, but I just tried it myself and it does work, so this is actually Fixed.

Sign in to add a comment