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

Issue metadata

Status: Fixed
Owner:
OOO (7/13 - 7/23)
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 485650
issue 455497

Blocking:
issue 475718



Sign in to add a comment

Needs spec: Coordinates system used in ui::TextInputClient

Project Member Reported by yukawa@chromium.org, Apr 5 2014

Issue description

Version: as of r243050
OS: All

As of r243050, we don't have a clear spec about whether ui::TextInputClient uses DIP (Density Independent Pixel) or not.
https://src.chromium.org/viewvc/chrome/trunk/src/ui/base/ime/text_input_client.h?revision=243050

The lack of spec makes it harder to support HiDPI in a platform independent way.
This issue is spun off from  Issue 260529 .
 
Can you describe the exact problem you have? (In many platforms, you don't have to worry about it)

Won't the following help?

gfx::Screen::GetScreenFor(attached_window)->GetDisplayNearestWindow(attached_window).device_scale_factor()


Can someone educate me as to the ozone issues here?

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 8 2014

------------------------------------------------------------------
r262473 | yukawa@chromium.org | 2014-04-08T19:30:39.503921Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/base/ime/input_method_win.cc?r1=262473&r2=262472&pathrev=262473
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/base/ime/text_input_client.h?r1=262473&r2=262472&pathrev=262473

Treats the returned value from ui::TextInputClient as DIP

On Windows, the returned value from ui::TextInputClient as DIP is supposed to be DIP (Density Independent Pixel). This CL adds relevant coordinate conversion so that IME UI can be placed at the proper position when  HiDPI is enabled on Windows.

This CL does not cover Win8 (Ash) mode.

BUG= 260529 ,  360334 
TEST=Manually tested on Windows 8.1 w/ and w/o HiDPI is enabled.

Review URL: https://codereview.chromium.org/227073002
-----------------------------------------------------------------

Comment 4 by spang@chromium.org, Jun 6 2014

Labels: -Proj-Ozone
Cc: osh...@chromium.org yukawa@chromium.org rjkroege@chromium.org
Oshima-san,
Sorry, I missed your comment in #1.

Basically this is a design/documentation TODO for IME folks.

Background:
As a common requirement for IME support, each implementation class of ui::InputMethod is likely to be required to return some character positions rendered in Chromium to the input method framework, in the screen coordinates.  On the other hand, ui::TextInputClient is responsible for providing this sort of positional information.  Here we have two options regarding device_scale_factor.

A. ui::TextInputClient is responsible for returning positions in DIP and ui::InputMethod* should take the scale factor into consideration before providing it to the input method framework.
B. ui::TextInputClient is responsible for taking the scale factor into consideration and ui::InputMethod* should be able to pass it to the input method framework as is.

I'm not sure which way we will choose finally but I choose the plan A in my r262473 as a tentative workaround for Windows HiDPI support.  I filed this issue as a remainder for future maintainer of IME support in Chromium.

--------
In reply to #2.
I don't know how Ozone handles device scale but it would be great if changes for  Issue 362698  and/or  Issue 143619  will not break HiDPI support on Windows.

Comment 6 by osh...@chromium.org, Jun 10 2014

#5, I still don't understand the issue (the problem you have now). Most of chrome code doesn't (and should not have to) care about scale factor because the conversion is done behind the scenes. There are still a few cases that requires scale factor knowledge, but it's hard for me to understand without concrete example.

Do you have a specific example that is not working correctly?

Comment 7 by yukawa@chromium.org, Jun 11 2014

Sorry for my unclear explanation.  As filed as  Issue 260529 , which was linked in the comment #1, MS-IME had been showing its candidate window at wrong place until I introduced gfx::win::DIPToScreenRect in ui/base/ime/input_method_win.cc.

See  Issue 260529  and r262473 about why I had to use gfx::win::DIPToScreenRect there.  Note that that code is responsible for providing the caret/character positions to the OS and IMEs in screen coordinates.  It should be also noted that the IMEs are not a part of Chromium except for Chromium OS.  These IMEs are not aware of Chrome-internal scale factor therefore we should finish coordinate conversion somewhere before the caret/character positions are received by the IMEs.

I totally agree that chrome code should not have to care about scale factor. Actually I'd like to get rid of gfx::win::DIPToScreenRect from ui/base/ime/input_method_win.cc if possible.  Is it possible?

Comment 8 by yukawa@chromium.org, Apr 22 2015

Blockedon: chromium:455497
Cc: shuchen@chromium.org jdduke@chromium.org
Labels: Needs-Feedback
Owner: bokan@chromium.org
Status: Assigned
Hi David. I have a hard time understanding the impact of your http://crrev.com/967213004 on the (undocumented) spec of ui::TextInputClient.

You have changed the comment for firstRectForCharacterRange but no one has updated its consumer functions. Actually ui::TextInputClient::GetCompositionCharacterBounds is one of such consumers.

https://chromium.googlesource.com/chromium/src/+/b676ea032a5e05c979b760600bf8f5bdb46ceaeb/ui/base/ime/text_input_client.h#88
  // Retrieves the composition character boundary rectangle in the universal
  // screen coordinates. The |index| is zero-based index of character position
  // in composition text.
  // Returns false if there is no composition text or |index| is out of range.
  // The |rect| is not touched in the case of failure.
  // Note: On Windows, the returned value is supposed to be DIP
  // (Density Independent Pixel).
  // TODO(ime): Have a clear spec whether the returned value is DIP or not.
  //  http://crbug.com/360334 
  virtual bool GetCompositionCharacterBounds(uint32 index,
                                             gfx::Rect* rect) const = 0;

It's OK to update the comment here to whatever correct, but I wanted to understand what you tried to address in  Issue 455497  to avoid further confusion.

Thanks!

Comment 9 by yukawa@chromium.org, Apr 22 2015

Summary: Needs spec: Coordinates system used in ui::TextInputClient (was: Needs a spec of whether ui::TextInputClient uses DIP (Density Independent Pixel) or not)

Comment 10 by bokan@chromium.org, Apr 22 2015

Hi yukawa@,

My change was actually fixing  issue 455497 , basically, the firstRectForCharacterRange was returning a rect in the "root frame" coordinates. Until --enable-pinch-virtual-viewport was turned on, this was the same coordinate space as the viewport (what used to be called "window" coordinates); however, once that setting is turned on, that's no longer the case so that CL fixed it to return the rect in viewport space as its callers expect. Basically, it made the method account for the scale and offset applied by pinch zoom (see https://docs.google.com/presentation/d/1nJvJqL2dw5STi5FFpR6tP371vSpDWWs5Beksbfitpzc/edit?usp=sharing for an explanation of the pinch-virtual-viewport setting and viewport vs root frame). 

I'm not sure about DIPs vs physical pixels but, as oshima@ mentioned, I'm under the impression most code, unless explicitly specified, is in DIPs and so you'd need to apply device_scale_factor if you want to go to physical screen pixels. I believe firstRectForCharacterRange in DIPs (though the CL you referenced doesn't deal with DIPs vs physical pixels).

Let free to ping me if you need more details.
Cc: -shuchen@chromium.org bokan@chromium.org
Labels: -Needs-Feedback
Owner: shuchen@chromium.org
Status: Available
Thank you for sharing the slide, David.  It looks really helpful.

Sounds like your CL addressed the confusion in the origin of the coordinates in firstRectForCharacterRange.  That's great, but I guess we need to address the similar confusion in ui::TextInputClient::GetCompositionCharacterBounds because currently the rectangle returned from firstRectForCharacterRange is eventually passed to RenderWidgetHostViewAura::ConvertRectToScreen, which I think may not work for coordinates in viewport space.

https://chromium.googlesource.com/chromium/src/+/144ff57dc021b068d0df1100406c59c25f724b99/content/browser/renderer_host/render_widget_host_view_aura.cc#1641
| bool RenderWidgetHostViewAura::GetCompositionCharacterBounds(
|     uint32 index,
|     gfx::Rect* rect) const {
|   DCHECK(rect);
|   if (index >= composition_character_bounds_.size())
|     return false;
|   *rect = ConvertRectToScreen(composition_character_bounds_[index]);
|   return true;
| }

https://chromium.googlesource.com/chromium/src/+/144ff57dc021b068d0df1100406c59c25f724b99/content/browser/renderer_host/render_widget_host_view_aura.cc#1595
| gfx::Rect RenderWidgetHostViewAura::ConvertRectToScreen(
|     const gfx::Rect& rect) const {
|   gfx::Point origin = rect.origin();
|   gfx::Point end = gfx::Point(rect.right(), rect.bottom());
| 
|   aura::Window* root_window = window_->GetRootWindow();
|   if (!root_window)
|     return rect;
|   aura::client::ScreenPositionClient* screen_position_client =
|       aura::client::GetScreenPositionClient(root_window);
|   if (!screen_position_client)
|     return rect;
|   screen_position_client->ConvertPointToScreen(window_, &origin);
|   screen_position_client->ConvertPointToScreen(window_, &end);
|   return gfx::Rect(origin.x(),
|                    origin.y(),
|                    end.x() - origin.x(),
|                    end.y() - origin.y());
| }

I'm under the impression that the behavior in Blink side is getting stabilized and well documented thanks to David's effort.

Shu, do you have any ideas to improve the implementation, documentation, and test coverage here in the browser process side in order to make IME support more reliable and stable?
For example, here is an example of pitfalls:
1. When the user is typing something, composing char bounds, in viewport coordinates system [t=T1], are sent from Blink to the browser process.
2. The user zooms and/or scrolls the content, which updates the viewport coordinates system in Blink [t=T2] but doesn't let Blink send the new composing char bounds.
3. RenderWidgetHostViewAura::GetCompositionCharacterBounds is called in the browser process.

In this scenario, the returned rectangle at step 3 should be transformed into the screen coordinates with the viewport system [t=T1] instead of [t=T2].

Comment 13 by bokan@chromium.org, Apr 23 2015

Re#11: I think all those APIs (including ConvertRectToScreen) expect rects returned from Blink to be in viewport coordinates (see http://www.chromium.org/developers/design-documents/blink-coordinate-spaces for what I mean by viewport coordinates). This broke when I changed our semantics for pinch-zoom but http://crrev.com/967213004 should have fixed this to return the previously returned rect, so if the method worked correctly prior to M41 then it should be correct now.
Thanks David for correcting me.  I now suspect that changing semantics for pinch-zoom itself might have had more impact than http://crrev.com/967213004.  

I guess that in the previous pinch-zoom semantics InputHostMsg_ImeCompositionRangeChanged IPC message was sent from the renderer process to the browser process here every time when the zoom ratio is changed.
https://chromium.googlesource.com/chromium/src/+/c0e251b8eedbeec65e9d0cafc3c270f072fc6724/content/renderer/render_widget.cc#2088
In this scenario, performance wasn't great probably, but the browser process could be kept updated about the character bounds rectangle in the updated viewport space.

In the new improved pinch-zoom semantics, however, it seems that InputHostMsg_ImeCompositionRangeChanged IPC message is no longer sent from the renderer process to the browser process when only visual viewport is moving around.  This is really great for performance I think, but it is now the browser process's responsibility to perform coordinate transformation so that the lastly-sent character bounds rectangles can be correctly mapped into the latest viewport space.

I also have a half-a-year-old pending CL (http://crrev.com/699333003) for Android that I guess was directly affected by those recent improvements in pinch-zoom semantics.  Yesterday I managed to update that CL to work again, and hopefully now I can understand the new semantics correctly.  http://www.chromium.org/developers/design-documents/blink-coordinate-spaces was really great help for me to understand the semantics. Thanks!

Comment 15 by bokan@chromium.org, Apr 24 2015

The new semantics shouldn't have changed any zoom callbacks. I don't know the details above Blink, but if you're not getting a zoom notification where one is expected that's a bug.

Looking at InputHostMsg_ImeCompositionRangeChanged however, I can't find any specific zoom-related callback where it leads to sending this IPC. It looks like willBeginCompositorFrame calls updateSelectionBounds which will send this message. Doesn't this mean we should be sending this message on each compositor frame?
Good catch.  I confirmed that I can receive InputHostMsg_ImeCompositionRangeChanged when pinch-zoom viewport is moving around if I call UpdateCompositionInfo(false) in willBeginCompositorFrame().  Probably it makes sense to do for the consistency.  Maybe we might want to improve the performance by having a fused IPC message for willBeginCompositorFrame though.

Blockedon: chromium:485650
Adding dependency on Issue 485650 in case it may affect ui::TextInputClient.  Please correct it if my understanding if incorrect.
Cc: suzhe@chromium.org
https://codereview.chromium.org/1496243005
> Use Window coordinates for IME composition bounds, auto resize, pepper's selection bounds.

The above spec looks clear enough to address this issue.
Cc: robliao@chromium.org robliao@chromium.org
Project Member

Comment 20 by sheriffbot@chromium.org, Dec 19 2016

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been available for more than 365 days, and should be re-evaluated. Please re-triage this issue.
The Hotlist-Recharge-Cold label is applied for tracking purposes, and should not be removed after re-triaging the issue.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Assigned (was: Untriaged)
shuchen@: ping? maybe you can update?
Blocking: 475718
Re #18, yukawa@, should adding some comments to the methods in ui::TextInputClient be enough for this issue?

Project Member

Comment 24 by bugdroid1@chromium.org, Feb 22 2017

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

commit da4307a16fe8c0bbecc8158e21ffef8190e23623
Author: shuchen <shuchen@chromium.org>
Date: Wed Feb 22 10:38:44 2017

Use the physical-pixel space for native IME on linux platform.

What's more, this cl makes a clear spec for the coordinates system in
ui::TextInputClient.

BUG= 475718 , 360334 

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

[modify] https://crrev.com/da4307a16fe8c0bbecc8158e21ffef8190e23623/chrome/browser/ui/libgtkui/x11_input_method_context_impl_gtk.cc
[modify] https://crrev.com/da4307a16fe8c0bbecc8158e21ffef8190e23623/ui/base/ime/text_input_client.h

Status: Fixed (was: Assigned)

Sign in to add a comment