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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Internationalization
P1
normal
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha1
x86
Windows XP
inputmethod, intl, regression, verified1.8.1.10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
Status: NEW → ASSIGNED
Priority: -- → P1
Created attachment 202154 [details] [diff] [review]
Patch rv1.0
Attachment #202154 - Flags: review?(timeless)
Created attachment 202159 [details]
screenshot
Created attachment 202161 [details]
screenshot with patch
Attachment #202161 - Attachment is patch: false
Attachment #202161 - Attachment mime type: text/plain → image/png
Attachment #202154 - Flags: review?(timeless) → review-
Created attachment 202188 [details] [diff] [review]
Patch rv1.1

Sorry. I have a mistake in previous patch.
Attachment #202154 - Attachment is obsolete: true
Attachment #202188 - Flags: review?(timeless)
Attachment #202188 - Flags: review?(timeless) → review-
Created attachment 202190 [details] [diff] [review]
Patch rv1.2
Attachment #202188 - Attachment is obsolete: true
Attachment #202190 - Flags: review?(timeless)

Comment 6

12 years ago
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);
Attachment #202190 - Flags: review?(timeless) → review-
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'.
Attachment #202190 - Flags: review- → review?(timeless)
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'.
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
timeless:

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

Updated

12 years ago
Attachment #202190 - Flags: review?(timeless) → review+
Attachment #202190 - Flags: superreview?(roc)

Comment 11

12 years ago
Please integrate this. Thanks. We IME users really need this.
(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 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.
Attachment #202190 - Flags: superreview?(roc) → superreview+
Created attachment 203492 [details] [diff] [review]
Patch rv1.3
Attachment #202190 - Attachment is obsolete: true
Attachment #203492 - Flags: superreview+
Attachment #203492 - Flags: review+
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(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.
Attachment #203492 - Attachment is obsolete: true
Attachment #203492 - Flags: superreview+
Attachment #203492 - Flags: review-
Attachment #203492 - Flags: review+
Attachment #202190 - Attachment is obsolete: false
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

(you should be able to use nsRefPtr)
Keywords: fixed1.8.1.10
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?
-> v. on 1.8.1.10
Keywords: fixed1.8.1.10 → verified1.8.1.10
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.