Closed
Bug 1109410
Opened 10 years ago
Closed 9 years ago
IME candidate window position doesn't honor CSS transform
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
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)
145.73 KB,
image/png
|
Details | |
7.19 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
We've solved this problem for <select> popups, we can probably use a similar solution for this.
Assignee | ||
Comment 2•10 years ago
|
||
(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?
Comment 3•10 years ago
|
||
Oh, right, that's the bug I was thinking of.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8666604 -
Flags: review?(roc)
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-
Assignee | ||
Comment 8•9 years ago
|
||
> 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)
Assignee | ||
Comment 10•9 years ago
|
||
> Can you add such documentation?
Sure. Indeed, WidgetQueryEvent definition should have it.
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8666604 -
Attachment is obsolete: true
Attachment #8667076 -
Flags: review?(roc)
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-
Assignee | ||
Comment 14•9 years ago
|
||
(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.
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8667316 -
Attachment is obsolete: true
Attachment #8667755 -
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+
Assignee | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd01c66f79b845822c6cbf73cc53f68bd292b700
Bug 1109410 Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset() r=roc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•