Closed Bug 1109410 Opened 9 years ago Closed 9 years ago

IME candidate window position doesn't honor CSS transform

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

()

Details

(Keywords: inputmethod, Whiteboard: [parity-IE][parity-Chrome])

Attachments

(2 files, 3 obsolete files)

Attached image screenshot
I got a bug report in tweetdeck:

1. Go to https://tweetdeck.twitter.com/ and log in with your twitter account.
2. Click a button to tweet (top-left of the page)
3. Type something in the edit field with IME.
4. Show candidate window of IME for the composing string with hitting space key or something.

Then, you can see candidate window with strange point. (See the screenshot)

This is caused by CSS transform (translate). ContentEventHandler uses frame rects but which are not transformed. I'm not sure how do we convert rects quickly.

Simple testcase is here:
http://jsfiddle.net/dj9bdbez/

FYI: IE and Chrome work fine in the testcase.
We've solved this problem for <select> popups, we can probably use a similar solution for this.
(In reply to Markus Stange [:mstange] from comment #1)
> We've solved this problem for <select> popups, we can probably use a similar
> solution for this.

Is it bug 467442?
Oh, right, that's the bug I was thinking of.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8666604 [details] [diff] [review]
Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset()

Review of attachment 8666604 [details] [diff] [review]:
-----------------------------------------------------------------

::: view/nsView.h
@@ +179,5 @@
>    /**
> +   * Called to query the root of this view.
> +   * @result view's root
> +   */
> +  nsView* GetRoot() const

This is potentially confusing. It's not clear whether it's the root of the overall view tree or the root view for the document that contains 'this'.

Also, this will return the wrong view when using the IME in a popup.

I think we should probably not add this. Instead, ContentEventHandler::ConvertToRootViewRelativeOffset should take a widget parameter (passed in from the event, normally) and convert to coordinates relative to that widget. You can do that by getting the frame from the widget. ContentEventHandler::OnQueryCharacterAtPoint does something like this already.
Attachment #8666604 - Flags: review?(roc) → review-
> Also, this will return the wrong view when using the IME in a popup.

At least I tested manually, that works on popup (<xul:panel>) too.

> I think we should probably not add this. Instead, ContentEventHandler::ConvertToRootViewRelativeOffset
> should take a widget parameter (passed in from the event, normally) and convert to coordinates
> relative to that widget. You can do that by getting the frame from the widget.

Hmm, currently, ContentEventHandler returns a rect of the root view containing the frame coordinates. So, if focused element is in a <xul:panel>, it returns a rect in the popup widget coordinates. If widget which dispatched the query event needs to convert the rect to another coordinates, the widget uses mFocusedWidget. At least on Cocoa, mFocusedWidget is necessary information for telling the window level to Cocoa. Therefore, I don't think that we should change the behavior of ContentEventHandler.

Even if we should always use the widget which dispatched the query event coordinates, we need to change the related code in the native IME handlers for all platforms. So, at least, we shouldn't do it in this bug.

So, I think that I should just move the nsView::GetRoot() to ContentEventHandler::ConvertToRootViewRelativeOffset(), how about you?
Flags: needinfo?(roc)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> > Also, this will return the wrong view when using the IME in a popup.
> 
> At least I tested manually, that works on popup (<xul:panel>) too.
> 
> > I think we should probably not add this. Instead, ContentEventHandler::ConvertToRootViewRelativeOffset
> > should take a widget parameter (passed in from the event, normally) and convert to coordinates
> > relative to that widget. You can do that by getting the frame from the widget.
> 
> Hmm, currently, ContentEventHandler returns a rect of the root view
> containing the frame coordinates. So, if focused element is in a
> <xul:panel>, it returns a rect in the popup widget coordinates. If widget
> which dispatched the query event needs to convert the rect to another
> coordinates, the widget uses mFocusedWidget. At least on Cocoa,
> mFocusedWidget is necessary information for telling the window level to
> Cocoa. Therefore, I don't think that we should change the behavior of
> ContentEventHandler.

Is it documented somewhere what the rectangle in the event reply is relative to? I can't see it. Can you add such documentation?
Flags: needinfo?(roc)
> Can you add such documentation?

Sure. Indeed, WidgetQueryEvent definition should have it.
Comment on attachment 8667076 [details] [diff] [review]
Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset()

Review of attachment 8667076 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/ContentEventHandler.cpp
@@ +1566,5 @@
> +  if (NS_WARN_IF(!rootFrame)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  aRect = nsLayoutUtils::TransformFrameRectToAncestor(aFrame, aRect, rootFrame);

I don't think this matches the comment you wrote in TextEvents.h. This is going to find the root frame for the toplevel document, even if aFrame is in a popup.
Attachment #8667076 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline Oct 1-4) from comment #13)
> Comment on attachment 8667076 [details] [diff] [review]
> Resolve CSS transform in
> ContentEventHandler::ConvertToRootViewRelativeOffset()
> 
> Review of attachment 8667076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/events/ContentEventHandler.cpp
> @@ +1566,5 @@
> > +  if (NS_WARN_IF(!rootFrame)) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  aRect = nsLayoutUtils::TransformFrameRectToAncestor(aFrame, aRect, rootFrame);
> 
> I don't think this matches the comment you wrote in TextEvents.h. This is
> going to find the root frame for the toplevel document, even if aFrame is in
> a popup.

Wow, you're right. I revisit all native IME handlers which dispatch eQueryTextRect. Then, they use nsBaseWidget::GetTopLevelWidget() for getting the screen offset of the widget. So, I misunderstood current behavior. I should fix the comment.
Changed the comment for correcting my misunderstood.
Attachment #8667076 - Attachment is obsolete: true
Attachment #8667316 - Flags: review?(roc)
Comment on attachment 8667316 [details] [diff] [review]
Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset()

Review of attachment 8667316 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/ContentEventHandler.cpp
@@ +1564,5 @@
> +  MOZ_RELEASE_ASSERT(rootView);
> +  nsIFrame* rootFrame = rootView->GetFrame();
> +  if (NS_WARN_IF(!rootFrame)) {
> +    return NS_ERROR_FAILURE;
> +  }

It's more efficient and simpler if you call aFrame->GetRootPresContext()->PresShell()->GetRootFrame() ... with a null check on GetRootPresContext and the root frame.
Attachment #8667316 - Flags: review?(roc)
Comment on attachment 8667755 [details] [diff] [review]
Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset()

Review of attachment 8667755 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/ContentEventHandler.cpp
@@ +1547,4 @@
>  }
>  
>  nsresult
>  ContentEventHandler::ConvertToRootViewRelativeOffset(nsIFrame* aFrame,

Change this to ConvertToRootRelativeOffset
Attachment #8667755 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd01c66f79b845822c6cbf73cc53f68bd292b700
Bug 1109410 Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset() r=roc
https://hg.mozilla.org/mozilla-central/rev/cd01c66f79b8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1261671
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: