Last Comment Bug 315457 - IME candidate list is positioned to wrong position if the editor is on frame page
: IME candidate list is positioned to wrong position if the editor is on frame ...
Status: RESOLVED FIXED
: inputmethod, intl, regression, verified1.8.1.10
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows XP
P1 normal with 1 vote (vote)
: mozilla1.9alpha1
Assigned To: Masayuki Nakano [:masayuki]
: Yuying Long
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 313918
  Show dependency treegraph
 
Reported: 2005-11-07 13:52 PST by Masayuki Nakano [:masayuki]
Modified: 2010-06-18 19:00 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch rv1.0 (8.83 KB, patch)
2005-11-07 14:11 PST, Masayuki Nakano [:masayuki]
masayuki: review-
Details | Diff | Splinter Review
screenshot (12.45 KB, image/png)
2005-11-07 14:26 PST, Masayuki Nakano [:masayuki]
no flags Details
screenshot with patch (10.47 KB, image/png)
2005-11-07 14:28 PST, Masayuki Nakano [:masayuki]
no flags Details
Patch rv1.1 (8.83 KB, patch)
2005-11-07 19:40 PST, Masayuki Nakano [:masayuki]
masayuki: review-
Details | Diff | Splinter Review
Patch rv1.2 (8.82 KB, patch)
2005-11-07 19:47 PST, Masayuki Nakano [:masayuki]
timeless: review+
roc: superreview+
Details | Diff | Splinter Review
Patch rv1.3 (8.82 KB, patch)
2005-11-17 21:28 PST, Masayuki Nakano [:masayuki]
masayuki: review-
Details | Diff | Splinter Review

Description User image Masayuki Nakano [:masayuki] 2005-11-07 13:52:45 PST
This is regression of bug 313918.
On Windows, IME events always returns root view coordinates for caret rect.
But if the editor is on frame page, the IME event processing widget is not equal root view related widget.
Comment 1 User image Masayuki Nakano [:masayuki] 2005-11-07 14:11:16 PST
Created attachment 202154 [details] [diff] [review]
Patch rv1.0
Comment 2 User image Masayuki Nakano [:masayuki] 2005-11-07 14:26:06 PST
Created attachment 202159 [details]
screenshot
Comment 3 User image Masayuki Nakano [:masayuki] 2005-11-07 14:28:10 PST
Created attachment 202161 [details]
screenshot with patch
Comment 4 User image Masayuki Nakano [:masayuki] 2005-11-07 19:40:51 PST
Created attachment 202188 [details] [diff] [review]
Patch rv1.1

Sorry. I have a mistake in previous patch.
Comment 5 User image Masayuki Nakano [:masayuki] 2005-11-07 19:47:09 PST
Created attachment 202190 [details] [diff] [review]
Patch rv1.2
Comment 6 User image timeless 2005-11-07 21:01:27 PST
Comment on attachment 202190 [details] [diff] [review]
Patch rv1.2

>RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v
>retrieving revision 3.590
>@@ -7239,16 +7243,33 @@ PRBool nsWindow::OnIMEQueryCharPosition(
>+void
>+nsWindow::ResolveIMECaretPos(nsWindow* aClient,
>+                             nsRect&   aEventResult,
>+                             nsRect&   aResult)
>+{
>+  // RootView coordinates -> Screen coordinates
>+  nsWindow* topWindow = GetTopLevelWindow();
theoretically in certain very strange cases, i believe this can return null. can you prove to me that this will never happen for ime?

>+  topWindow->WidgetToScreen(aEventResult, aResult);
>+  NS_RELEASE(topWindow);
Comment 7 User image Masayuki Nakano [:masayuki] 2005-11-08 07:31:23 PST
Comment on attachment 202190 [details] [diff] [review]
Patch rv1.2

> theoretically in certain very strange cases, i believe this can return null.
> can you prove to me that this will never happen for ime?

I don't think so. GetTopLevelWindow always 
returns non-null. See this code.

> 8046 nsWindow* nsWindow::GetTopLevelWindow()
> 8047 {
> 8048   nsWindow* curWindow = this;
> 8049   NS_ADDREF(curWindow);
> 8050 
> 8051   while (PR_TRUE)
> 8052   {
> 8053     nsWindow* parentWindow = curWindow->GetParent(PR_TRUE);
> 8054 
> 8055     if (parentWindow)
> 8056     {
> 8057       NS_RELEASE(curWindow);
> 8058       curWindow = parentWindow;
> 8059     } else
> 8060       return curWindow;
> 8061   }
> 8062 }

if 'this' doesn't have parent, it returns 'this'.
Comment 8 User image Masayuki Nakano [:masayuki] 2005-11-08 07:31:32 PST
Comment on attachment 202190 [details] [diff] [review]
Patch rv1.2

> theoretically in certain very strange cases, i believe this can return null.
> can you prove to me that this will never happen for ime?

I don't think so. GetTopLevelWindow always 
returns non-null. See this code.

> 8046 nsWindow* nsWindow::GetTopLevelWindow()
> 8047 {
> 8048   nsWindow* curWindow = this;
> 8049   NS_ADDREF(curWindow);
> 8050 
> 8051   while (PR_TRUE)
> 8052   {
> 8053     nsWindow* parentWindow = curWindow->GetParent(PR_TRUE);
> 8054 
> 8055     if (parentWindow)
> 8056     {
> 8057       NS_RELEASE(curWindow);
> 8058       curWindow = parentWindow;
> 8059     } else
> 8060       return curWindow;
> 8061   }
> 8062 }

if 'this' doesn't have parent, it returns 'this'.
Comment 9 User image Masayuki Nakano [:masayuki] 2005-11-08 07:38:48 PST
This is not a scope of this bug but...

> 8055     if (parentWindow)
> 8056     {
> 8057       NS_RELEASE(curWindow);
> 8058       curWindow = parentWindow;
             NS_ADDREF(curWindow);           // <- Don't we need it?
> 8059     } else
Comment 10 User image Masayuki Nakano [:masayuki] 2005-11-10 00:29:45 PST
timeless:

Please hurry up to re-review for this patch.
This bug is serious for IME users.
Comment 11 User image wind li 2005-11-16 02:04:00 PST
Please integrate this. Thanks. We IME users really need this.
Comment 12 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-17 19:09:49 PST
(In reply to comment #9)
> This is not a scope of this bug but...
> 
> > 8055     if (parentWindow)
> > 8056     {
> > 8057       NS_RELEASE(curWindow);
> > 8058       curWindow = parentWindow;
>              NS_ADDREF(curWindow);           // <- Don't we need it?
> > 8059     } else
> 

We don't need it because GetParent already addrefed parentWindow.
Comment 13 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-17 19:10:51 PST
Comment on attachment 202190 [details] [diff] [review]
Patch rv1.2

+  nsWindow* topWindow = GetTopLevelWindow();

You can write
nsCOMPtr<nsWindow> topWindow = getter_AddRefs(GetTopLevelWindow());
and then you don't have to manually release.
Comment 14 User image Masayuki Nakano [:masayuki] 2005-11-17 21:28:44 PST
Created attachment 203492 [details] [diff] [review]
Patch rv1.3
Comment 15 User image Masayuki Nakano [:masayuki] 2005-11-17 21:30:09 PST
checked-in.
Comment 16 User image Masayuki Nakano [:masayuki] 2005-11-17 22:33:19 PST
(In reply to comment #13)
> (From update of attachment 202190 [details] [diff] [review] [edit])
> +  nsWindow* topWindow = GetTopLevelWindow();
> 
> You can write
> nsCOMPtr<nsWindow> topWindow = getter_AddRefs(GetTopLevelWindow());
> and then you don't have to manually release.
> 

this makes bustage. I change to rv1.2.
Comment 17 User image Masayuki Nakano [:masayuki] 2005-11-17 22:40:49 PST
http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/widget/src/windows&command=DIFF_FRAMESET&file=nsWindow.cpp&rev1=3.596&rev2=3.597&root=/cvsroot

> ../../../dist/include/xpcom\nsCOMPtr.h(635) : error C2594: 'argument' : ambiguous conversions from 'class nsWindow *const ' to 'class nsISupports *'
>         ../../../dist/include/xpcom\nsCOMPtr.h(632) : while compiling class-template member function '__thiscall nsCOMPtr<class nsWindow>::nsCOMPtr<class nsWindow>(const struct already_AddRefed<class nsWindow> &)'
> make[6]: *** [nsWindow.obj] Error 2

Comment 18 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-11-18 04:18:43 PST
(you should be able to use nsRefPtr)
Comment 19 User image Al Billings [:abillings] 2007-11-15 16:32:46 PST
Masayuki, can you check with the Firefox 2.0.0.10 release candidate at http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/ and verify that this is fixed?
Comment 20 User image Masayuki Nakano [:masayuki] 2007-11-15 22:27:07 PST
-> v. on 1.8.1.10

Note You need to log in before you can comment on or make changes to this bug.