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

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-11-07 14:11:16 PST
Created attachment 202154 [details] [diff] [review]
Patch rv1.0
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-11-07 14:26:06 PST
Created attachment 202159 [details]
screenshot
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-11-07 14:28:10 PST
Created attachment 202161 [details]
screenshot with patch
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-11-07 19:47:09 PST
Created attachment 202190 [details] [diff] [review]
Patch rv1.2
Comment 6 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 wind li 2005-11-16 02:04:00 PST
Please integrate this. Thanks. We IME users really need this.
Comment 12 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 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-11-17 21:28:44 PST
Created attachment 203492 [details] [diff] [review]
Patch rv1.3
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-11-17 21:30:09 PST
checked-in.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 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 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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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.