259454 - IME input (e.g., Chinese, Japanese and Korean) cannot be performed in Find Toolbar when opened with "/" or "'"

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Hiro]

It is a different problem in bug251891.
search in Japanese is impossible because of bug251891 and this problem.

Reproducible: Always
Steps to Reproduce:
1. IME is turned OFF.
2. push "/"
3. IME is turned ON.
4. The Search field of Find Tool Bar is clicked with a mouse.
5. Japanese is inputted.

Actual Results:
Find Tool Bar will close.
For the reason, it cannot search in Japanese.
In the case of an alphabetic character, this problem is not generated.

Expected Results:
A Japanese input can be performed in the reference field of Find Tool Bar.
And reference is possible in Japanese.

Windows XP Pro SP1
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7.3) Gecko/20040914
Firefox/0.10

Comment 1

[HARUNAGA Hirotoshi]

Steps to Reproduce:
1. turn off IME
2. type "/"
3. turn on IME
4. click the search field of Find Toolbar
5. type something

Result:
Find toolbar closes

confirmed with 1.0PR Win and Mac build.
if we do the steps quickly, sometimes it works, but that's very difficult.
"Find in This Page" (Ctrl+F/Cmd+F) don't have this problem.

not critical but major,intl.
marking new and requesting blocking-aviary1.0.

Comment 2

[Jungshik Shin]

The same is true of Firefox on Linux. 

Comment 3

[Tomer Altman]

Confirmed on 1.0PR on WinXP.

Currently the only way to search is by:
1. IME turned off
2. type something (if FAYT is enabled without /) or click "/" or click "ctrl-f"
3. click on the find bar fast enough
4. IME turned on
steps 3 and 4 can be inverted.

Various points that can help, my personal thought which might be stupid:
* "/" and "ctrl-f" should open the find bar even if IME is on, even if the "f"
in ctrl-f is actually an IME type, and not a real f.
* in case FAYT without / is enabled, any beginning of input, even if it's
somehow controlled by the IME should open the bar as if we were at step 4.

Comment 4

[chris hofmann]

jshin,  can you help on fixing this bug for avairy 1.0?

Comment 5

[Jungshik Shin]

Masayuki-san, can you take a look?  This and bug 251891 are serious enough for
us to consider backing out 'Find toolbox', IMHO if we cannot fix them in time
for 1.0 release.
 

Comment 6

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

Yes, it is very important problem for CJK users.
I think 'Find Toolbar' should be backed out before 1.0 final.

I want to look the source code. But I cannot find it.
Please tell me where is 'Find Toolbar'.

Comment 7

[Jungshik Shin]

blakeross, can you help Masayuki-san with locating the relevant files? 

Comment 8

[Michael Clackler]

(In reply to comment #6)
> Yes, it is very important problem for CJK users.
> I think 'Find Toolbar' should be backed out before 1.0 final.
> 
> I want to look the source code. But I cannot find it.
> Please tell me where is 'Find Toolbar'.

The control of the find toolbar is in browser/base/content/browser.js

Here's a link to blake's last change to fix international characters with the
find bar:
http://webtools.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser.js&rev1=1.296.2.3.2.57&rev2=1.296.2.3.2.58&root=/cvsroot

The function in browser.js that is called to close the find bar is for some
reason named closeFindBar()!!

Comment 9

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

When the user inputed with IME, |onBrowserKeyPress| has '0' for |evt.keyCode|.

In current architecture, the find bar is controled by browser's key press event.
But the architecture is not better for IME.
I propose following architecture.

1. The input characters is not controled by browser's key press event.
   So, same as 'Ctrl+F', the input characters are controled by 'editor field'.

2. In FAYT mode, re-find string by changing event of 'editor field'.

Comment 10

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

I cannot fix this bug.
Because it is _not_ IME bug.
It is the implementation problem of XUL.

Comment 11

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

Currently, |onBrowserKeyPress| is controling the string.
But, the Javascript code cannot get IME string.
Because IME string has 'composition string'.

I think that Firefox should be controling the string by 'nsTypeAheadFind.cpp',
not by Javascript code.
And the finding string that is the feedback of nsTypeAheadFind should be
inputted to 'Find Toolbar' by Javascript code(browser.js).

Comment 12

[Blake Ross]

Masayuki, the goal is to move towards a JavaScript implementation of typeahead
find, and away from the old, heavy cpp implementation. This is not really a find
toolbar bug but I will investigate ways to workaround it.

Can you explain why this bug is so significant that you'd want to back out find
toolbar? As I understand it, normal find (with Ctrl+F or from the Edit menu--the
kind that most users are going to use) works properly. We've never actually
exposed the "/" method of searching in Firefox--it was just a carry-over from
Seamonkey--so while I will certainly do my best to get this bug fixed, I don't
understand the significance that some attach to it.

Comment 13

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

Blake Ross:

Do you know IME system?

Chinese, Korean and Japanese languages have many many characters.
Because of it, we cannot type all characters directly.
IME provides the way to type all character for us.
When we type to one or more characters,
IME converts to the other character(s) from them.

In other words, IME needs 'inputting' string.
In English, not need it. Because only need 'inputted' string.

'inputting' string is called "composition string".
I think that on javascript code cannot use "composition string".
But cpp file can it.

Comment 14

[Wilson Lee [:kourge]]

Actually, it works like this
you enter base characters and they would be converted to real characters
but at this time, they're still not really inputted, it just shows on the screen
and the string remains in the IME buffer, not the real input text box
after you verify all the converted characters are correct, you can press enter
and the string will be really inputted into the input box

Comment 15

[Hung-Te Lin]

Confirmed on Traditional Chinese Windows series (XP, 2000, 98).

(In reply to comment #12)
> toolbar? As I understand it, normal find (with Ctrl+F or from the Edit menu--the
> kind that most users are going to use) works properly. We've never actually
> exposed the "/" method of searching in Firefox--it was just a carry-over from
Yes. Ctrl-F works perfectly.

The "/"(text) and "'"(link) are already known by many people, so they will use
that feature if there it is. If Firefox wants to make Ctrl-F code the official way
to access find toolbar, maybe you can "disable"  "/" and "'" or make it call
Ctrl-F actually although that may result in some confusion(because the behavior
or "'" was changed).

Comment 16

[Hung-Te Lin]

one more comment. 

In(In reply to comment #15)
> to access find toolbar, maybe you can "disable"  "/" and "'" or make it call
But even if "/" and "'" are disabled, the problem of FAYT is still there.
For a CJK user using Firefox with FAYT enabled, he will quickly find FAYT
does work for CJK characters (and will sometimes close findbar if he tries to
click in the input area) while the Control-F does work "with same user
interface". Apparently he will then consider this a bug and come report here.

Comment 17

[chris hofmann]

if possbible lets try and fix or find away to disable FAYT for JA builds.

Comment 18

[Jeff Walden (gone starting June 8)]

(In reply to comment #12)
> We've never actually exposed the "/" method of searching in Firefox--it was just
> a carry-over from Seamonkey--so while I will certainly do my best to get this
> bug fixed, I don't understand the significance that some attach to it.

It's been in Firefox Help (built-in, texturizer, and now
products/firefox/support/) since the beginning of time, so it has been exposed.
 If you want, we can remove it from (built-in) Help if necessary as an
"advanced" feature (built-in Help doesn't touch advanced topics very much -- we
leave that to resources for the tweakers, not the average Joe).

Comment 19

[Jungshik Shin]

(In reply to comment #14)
> Actually, it works like this

  What you described is correct for most Japanese and Chinese IMEs, but not 100%
correct for Korean IMEs. 

> you enter base characters and they would be converted to real characters
> but at this time, they're still not really inputted, it just shows on the screen
> and the string remains in the IME buffer, not the real input text box
> after you verify all the converted characters are correct,

Up to this part, it's the case for Korean IMEs as well. 

> you can press enter
> and the string will be really inputted into the input box

  Most Korean IMEs automatically commit the content of  their internal buffer
without requiring any explicit commit 'signal' like 'pressing enter' because in
case of Korean IMEs, the boundary between Korean and non-Korean input is easy to
tell without any user-intervention in most cases.  

Comment 20

[Blake Ross]

*** Bug 251891 has been marked as a duplicate of this bug. ***

Comment 21

[Blake Ross]

There just isn't time to fix this the right way for 1.0. I'd like to propose a
possible solution. Since normal (Ctrl+F) mode works, I would like to add a pref
that controls whether or not typeahead find mode is supported. The owners of
those localizations for which typeaheadfind does not work (since it does work
for some IMEs) can then flip the pref, and / won't do anything anymore, and the
typeahead find pref in Advanced will disappear. This will require an addition to
a .properties file, so localizers will need to sign off on this.

Comment 22

[Taku0]

Ctrl+F is not work perfectory.
Using the find dialog box in Firefox 0.93 or FAYT, I could search ancher text
and press enter to activate the link, but can not Ctrl+F in 1.0PR.
This is a indispensable feature to control Firefox using only keyboard.
I can still search and press enter to activate link by FAYT, but it is
restricted to ascii text by the bug 251891.
Therefore I need the bug 251891 to be fixed or Ctrl+F behaves like the find
dialog box in Firefox 0.93.

Comment 23

[Benjamin Smedberg]

Why would this require a properties change? We can localize arbitrary prefs.

Comment 24

[Jungshik Shin]

I would prefer to back out this thing because to me it's more important to make
things work (as has been known to users for a long time whether it's
'advertised' or not) than to do it neatly (with XUL). Moreover, disabling it in
localized builds (for CJK and some more locales. it's not just Japanese) doesn't
shield everyone from this problem. Note that not everyone uses localized builds.  

At the same time, I understand why you're reluctant to do that(reverting to cpp). 

So, what is to be done?   An important issue (the keyboard control and
accessibility) was raised in comment #22. Would it be possible to get CTRL-F to
work as before?  

Comment 25

[Blake Ross]

It's very important to me too that things work. However, I don't think we can
justify backing out the find toolbar this late in the game because a pretty
hidden, fairly advanced feature doesn't work for some users.

As for the issue in comment 22: the problem there is very much the same. The
reason why Ctrl+F works with IME right now is because the editor takes care of
it for us. If we were to focus found links, then focus would move from the
textbox to the browser, and future keystrokes wouldn't work properly. What I
could try to do is change the behavior of Ctrl+F (only if you have this pref
toggled, which the right localized builds would) such that when the user stops
typing, if the found result is a link, *then* focus would move to that link. In
other words, a timer with a short delay would fire after the user finishes
typing and move focus to the link. Is that a fair compromise for 1.0?

Comment 26

[Hung-Te Lin]

Attachment: The table of all combinations to invoke findbar

I made a table to list all combinations to invoke findbar.
From this table we can see that each type is different from others.
Maybe we should consider what to be preserved and what to be changed.

Comment 27

[Hung-Te Lin]

According to the table, maybe we can change like this:
First, redefine ENTER action:
 ENTER = find next text/link, 
 Ctrl-Enter = follow the link or donothing if target is text.

And for invocation methods:
(1) Ctr-F: not changed.
(2) /: change to invoke Ctrl-F
(3) ': change to invoke Ctrl-F, or disable this forever.
(4) FAYT: disable it forever (and for everyone, not only CJK)because we have
Ctrl-F now.
 Or just leave it untouched with the option disabled by default.
 
The proposed changes should be able to be patched easier than writing WM_CHAR
support in XUL or other solutions so we can put it into Firefox 1.0?

Comment 28

[Hung-Te Lin]

(In reply to comment #25)
> because a pretty hidden, fairly advanced feature doesn't work for some users.
 That's not true. maybe the "/" and "'" are 'hidden' , but FAYT is not hidden
 and not advanced, and we are not "SOME" users.
 
 Apparently every CJK user will notice this "bug" immediately. Many people 
 used Firefox 0.9 and they knew  FAYT, so they will consider that as a 
 regression which would be a great disappointment for the users installing 
 the long-awaited firefox 1.0.

Comment 29

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

I think following patch is solved this problem.
But it is not perfectly.
Because it cannot use IME for first char of find string.
However, we can use "/" or "'". It is enough.

 function onBrowserKeyPress(evt)
 {  
   // Check focused elt
   if (!shouldFastFind(evt))
     return;
  
+  focusFindBar();
  
   var findField = document.getElementById("find-field");

Comment 30

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

Oops...

It doesn't work fine with "'".
Because find toolbar is lost focus by found link.

Comment 31

[Blake Ross]

Attachment: event patch

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 163107 [details] [diff] [review]
event patch

I'm not sure what this is, but there's no way it's even going to compile
without nsDOMEvent.h changes

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 163107 [details] [diff] [review]
event patch

nor nsLayoutAtomList.h changes.

And there's a gratuitous whitespace change in the nsDOMEvent.cpp changes.

(And to fix the bug I'd expect some JS changes too.  Perhaps you have those
too, but just didn't attach them.)

Comment 34

[Brian Ryner (not reading)]

Attachment: revised events patch

Ok, so the idea here is to allow the following events to be listened for from
javascript:

compositionstart
compositionend
text

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

This is missing the train. 

Comment 36

[Asa Dotzler [:asa]]

We're going to look at this for Firefox 1.1. If blake's got patches, we'll try
to get those into the bug and try to be working on this on the trunk early for 1.1.

Comment 37

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

(In reply to comment #34)
> Created an attachment (id=163193)
> revised events patch
> 
> Ok, so the idea here is to allow the following events to be listened for from
> javascript:
> 
> compositionstart
> compositionend
> text
> 

It is not good idea.
Because composition string should be managed by C++ code, not Javascript code.
If we manage with Javascript, we cannot get feedback.
The composition string state is not one.
See http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#5872
The states are NS_TEXTRANGE_RAWINPUT, NS_TEXTRANGE_CONVERTEDTEXT,
NS_TEXTRANGE_SELECTEDRAWTEXT and NS_TEXTRANGE_SELECTEDCONVERTEDTEXT.

Comment 38

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

I have a question.
Where is FAYT source code after build?
I cannot find findBar.js in installed folder.

Comment 39

[Hiro]

(In reply to comment #38)
> I have a question.
> Where is FAYT source code after build?
> I cannot find findBar.js in installed folder.

They may be the following places.
toolkit/components/typeaheadfind/

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=AVIARY_1_0_20040515_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-07-07+01%3A31&maxdate=2004-07-07+01%3A31&cvsroot=%2Fcvsroot

Comment 40

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

No. I want to know findBar.js that is added by Bug 250279.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #40)
> No. I want to know findBar.js that is added by Bug 250279.

It's in chrome/toolkit.jar , then the content/global/findBar.js path within that
archive.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

*** Bug 270936 has been marked as a duplicate of this bug. ***

Comment 43

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

Attachment: Patch

This patch is fixed IME problem.
But it makes regression that user cannot access the found link.

We must emulate the current behavior when user press DOV_VK_RETURN key.

Comment 44

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

Blake Ross:

Do you have an idea for emulate the current behavior?

Comment 45

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

Attachment: Patch of my concept

Comment 46

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

Attachment: Patch of my concept rv1.1

Comment 47

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

I think that my concept is correctly.
We need to disable IME on other of Editor for bug 55751 and bug 113187.
So, if IME is disabled on other of Editor, we cannot use IME when FAYT, forever.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: a continuation of bryner's approach

Comment 49

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

David:

However, we cannot fix bug 55751 and bug 113187 by the patch...

Comment 50

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

We should not process IME event in non-editor widget.

Because other applications disable IME in its non-editor widget on Windows OS. 
But Mozilla enable IME in all widgets. Therefore, we have bug 55751 and bug
113187. These bugs are very disrepute in Japan. And I'm working on bug 55751 and
bug 113187 now.

Please change to another approach for fixing this bug.

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 181606 [details] [diff] [review]
a continuation of bryner's approach

OK, that's fine.  Is the patch for the other approach close to ready?  And does
it behave pretty much the same (regarding making the find bar go away after a
timeout) when the user finds with "/" or "'"?

Comment 52

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

No. I'm not working in this bug.
But I think that my approach is better way(however, it is difficult).

Comment 53

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

Attachment: proposal patch

I propose this approach.
But this approach has a regression.
The focus is not painted for link.
Because the focus is always set to find toolbar while FAYT.

Comment 54

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

*** Bug 269222 has been marked as a duplicate of this bug. ***

Comment 55

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

Attachment: Patch rv1.0

O.K. This patch fixes the regression of drawing focus rect for the found link.
Please check it.

Comment 56

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

Attachment: Patch rv1.0.1

Update for bug 291613.

Comment 57

[Anbo Motohiko]

(In reply to comment #53)
> I propose this approach.
> But this approach has a regression.
> The focus is not painted for link.
> Because the focus is always set to find toolbar while FAYT.
Another regression of your approach will be Bug 264841.

To avoid this, follow found link if Enter key is pressed in FAYT-mode. Is this
difficult?
And isn't my way bad for screen readers?

IMHO, without enter-key-hack, this behavor should be pref-controlable and
disabled by default.
For CJK and other locales where IME is often used, enable this pref in their
language packs.

Comment 58

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

Motohiko:

Are you read the patch?
The patch send the event to found link if Enter key is pressed in FAYT-mode.

Comment 59

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

Attachment: Patch rv1.1

Comment 60

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

Comment on attachment 182260 [details] [diff] [review]
Patch rv1.1

I found a bug.
In IME composing, we should not timeup the FAYT mode.

Comment 61

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

Attachment: Patch rv2.0

Comment 62

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

Attachment: Patch rv2.0 -u10pw

Comment 63

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

Comment on attachment 182272 [details] [diff] [review]
Patch rv2.0

I found another bug, we should not process KeyPress event while IME is
composing.

Comment 64

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

Attachment: Patch rv3.0

Comment 65

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

Comment on attachment 182280 [details] [diff] [review]
Patch rv3.0

Umm... Sorry.
rv3.0 doesn't work fine with Korean IME.

Comment 66

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

Comment on attachment 182280 [details] [diff] [review]
Patch rv3.0

jshin:

Please test the Patch rv3.0(attachment 182280 [details] [diff] [review]) with Korean IME.
In the patch, if using Korean IME, we need to press Enter for find trigger.
Is it OK?

If we call find() in every oncompositionend, Korean IME is aborted when first
key press for 2nd or more charachter.
I think that it is limit of this approach.

If you grant it, please change to r=mconner, sr=dbaron.

Comment 67

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

Attachment: Patch rv3.0 -u10pw

Comment 68

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

dbaron:

If we can get found link without focus switching, I create better patch.
Is it possible?

Comment 69

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is *what* possible?

Comment 70

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

Can I get the found link element(that is not focused)?

Comment 71

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

Comment on attachment 182280 [details] [diff] [review]
Patch rv3.0

I successed non-focus-switching way.
I will create new patch tomorrow.

Comment 72

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

Attachment: Patch rv4.0

Comment 73

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

Attachment: Patch rv4.0 -u10pw

Comment 74

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

Attachment: Patch rv4.0

Comment 75

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

Attachment: Patch rv4.0 -u10pw

Comment 76

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

This patch is following fix.
1. The key event is processed in editor.
2. If it finds the link in FAYT mode, the link is changed outline.
3. If IME is composing, the find toolbar is not closed in FAYT mode.
4. If the find toolbar is closed by timeout in FAYT mode, focus is set to found
link.

Comment 77

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

Aaron and dbaron:

Umm...

I think that following function has a bug.
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#4577
void nsEventStateManager::FocusElementButNotDocument(nsIContent *aContent)

I think that if the document doesn't have focus, we shouldn't set new element to
focus controller. I.e.,

  nsIFocusController *focusController =
    GetFocusControllerForDocument(mDocument);
  if (!focusController)
      return;

  // Get previous focus
  nsCOMPtr<nsIDOMElement> oldFocusedElement;
  focusController->GetFocusedElement(getter_AddRefs(oldFocusedElement));
  nsCOMPtr<nsIContent> oldFocusedContent(do_QueryInterface(oldFocusedElement));
#if 0
  // Notify focus controller of new focus for this document
  nsCOMPtr<nsIDOMElement> newFocusedElement(do_QueryInterface(aContent));
  focusController->SetFocusedElement(newFocusedElement);
#endif
  // Temporarily set mCurrentFocus so that esm::GetContentState() tells
  // layout system to show focus on this element.
  SetFocusedContent(aContent);  // Reset back to null at the end of this method.
  mDocument->BeginUpdate(UPDATE_CONTENT_STATE);
  mDocument->ContentStatesChanged(oldFocusedContent, aContent,
                                  NS_EVENT_STATE_FOCUS);


If it is so, 'document.commandDispatcher.focusedElement' is not broken by find.

Comment 78

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

Attachment: Patch rv4.1

Comment 79

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

Attachment: Patch rv4.1 -u10pw

Comment 80

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

test build for windows, here.
http://www.d-toybox.com/mozilla/testbuilds/bug3976.zip

Comment 81

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

Attachment: Patch rv4.2

The tester found a focus problem.

Comment 82

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

Attachment: Patch rv4.2 -u10pw

Comment 83

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

*** Bug 292900 has been marked as a duplicate of this bug. ***

Comment 84

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

Attachment: Patch rv4.2 (content/)

Comment 85

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

Attachment: Patch rv4.2 (browser/, toolkit/)

mconnor:

I think that we need the pseudo focus style. Because if it is not rendered,
the focused link is unclear.

Comment 86

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

Attachment: Patch rv4.2 (content/)

Updating for latest trunk.

Comment 87

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

Attachment: Patch rv4.2 (browser/ toolkit/)

Comment 88

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

Attachment: Patch rv4.2

Comment 89

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

Attachment: Patch rv4.2 -u20pw

Comment 90

[Brian Ryner (not reading)]

Comment on attachment 183639 [details] [diff] [review]
Patch rv4.2 (content/)

aaronl knows much more about FocusElementButNotDocument than I do, so I'm
asking him to review this.

I am skeptical of the GetLastFocusedContent change, it seems like yet another
focus tracking variable that can get out of sync (we're already tracking
something like this in the focus controller).

Can you describe why the current code that locates the found link works
correctly for non-IME input but breaks for IME input?

Comment 91

[Brian Ryner (not reading)]

Comment on attachment 183639 [details] [diff] [review]
Patch rv4.2 (content/)

actually setting reviewer to aaronl

Comment 92

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

> Can you describe why the current code that locates the found link works
> correctly for non-IME input but breaks for IME input?

On current code, browser content area has a focus when FAYT. Therefore, the
found link element can have focus. But if it has focus, we cannot use IME.
Because if we want to use IME, the editor should have focus.

My patch makes to set the focus to editor every time while FAYT. So I need found
link that doesn't have focus.

Comment 93

[Brian Ryner (not reading)]

(In reply to comment #92)
> My patch makes to set the focus to editor every time while FAYT. So I need found
> link that doesn't have focus.
> 

Ok, so when the link is found, it receives focus briefly?  Where does that happen?

I think I understand the basic approach, but I'd prefer if the found link could
be tracked in the FAYT code instead of in the EventStateManager.  Having it on
the ESM is ambiguous in my opinion because it's not clear whether it's the
previous focus globally, or for the current toplevel window, or just for the
document the ESM is attached to.

Comment 94

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

See another patch. In nsTypeAheadFind.cpp, it uses ESM's |MoveFocusToCaret|.
So, I cannot get the found link without ESM.

>        if (mFocusLinks) {
>          nsIEventStateManager *esm = presContext->EventStateManager();
>          PRBool isSelectionWithFocus;
>          esm->MoveFocusToCaret(PR_TRUE, &isSelectionWithFocus);
> +        nsCOMPtr<nsIContent> lastFocusedContent;
> +        esm->GetLastFocusedContent(getter_AddRefs(lastFocusedContent));
> +        nsCOMPtr<nsIDOMElement>
> +          lastFocusedElement(do_QueryInterface(lastFocusedContent));
> +        mFoundLink = lastFocusedElement;
>        }

Comment 95

[Brian Ryner (not reading)]

Ok.  I am fine with the GetLastFocusedContent change if you add a comment to
nsIEventStateManager.h describing what it does.  For example,

// Get the previously-focused content node for this document

I'd still like to get Aaron's opinion on removing the call to
nsIFocusController::SetFocusedElement().  Comments in the code (which were
removed in rev 1.451) seem to indicate that this call is needed:

// Make sure our document's window's focus controller knows the focus has changed
// That way when our window is activated (PreHandleEvent get NS_ACTIVATE), the
correct element & doc get focused

Comment 96

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

Brian:

Cannot you review for another patch(attachment 183640 [details] [diff] [review])?
I don't get the reply from mconner...

Comment 97

[Aaron Leventhal]

(In reply to comment #95)
> I'd still like to get Aaron's opinion on removing the call to
> nsIFocusController::SetFocusedElement().  Comments in the code (which were
> removed in rev 1.451) seem to indicate that this call is needed:

Why do we need to remove it? It was added to fix bug 159998 -- please see that
bug. Changing core focus stuff like this at this stage of the trunk is not a
good idea. As Chris Saari told me, no one ever been able to make changes to the
way nsEventStateManager handles focus without causing regressions. What's the
exact story with this change?

Comment 98

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

Attachment: Patch rv4.3 (content/)

O.K. I removed the changing that is removing
nsIFocusController::SetFocusedElement().
And adding the comment to nsIEventStateManager.h

Comment 99

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

Attachment: Patch rv4.3 (browser/ toolkit/)

Brian:
Please review it too. I cannot trust mconner. He ignores my request and mail
while several week...

Comment 100

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

Attachment: Patch rv4.3

Comment 101

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

Attachment: Patch rv4.4 (content/)

Oops... Sorry, my mistake.

Comment 102

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

Attachment: Patch rv4.4

Comment 103

[Asa Dotzler [:asa]]

Potential for fallout so we need this in for b3 if it's gonna make 1.1.

Comment 104

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 185294 [details] [diff] [review]
Patch rv4.4 (content/)

You should probably modify nsEventStateManager::ContentRemoved to clear
mLastFocus when the content node is removed.

Is the change to nsXULElement.cpp just adding oncompositionstart and
oncompositionend?

Comment 105

[Aaron Leventhal]

Comment on attachment 185294 [details] [diff] [review]
Patch rv4.4 (content/)

I am sorry, I won't have time to review this any time soon.

Comment 106

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

(In reply to comment #104)
> Is the change to nsXULElement.cpp just adding oncompositionstart and
> oncompositionend?

Yes.

Comment 107

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

Comment on attachment 185294 [details] [diff] [review]
Patch rv4.4 (content/)

Please wait. Too, we MUST remove here.

  // Notify focus controller of new focus for this document



  nsCOMPtr<nsIDOMElement> newFocusedElement(do_QueryInterface(aContent));



  focusController->SetFocusedElement(newFocusedElement);

Comment 108

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

Attachment: Patch rv4.5 (browser/ toolkit/)

This patch doesn't work fine when we press enter in FAYT mode, but it is
another bug see bug 298423.

Comment 109

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

Attachment: Patch rv4.5 (content/)

Comment 110

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

I removed following code from nsEventStateManager.cpp,

  // Notify focus controller of new focus for this document
  nsCOMPtr<nsIDOMElement> newFocusedElement(do_QueryInterface(aContent));
  focusController->SetFocusedElement(newFocusedElement);

but I cannot reproduce bug 159998. I think that it is not necessary for bug 159998.

Comment 111

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

Attachment: Patch rv4.5

Comment 112

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

Attachment: Patch rv4.6 (browser/ toolkit/)

It solves previous problem. Please review it.

Comment 113

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

Attachment: Patch rv4.6

Comment 114

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

Attachment: Patch rv4.7 (browser/ toolkit/)

Comment 115

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

Attachment: Patch rv4.7

Comment 116

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

*** Bug 255334 has been marked as a duplicate of this bug. ***

Comment 117

[Brian Ryner (not reading)]

Comment on attachment 187034 [details] [diff] [review]
Patch rv4.7 (browser/ toolkit/)

Looks good, just one comment.  This block:

+  var isFAYT = (gFindMode != FIND_NORMAL);
+  if (isFAYT) {
+    if (res == Components.interfaces.nsITypeAheadFind.FIND_NOTFOUND || !val)
+      setFoundLink(null);
+    else
+      setFoundLink(fastFind.foundLink);
+  }

is repeated a few times, maybe pull it out into a new function
(updateFoundLink).  r=me with that change.

Comment 118

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

Attachment: Patch rv4.8 (browser/ toolkit/)

Comment 119

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

Attachment: Patch rv4.8

Comment 120

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

Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.263; previous revision: 1.262
done
Checking in content/events/public/nsIEventStateManager.h;
/cvsroot/mozilla/content/events/public/nsIEventStateManager.h,v  <-- 
nsIEventStateManager.h
new revision: 1.63; previous revision: 1.62
done
Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.579; previous revision: 1.578
done
Checking in content/events/src/nsEventStateManager.h;
/cvsroot/mozilla/content/events/src/nsEventStateManager.h,v  <-- 
nsEventStateManager.h
new revision: 1.135; previous revision: 1.134
done
Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.cpp
new revision: 1.576; previous revision: 1.575
done
Checking in toolkit/components/typeaheadfind/content/findBar.inc;
/cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.inc,v  <-- 
findBar.inc
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/typeaheadfind/content/findBar.js;
/cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.js,v  <-- 
findBar.js
new revision: 1.17; previous revision: 1.16
done
Checking in toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl;
/cvsroot/mozilla/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl,v
 <--  nsITypeAheadFind.idl
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp;
/cvsroot/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp,v  <--
 nsTypeAheadFind.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/typeaheadfind/src/nsTypeAheadFind.h;
/cvsroot/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.h,v  <-- 
nsTypeAheadFind.h
new revision: 1.4; previous revision: 1.3
done

Comment 121

[Nickolay_Ponomarev]

*** Bug 288141 has been marked as a duplicate of this bug. ***

Comment 122

[Adam Guthrie]

*** Bug 306968 has been marked as a duplicate of this bug. ***

Comment 123

[:Gavin Sharp [email: [e-mail]]]

*** Bug 271580 has been marked as a duplicate of this bug. ***