MacViews: Support Mac dictionary popup. |
|||||||||||||||||
Issue descriptionVersion: 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.
,
Aug 25 2016
You'll need to enable chrome://flags/#mac-views-native-dialogs and restart the browser to reproduce.
,
Aug 25 2016
,
Aug 30 2016
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?
,
Aug 31 2016
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.
,
Aug 31 2016
,
Sep 2 2016
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.
,
Sep 20 2016
,
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
,
Oct 10 2016
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.
,
Oct 27 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
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Dec 12 2016
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...)
,
Dec 12 2016
>> 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.
,
Dec 13 2016
>>>> 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 )
,
Dec 13 2016
For MacViews at least, I remember it worked earlier. It seems there has been some regression. Will take a look.
,
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
,
Dec 19 2016
Status: Force touch should now work. Remaining: Enable word lookup for labels and make it support multiline.
,
Dec 19 2016
,
Jan 2 2017
Deprecating UI>OSIntegration in favor of the more generic Internals>PlatformIntegration
,
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
,
Jan 23 2017
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.
,
Apr 12 2017
,
Oct 4 2017
This bug is still live, and should be fixed before the MacViews launch.
,
Oct 12 2017
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.
,
Oct 12 2017
+ellyjones@, to see c#25.
,
Oct 12 2017
#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 |
|||||||||||||||||
Comment 1 by pal...@gmail.com
, Aug 24 2016