[e10s] candidate window sometimes appears at top-left of screen because GetTextExt() returns empty rect

NEW
Assigned to

Status

()

Core
Widget: Win32
P3
normal
3 years ago
2 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
x86
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: tpi:+)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Here is a nsTextStore log.

0[f919a0d800]: TSF: 0xf92e8e20c0   nsTextStore::GetActiveView() succeeded: *pvcView=1
0[f919a0d800]: TSF: 0xf92e8e20c0 nsTextStore::GetScreenExt(vcView=1, prc=0xf917c0d410)
0[f919a0d800]: TSF: 0xf92e8e20c0   nsTextStore::GetScreenExt() succeeded: *prc={ left=813, top=607, right=1160, bottom=644 }
0[f919a0d800]: TSF: 0xf92e8e20c0 nsTextStore::GetTextExt(vcView=1, acpStart=0, acpEnd=0, prc=0xf917c0d420, pfClipped=0xf917c0d3e8)
0[f919a0d800]: TSF: 0xf92e8e20c0   nsTextStore::GetTextExt() succeeded: *prc={ left=0, top=0, right=0, bottom=0 }, *pfClipped=true

We need consider acpEnd-acpStart=0 case on e10s mode.
(Assignee)

Updated

3 years ago
Assignee: nobody → m_kato
(Assignee)

Updated

3 years ago
Summary: [e10s] candidate window sometimes top-left of screen because GetTextExt() returns empty rect → [e10s] candidate window sometimes appears at top-left of screen because GetTextExt() returns empty rect
(Assignee)

Comment 1

3 years ago
Created attachment 8549431 [details] [diff] [review]
Part 1. Remote NS_QUERY_TEXT_RECT doesn't return empty rect if length is 0
(Assignee)

Comment 2

3 years ago
Created attachment 8549445 [details] [diff] [review]
Part 2. Return error instead of emtpy rect If text rect isn't intersected with screen rect
(Assignee)

Updated

3 years ago
Attachment #8549445 - Attachment description: Return error instead of emtpy rect If text rect isn't intersected with screen rect → Part 2. Return error instead of emtpy rect If text rect isn't intersected with screen rect

Updated

3 years ago
tracking-e10s: --- → +

Comment 3

3 years ago
this blocks an m6.
tracking-e10s: + → m6+
(Assignee)

Comment 4

3 years ago
Comment on attachment 8549431 [details] [diff] [review]
Part 1. Remote NS_QUERY_TEXT_RECT doesn't return empty rect if length is 0

nsTextStore::GetTextExt() is sometimes called by acpEnd-acpStart=0.  So it will call as length=0 with NS_QUERY_TEXT_RECT.  Even if length is 0 by NS_QUERY_TEXT_RECT, it doesn't return error and empy rect (it will return width is 1).

So we need consider this situation for e10s.
Attachment #8549431 - Flags: review?(masayuki)
(Assignee)

Comment 5

3 years ago
Comment on attachment 8549445 [details] [diff] [review]
Part 2. Return error instead of emtpy rect If text rect isn't intersected with screen rect

When nsTextStore::GetTextExt returns empty rect, candidate window will be shown at top-left.  So we should return error instead.

If we should return empty rect with error situation, all return value should return S_OK with empty rect instead of E_FAIL.

Also, by part 1's fix, IntersectRect doesn't return any error.  But we should add log for this situation.
Attachment #8549445 - Flags: review?(masayuki)
Comment on attachment 8549431 [details] [diff] [review]
Part 1. Remote NS_QUERY_TEXT_RECT doesn't return empty rect if length is 0

In non-e10s mode, ContentEventHandler returns at least 1px width rect:
http://mxr.mozilla.org/mozilla-central/source/dom/events/ContentEventHandler.cpp#763

So, your approach must be right.

>       if (aEvent.mInput.mOffset < mIMECompositionRectOffset ||
>           (aEvent.mInput.mOffset + aEvent.mInput.mLength >
>-            mIMECompositionRectOffset + mIMECompositionRects.Length())) {
>+            mIMECompositionRectOffset + mIMECompositionRects.Length() ||
>+          !mIMECompositionRects.Length())) {

Intent +1 space? And it might be better to use IsEmpty().
Attachment #8549431 - Flags: review?(masayuki) → review+
Comment on attachment 8549445 [details] [diff] [review]
Part 2. Return error instead of emtpy rect If text rect isn't intersected with screen rect

Um, I don't think this is right behavior:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms538435%28v=vs.85%29.aspx

> Remarks
> 
> If the document window is minimized, or if the specified text is not currently
> visible, the method returns S_OK with the prc parameter set to {0,0,0,0}.

Do you really need this change, now? Or isn't it okay to just add a log in this case without changing the behavior?

Updated

3 years ago
Flags: needinfo?(m_kato)
Requesting re-triage. Bug 1147722 should be an M6. I'm thinking we could move this up to M7 or M8.
tracking-e10s: m6+ → ?
ack, this should have been kicked to triage with billm's last comment.  The active NI blocked that from happening.
Flags: needinfo?(m_kato)
tracking-e10s: ? → +
(Assignee)

Comment 10

3 years ago
Comment on attachment 8549445 [details] [diff] [review]
Part 2. Return error instead of emtpy rect If text rect isn't intersected with screen rect

If not return E_FAIL, log is unnecessary because we can found whether reset or not by rect's value.
Attachment #8549445 - Flags: review?(masayuki)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8549431 [details] [diff] [review]
Part 1. Remote NS_QUERY_TEXT_RECT doesn't return empty rect if length is 0

This code had some problems.

- We should use caret rect like Cocoa implementation if length (acpEnd - acpStart) is 0.
- This fix don't return correct string rect of current composition.  GetTextExt will be called at too early timing.  So I should adjust acpStart and acpEnd to get temporary rect.

ChildEBP RetAddr
0062e6a4 753ca6f8 xul!nsTextStore::GetTextExt
0062e6ec 5db8dbca MSCTF!CContextView::GetTextExt+0x188
0062e724 5dba96c9 mscand20!GetTextExtInActiveView+0x6f
0062e778 5dba965c mscand20!CTfCandidateUI::EditSessionCallback+0x49
0062e790 753d247d mscand20!COurEditSession::DoEditSession+0x2c
0062e7cc 5dbac796 MSCTF!CInputContext::RequestEditSession+0x182
0062e828 5dbba68f mscand20!CTfCandidateUI::UpdateTargetRect+0x7f
0062e844 6483cd1f mscand20!CTfCandidateUI::SetTargetRange+0x5f
0062e88c 646c36cc imjptip!CTipFnCandidateUI::UpdateCandidateUI+0x15f
0062e8b0 646b1f9c imetip!CTipFnCandidateHandler::EsfnUpdateCandidateUI+0x41
0062e8f8 64699532 imetip!CTipFnCandidateHandler::OnEditSession+0xf4
0062ea9c 64695387 imetip!CTipFnEditSessionHandler::_OnDoEditSession+0x24d2
0062eab8 753c1e2a imetip!Tsfutil::CTfEditSession::DoEditSession+0x37
0062eb00 753c1d99 MSCTF!CInputContext::_DoEditSession+0x80
0062eb50 753b6826 MSCTF!CInputContext::_EditSessionQiCallback+0xda
0062eb6c 753b6aa4 MSCTF!CInputContext::_DispatchQueueItem+0x24
0062eba0 753ac07a MSCTF!CInputContext::_EmptyLockQueue+0x71
0062ebf0 11ae05e9 MSCTF!CACPWrap::OnLockGranted+0x11a
0062ec24 753c0df6 xul!nsTextStore::RequestLock+0xf5
0062ec40 753b6a1b MSCTF!CACPWrap::RequestLock+0x3d
...
0062f700 11adedfc MSCTF!CThreadInputMgr::KeyDown+0x1b
0062f724 11b17f1e xul!nsTextStore::ProcessRawKeyMessage+0x5f
...
0062fe8c 00000000 ntdll!_RtlUserThreadStart+0x1b
Attachment #8549431 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8549445 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8605600 [details] [diff] [review]
Part 1 . Use caret rect if TSF queries length 0 string
(Assignee)

Updated

3 years ago
Depends on: 1120851
(Assignee)

Comment 13

3 years ago
Nakano-san seems to take care length=0 issue as bug 1171332.

But like comment #11, we have no change to get composition rect due to no event loop.  So I should use fake rect at this situation.
(In reply to Makoto Kato (:m_kato) from comment #13)
> Nakano-san seems to take care length=0 issue as bug 1171332.
> 
> But like comment #11, we have no change to get composition rect due to no
> event loop.  So I should use fake rect at this situation.

Anyway, for suppressing a lot of warning noise in new ContentCache, I'll include a patch like attachment 8549431 [details] [diff] [review]. If we need additional hack as you said, let's fix it in this bug.
Priority: -- → P3

Updated

2 years ago
Whiteboard: tpi:+
You need to log in before you can comment on or make changes to this bug.