315457 - IME candidate list is positioned to wrong position if the editor is on frame page

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot with patch

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

Sorry. I have a mistake in previous patch.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.2

Comment 6

[timeless]

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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

timeless:

Please hurry up to re-review for this patch.
This bug is serious for IME users.

Comment 11

[wind li]

Please integrate this. Thanks. We IME users really need this.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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)]

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] (he/him)(JST, +0900)]

Attachment: Patch rv1.3

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

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)]

(you should be able to use nsRefPtr)

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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] (he/him)(JST, +0900)]

-> v. on 1.8.1.10