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

Issue 640355 link

Starred by 1 user
Status: Fixed
Owner:
ekaramad@chromium.org
Closed: Sep 2016
Cc:
kenrb@chromium.org
creis@chromium.org
█ nasko@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Dictionary pop-up does not work in OOPIF.

Project Member Reported by ekaramad@chromium.org, Aug 23 2016 
Version: <54.0.2833.0 (Official Build) canary (64-bit)
OS: Mac OSX

What steps will reproduce the problem?
(1) Run chrome with --site-per-process or alternatively, enable the --site-per-process flag in chrome://flags.
(2) Navigate chrome to a page with a cross-origin <iframe> with some text.
(3) Hover over the text inside OOPIF and press Command + Ctrl + D.

What is the expected output?
Dictionary should pop-up at the location of the word.

What do you see instead?
Dictionary pops-up empty and at an incorrect location (not next to word).

Comment 1 by ekaramad@chromium.org, Aug 23 2016 

I have reported another bug regarding the position of the dictionary in  issue 640353 .

The issue here is that we need to forward the TextInputClientMsgStart class of IPCs to the RenderWidget where the mouse position is right now (not the focused widget). Also, on the renderer side, TextInputClientObserver is owned by RenderViewImpl. This would not work for OOPIF since the render view uses mainFrameImpl() for hit testing.

Comment 2 by ekaramad@chromium.org, Aug 25 2016

Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 23 2016 

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

commit 330ba4235e6b32ccad99d48427bc8b7b2df16130
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Sep 23 17:57:47 2016

Implement Mac Pop-up Dictionary for OOPIF.

Ctrl+Command+D pops up a dictionary at current mouse location for the underlying text.
To implement this for OOPIF, the following changes where made:
+ Add a method to RenderWidgetHostInputEventRouter to return the RenderWidgetHost at the given location.
+ Move TextInputClientObserver logic to RenderFrameImpl
+ Pass both WebWidget and WebView to WebSubStringUtil so that it can query the string for both main frame and sub frames.

Ideally, TextInputClientObserver should belong to RenderWidget. But due to protected inheritance of WebViewImpl form
WebWidget, this would not work for main frame which still holds a WebViewImpl as its WebWidget.

BUG= 640355 

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

[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/browser/renderer_host/text_input_client_mac.mm
[add] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/browser/site_per_process_mac_browsertest.mm
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/public/test/DEPS
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/public/test/content_browser_test_utils.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/public/test/content_browser_test_utils_mac.mm
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/renderer/render_view_impl.cc
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/renderer/render_view_impl.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/renderer/render_widget.cc
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/renderer/render_widget.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/renderer/text_input_client_observer.cc
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/renderer/text_input_client_observer.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/content/test/BUILD.gn
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/WebFrameWidgetBase.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/WebFrameWidgetImpl.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/WebViewFrameWidget.cpp
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/WebViewFrameWidget.h
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/mac/WebSubstringUtil.mm
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/Source/web/tests/WebViewTest.cpp
[modify] https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130/third_party/WebKit/public/web/mac/WebSubstringUtil.h

Comment 4 by ekaramad@chromium.org, Sep 27 2016

Status: Fixed (was: Started)
I locally verified this on Canary. Dictionary is shown for the correct word and at the right location. Marking the bug as fixed.
►

Sign in to add a comment