The default bug view has changed. See this FAQ.

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)

(Assignee)

Description

12 years ago
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.
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Priority: -- → P1
(Assignee)

Comment 1

12 years ago
Created attachment 202154 [details] [diff] [review]
Patch rv1.0
Attachment #202154 - Flags: review?(timeless)
(Assignee)

Comment 2

12 years ago
Created attachment 202159 [details]
screenshot
(Assignee)

Comment 3

12 years ago
Created attachment 202161 [details]
screenshot with patch
(Assignee)

Updated

12 years ago
Attachment #202161 - Attachment is patch: false
Attachment #202161 - Attachment mime type: text/plain → image/png
(Assignee)

Updated

12 years ago
Attachment #202154 - Flags: review?(timeless) → review-
(Assignee)

Comment 4

12 years ago
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)
(Assignee)

Updated

12 years ago
Attachment #202188 - Flags: review?(timeless) → review-
(Assignee)

Comment 5

12 years ago
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-
(Assignee)

Comment 7

12 years ago
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)
(Assignee)

Comment 8

12 years ago
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'.
(Assignee)

Comment 9

12 years ago
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
(Assignee)

Comment 10

12 years ago
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+
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 14

12 years ago
Created attachment 203492 [details] [diff] [review]
Patch rv1.3
Attachment #202190 - Attachment is obsolete: true
Attachment #203492 - Flags: superreview+
Attachment #203492 - Flags: review+
(Assignee)

Comment 15

12 years ago
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #203492 - Attachment is obsolete: true
Attachment #203492 - Flags: superreview+
Attachment #203492 - Flags: review-
Attachment #203492 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #202190 - Attachment is obsolete: false
(Assignee)

Comment 17

12 years ago
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)
(Assignee)

Updated

10 years ago
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?
(Assignee)

Comment 20

10 years ago
-> v. on 1.8.1.10
Keywords: fixed1.8.1.10 → verified1.8.1.10
(Assignee)

Updated

7 years ago
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.