433340 - bookmark dialog covers candidate window when using IME

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Masahiro YAMADA]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051222 Minefield/3.0pre

When inputting Tag string in bookmark dialog,
candidate window is covered by bookmark dialog


Reproducible: Always

Steps to Reproduce:
1.click star in location bar
2.Click Tags textbox
3.enable IME
4.input some word and show candidate window
Actual Results:  
candidate window is covered by bookmark dialog

Expected Results:  
candidate window should not be covered by bookmark dialog

Screenshot is here:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3771

Comment 1

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

I can reproduce this bug on Mac too. This is too bad accessibility bug for Japanese users. The candidate window is too important.

Comment 2

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

Attachment: Patch v1.0

If we add a new attribute for panel element, we can fix this bug with very low risk.

Comment 3

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

The panel element which doesn't have noautohide="true", that is top most window. It hides other normal z-index windows. So, the IME candidate window is hide by the panel. I add a notopmost attribute. If that is true, the panel has normal z-index, so, that cannot hide the IME candidate window. And also the auto hide behavior is not killed.

Comment 4

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

Attachment: Patch v1.0.1

adding comments.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think we'll block on this, but we can take it in RC2 (if we do it) or a point release. Enn/Dietrich/Mano can you comment or review?

Comment 6

[Neil Deakin]

Why are you only fixing this for the 'bookmark dialog'? Wouldn't the same bug apply to all popups?

Comment 7

[John Daggett (:jtd)]

Attachment: screenshot, input method example

Sorry but I really think this needs to block.  The use of this popup is an
absolute requirement for inputing Japanese text.  

The input method popup allows alternates to the primary choice to be selected. 
For example, say you wanted to enter 化学(chemistry).  In Japanese this is
inputted as "kagaku".  The word for science 科学 is spelled the same way.

With the input method enabled (on a Mac, choose 'Hiragana' on the keyboard
menu), enter 'k'.  

Steps:

1. Switch the input method to Japanese (on a Mac, choose 'Hiragana' on the
keyboard menu)
2. Enter 'k' (k with an underline appears)
3. Enter 'a' (か with underline appears)
4. Enter 'gaku' (かがく with underline appears)
5. Hit the spacebar once (科学 with underline appears)

This reflects the fact that 科学 is the most probable choice.

6. Hit spacebar again.  A popup now appears with other choices, including
化学.  Using the arrow keys allows you to choose another compound.
7. Select 化学 and hit enter twice, underline no longer appears.

This type of character input is *very* common with Japanese text, it is not a
simple nicety.

Comment 8

[Gen Kanai [:gen]]

(In reply to comment #5)
> Don't think we'll block on this, but we can take it in RC2 (if we do it) or a
> point release. 

Mike, this makes the functionality basically unusable.  I'd have to disagree with your assessment.  This impacts all of our non-Roman language locales.

Comment 9

[Gen Kanai [:gen]]

(In reply to comment #6)
> Why are you only fixing this for the 'bookmark dialog'? Wouldn't the same bug
> apply to all popups?

You're right.  We need to double-check to make sure that we don't have similar issues with other popups.  Those would be separate bugs, of course.

Comment 10

[Gen Kanai [:gen]]

(In reply to comment #8)
> This impacts all of our non-Roman language locales.
> 

Apologies- that above statement of mine is too broad.  It certainly impacts all locales which are IME-driven.

Comment 11

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

Is this a recent regression? Why did we only discover it now?

Comment 12

[Gen Kanai [:gen]]

(In reply to comment #11)
> Is this a recent regression? Why did we only discover it now?
> 

Gavin, good question.  At the core, I think it is because we don't have as much testing in Asia overall as we have elsewhere.

More focused on this particular bug, the problem was harder to find due to IME behavior.  If you regularly use an IME, you'll know that the IME tries to predict what character sets you want to input.  This bug rears its head only when the initial prediction is wrong and the user must select a different character than what the IME suggested.  Testing that was done until now did not go deep enough into IME behavior.

Comment 13

[Channy Yun [:channy]]

Attachment: screenshot, error of input chinese character in ko IME

In Korean IME, it's less than Japanese because Korean almost don't use Chinese chracter in general writing. If someone want to input Chinese character using toggle key, it shows same problem in this issue. 

I guess its effect is significant in Chinese version rather than Japanese or Korean because Chinese character can be input by English typing transition. Right?

Comment 14

[Po-chiang Chao [:bobchao]]

I think that most user in Taiwan don't input Chinese by English typing transition, but people in China do.

In Microsoft New IME (新注音/新倉頡) the candidate window is correctly pop-up on the bookmark dialog, GCIN on linux is the same. It seems (luckily) not a problem for us. But in Search field we meet the same problem, the search suggestions panel do covers the candidate window when use GCIN in Linux. Should I fire another bug or it's the same situation? 

Also, when using Natural IME (another popular IME in Taiwan), bookmarks windows sometimes disappear after enable the IME in the tag field. 

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It's a problem for Chewing (新酷音) IME users on Windows. I agree with previous comments regards blocking. I couldn't test the search field, however, because the suggestion simply gone when I start typing new words.

(In reply to comment #14)
> In Microsoft New IME (新注音/新倉頡) the candidate window is correctly
> pop-up on the bookmark dialog, GCIN on linux is the same. It seems (luckily)
> not a problem for us. But in Search field we meet the same problem, the search
> suggestions panel do covers the candidate window when use GCIN in Linux. Should
> I fire another bug or it's the same situation? 

Comment 16

[Gen Kanai [:gen]]

(In reply to comment #14)
> Should I fire another bug or it's the same situation? 
> 

Bob, please file another bug and please add a screenshot for attachment. Please cc me as well.

> Also, when using Natural IME (another popular IME in Taiwan), bookmarks windows
> sometimes disappear after enable the IME in the tag field. 
> 

Again, please file another bug and add a screenshot.  Please cc me :)

Comment 17

[Jia Mi [:mijia]]

We tested on Simplify Chinese IME for this bug.

1) In Mac, cannot be reproduced, the IMEs we tested are F.I.T and QIM, they are all working well.
2) In Windows, the bug can be reproduced, the IMEs we tested are Google IME, Sogou IME, and Microsoft IME. Google and Sogou IME are reproducing the bug everytime. But MS IME works well for sometimes.
3) In Ubuntu, the bug cannot be reproduced with fctix IME.

I will attach Windows bug screenshot later.

Comment 18

[Jia Mi [:mijia]]

Attachment: Screenshot, Chinses IME of Sogou Pinyin

Comment 19

[Jia Mi [:mijia]]

Attachment: Screenshot, Chinese IME, Guge(Google.cn) Pinyin

Comment 20

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

(In reply to comment #6)
> Why are you only fixing this for the 'bookmark dialog'? Wouldn't the same bug
> apply to all popups?

Right. I was choice the lowest risk way. Basically, all panel elements should not be top-most window. Because they are container of any elements. So, it can have input/textarea elements. The same bug may be reproduced with some extensions, it's not good.

I think that panel should be non-top-most window at default. However, the top-most-window behavior is already released by our beta releases. So, some extension developers might like the behavior. So, we must leave the top-most-window specifying way for them.

I'll post the new patch that is an ideal fix of this bug for me.

Comment 21

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

(In reply to comment #12)
> (In reply to comment #11)
> > Is this a recent regression? Why did we only discover it now?
> > 
> 
> Gavin, good question.  At the core, I think it is because we don't have as much
> testing in Asia overall as we have elsewhere.

We need to think the i18n testing for new UIs after 1.9.

Comment 22

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

Attachment: Patch v2.0

This is ideal patch, I think.

I have an worry. I'm not sure this is needed: |!mInContentShell|. Why this is needed in IsNoAutoHide()?

Comment 23

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

Comment on attachment 320859 [details] [diff] [review]
Patch v2.0

oops, this may break many features.

Comment 24

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

Attachment: Patch v2.1

This is correct.

We are using panel element at:

* Autocomplete candidate window of location bar.
* Autocomplete candidate window of others (e.g., password manager, contents)
* Toolbar customizing dialog
* Site identity popup

I think that this patch is safe for them.

> accessible\src\xul\nsXULSelectAccessible.cpp(235):     // and has a parent popup <panel>
> browser\base\content\browser.xul(100):     <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoComplete" noautofocus="true" hidden="true"/>
> browser\base\content\browser.xul(103):     <panel type="autocomplete-richlistbox" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true" hidden="true"/>
> browser\base\content\browser.xul(105):     <panel id="editBookmarkPanel"
> browser\base\content\browser.xul(193):     <panel id="identity-popup" position="after_start" hidden="true" noautofocus="true"
> browser\base\content\browser.xul(523):   <panel id="customizeToolbarSheetPopup" noautohide="true">
> extensions\manticore\browser\PrefPanels.xml(3): <panels xmlns="http://www.silverstone.net.nz/2001/manticore/uidl">
> extensions\manticore\browser\PrefPanels.xml(4):   <panel label="Web Browser">
> extensions\manticore\browser\PrefPanels.xml(5):     <panel label="Browser Display" id="browser-display"/>
> firefox-debug-build\_tests\testing\mochitest\chrome\toolkit\content\tests\chrome\window_panel_focus.xul(20): <panel id="panel" norestorefocus="true" onpopupshown="panelShown()" onpopuphidden="panelHidden()">
> firefox-debug-build\_tests\testing\mochitest\chrome\toolkit\content\tests\chrome\window_panel_focus.xul(32): <panel id="noautofocusPanel" noautofocus="true"
> firefox-debug-build\_tests\testing\mochitest\tests\toolkit\content\tests\widgets\test_panelfrommenu.xul(32): <panel id="panel" onpopupshown="panelOpened()" onpopuphidden="SimpleTest.finish()">
> firefox-debug-build\_tests\testing\mochitest\tests\toolkit\content\tests\widgets\test_popup_coords.xul(19): <panel id="outerpopup"
> firefox-debug-build\_tests\testing\mochitest\tests\toolkit\content\tests\widgets\test_popup_tree.xul(13): <panel id="panel" onpopupshown="treeClick()" onpopuphidden="SimpleTest.finish()">
> mail\base\content\messageWindow.xul(174):   <panel id="customizeToolbarSheetPopup" noautohide="true">
> mail\base\content\messenger.xul(417):   <panel id="customizeToolbarSheetPopup" noautohide="true">
> mail\components\addrbook\content\addressbook.xul(784):   <panel id="customizeToolbarSheetPopup" noautohide="true">
> mail\components\compose\content\messengercompose.xul(773):   <panel id="customizeToolbarSheetPopup" noautohide="true">
> suite\browser\navigator.xul(137):   <panel type="autocomplete" id="PopupAutoComplete"/>
> toolkit\content\tests\chrome\window_panel_focus.xul(20): <panel id="panel" norestorefocus="true" onpopupshown="panelShown()" onpopuphidden="panelHidden()">
> toolkit\content\tests\chrome\window_panel_focus.xul(32): <panel id="noautofocusPanel" noautofocus="true"
> toolkit\content\tests\widgets\test_panelfrommenu.xul(32): <panel id="panel" onpopupshown="panelOpened()" onpopuphidden="SimpleTest.finish()">
> toolkit\content\tests\widgets\test_popup_coords.xul(19): <panel id="outerpopup"
> toolkit\content\tests\widgets\test_popup_tree.xul(13): <panel id="panel" onpopupshown="treeClick()" onpopuphidden="SimpleTest.finish()">

Comment 25

[Neil Deakin]

> I have an worry. I'm not sure this is needed: |!mInContentShell|. Why this is
> needed in IsNoAutoHide()?

So that web pages cannot create popups that the user cannot hide.

The 'topmost' feature doesn't require such a check. If anything, IsTopMost should either just not check mInContentShell or return false rather than true.

Comment 26

[Hideo Oshima]

(In reply to comment #24)
> Created an attachment (id=320864) [details]
> Patch v2.1
> 
> This is correct.

I cannot compile with this patch.

Here is error log.

c++ -o nsMenuPopupFrame.o -c -I../../../../dist/include/system_wrappers -include /home/hideo/develop/mozilla/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -D_IMPL_NS_LAYOUT -I/home/hideo/develop/mozilla/layout/xul/base/src -I/home/hideo/develop/mozilla/layout/xul/base/src/../../../base -I/home/hideo/develop/mozilla/layout/xul/base/src/../../../../content/events/src -I/home/hideo/develop/mozilla/layout/xul/base/src/../../../generic -I/home/hideo/develop/mozilla/layout/xul/base/src/../../../style  -I/home/hideo/develop/mozilla/layout/xul/base/src -I. -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/dom -I../../../../dist/include/locale -I../../../../dist/include/content -I../../../../dist/include/gfx -I../../../../dist/include/thebes -I../../../../dist/include/widget -I../../../../dist/include/view -I../../../../dist/include/docshell -I../../../../dist/include/necko -I../../../../dist/include/webshell -I../../../../dist/include/pref -I../../../../dist/include/intl -I../../../../dist/include/imglib2 -I../../../../dist/include/unicharutil -I../../../../dist/include/xpconnect -I../../../../dist/include/js -I../../../../dist/include/xultmpl -I../../../../dist/include/cairo -I../../../../dist/include/libpixman -I../../../../dist/include   -I../../../../dist/include/layout -I../../../../dist/include/nspr     -I../../../../dist/sdk/include    -fPIC   -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O2   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsMenuPopupFrame.pp /home/hideo/develop/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp
/home/hideo/develop/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp: In member function ‘PRBool nsMenuPopupFrame::IsTopMost()’:
/home/hideo/develop/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:204: error: expected ‘;’ before ‘)’ token
/home/hideo/develop/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:204: error: expected primary-expression before ‘)’ token
/home/hideo/develop/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:204: error: expected `;' before ‘)’ token
make[7]: *** [nsMenuPopupFrame.o] エラー 1

Debian GNU/Linux Sid
gcc 4.2.3
Linux kernel 2.6.25

If more information is needed, please tell me.

Comment 27

[Atsushi Sakai]

Attachment: Patch v2.1.1

Remove unnecessary ')' from Patch v2.1.
This works for me.

Comment 28

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

Attachment: Patch v2.2

Thank you for your advice!

Comment 29

[Neil Deakin]

Comment on attachment 320922 [details] [diff] [review]
Patch v2.2

Looks good. I'll just suggest better grammar for the comments.
> 
>+PRBool
>+nsMenuPopupFrame::IsTopMost()
>+{
>+  // If this panel is in content shell or not a panel element,
>+  // this is alwyas top-most window.

// If this panel is not a panel, this is always a top-most popup

>+  // The web pages should not be able to create a top-most window, because that
>+  // may not be able to close. And if noautohide is true, then the panel should
>+  // not be top-most too.

// Web pages should not be able to create a top-most popup. If this panel is
// a noautohide panel, it should appear just above the parent window.

>+  if (mInContentShell || IsNoAutoHide())
>+    return PR_FALSE;
>+  // If this is panel element, should prefer topmost attribute.

// Otherwise, check the topmost attribute.

> 
>-  // panels which don't auto-hide need a parent widget. This allows them
>-  // to always appear in front of the parent window but behind other windows
>-  // that should be in front of it.
>+  // panels which don't auto-hide need a parent widget. or panels are not top
>+  // most window. This allows them to always appear in front of the parent
>+  // window but behind other windows that should be in front of it.

// panels which are not topmost need a parent widget. This allows them to
// always appear in front of the parent window but behind other windows that
// should be in front of it.

> 
>+  // returns true if the popup is a top most window.
>+  // Otherwise, the panels are always front of the parent window.
>+  PRBool IsTopMost();

// returns true if the popup is a top-most window. Otherwise, the
// panel appears in front of the parent window.

Comment 30

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

Attachment: Patch v2.2.1

Thank you!

Comment 31

[[e-mail]]

Comment on attachment 320928 [details] [diff] [review]
Patch v2.2.1

Well, I'm going to nit Enn's grammar nits!

>+  // If this panel is not a panel, this is always a top-most popup
"If this popup is not a panel, this is always a topmost popup."

>+  // Web pages should not be able to create a top-most popup.
"Web pages should not be able to create a topmost panel."

(They can create a top-most popup by virtue of the previous condition.)

Comment 32

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

Attachment: Patch v2.2.2

Even if this gets the approval, I cannot check-in the patch to tree now. Please check-in this patch after a1.9 to both trunk(hg) and 1.9.0 branch(cvs) if you can.

Comment 33

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot

Another screenshot, hit me pretty easily while testing RC1.

FWIW, I use hanyu pinyin to type Chinese characters, which means typing the English phonetic characters. And the screenshot shows that I am virtually unable to select the character I want, if it is blocked.

Comment 34

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #14)
> I think that most user in Taiwan don't input Chinese by English typing
> transition, but people in China do.

Agreed. Mainland Chinese usually use hanyu pinyin, along with folks from Singapore, possibly Malaysia among others as well.

(In reply to comment #7)
> Sorry but I really think this needs to block.  The use of this popup is an
> absolute requirement for inputing Japanese text.

(In reply to comment #10)
> Apologies- that above statement of mine is too broad.  It certainly impacts all
> locales which are IME-driven.

Agreed, especially regarding affecting a lot of IME-driven locales.

Comment 35

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008051604 Minefield/3.0pre

Doing further investigation shows that I can make the IMEs of all Traditional Chinese appear covered the tag dialog (Pinyin, Cangjie, Jianyi, Dayi(Pro), Hanin), i.e. those that are installed in Mac OS X, except that of Zhuyin, which I didn't know how to use. Hiragana and Katakana are also affected, for my little Japanese testing that I could comprehend.

This is in addition to the hanyu pinyu style for Simplified Chinese as demonstrated.

This affects both the Name and the Tags textfield in the dialog in comment #33.

I can test other platforms with IMEs if so requested.

Comment 36

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #35)
> Doing further investigation shows that I can make the IMEs of all Traditional
> Chinese appear covered the tag dialog (Pinyin, Cangjie, Jianyi, Dayi(Pro),
> Hanin), ...

s/appear covered the tag dialog/appear to be covered by the tag dialog

Comment 37

[Mike Connor [:mconnor]]

This is breaking a key feature for most IME users.  We'll have to do an RC2, this will not stop RC1 ship.

Comment 38

[Mike Connor [:mconnor]]

Discussed at length with 1.9 drivers.  We will not force a respin for only this issue.  We will have to fix in 3.0.1 (4-6 weeks after release).

Comment 39

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

(In reply to comment #38)
> Discussed at length with 1.9 drivers.  We will not force a respin for only this
> issue.  We will have to fix in 3.0.1 (4-6 weeks after release).

This will change the panel element behavior, so, that means we kill the compatibility of the extensions between 3.0 and 3.0.1. Is it ok?? (But probably, the changing doesn't break most extensions.)

Comment 40

[Dave Townsend [:mossop]]

(In reply to comment #39)
> (In reply to comment #38)
> > Discussed at length with 1.9 drivers.  We will not force a respin for only this
> > issue.  We will have to fix in 3.0.1 (4-6 weeks after release).
> 
> This will change the panel element behavior, so, that means we kill the
> compatibility of the extensions between 3.0 and 3.0.1. Is it ok?? (But
> probably, the changing doesn't break most extensions.)
> 

I've skimmed the bug but couldn't find it, could you summarize how this changes the behaviour of the panel element?

Comment 41

[Neil Deakin]

It makes <panel> elements not be topmost windows, and gives them a parent/owner window at the OS level.

Comment 42

[Dave Townsend [:mossop]]

(In reply to comment #41)
> It makes <panel> elements not be topmost windows, and gives them a parent/owner
> window at the OS level.

Thanks, so that sounds like it shouldn't impact pure XUL users.

Comment 43

[Mike Schroepfer]

Comment on attachment 321102 [details] [diff] [review]
Patch v2.2.2

a+ schrep for 3.0.1 or RC2 please land on cvs trunk.

Comment 44

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

(In reply to comment #43)
> (From update of attachment 321102 [details] [diff] [review])
> a+ schrep for 3.0.1 or RC2 please land on cvs trunk.

landed to cvs. I'm trying to land to Hg too.

Comment 45

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

(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 321102 [details] [diff] [review] [details])
> > a+ schrep for 3.0.1 or RC2 please land on cvs trunk.
> 
> landed to cvs. I'm trying to land to Hg too.

backed out the patch. it seems that the cause of test failure on linux.

Comment 46

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

Ugh... thinderbox is  all green now. Maybe, widget code of linux has a bug at focus handling....

Comment 47

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

ok. I understand what is a problem on Linux. On Linux, the popup window deactivates the parent window at opening. Therefore, the deactivate event kills popup window by auto-hide. So, the patch is too risky for Linux. And we need better approach for this bug.

Comment 48

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

Drivers:

Please give me several hours for thinking the new approach. If the new approach doesn't change the XUL spec, we should fix this bug on 3.0.1. However, the new approach also changes the XUL spec, we should fix this bug on RC2.

Comment 49

[Mike Schroepfer]

Comment on attachment 321102 [details] [diff] [review]
Patch v2.2.2

just removing the approval since we backed this out and need a different patch

Comment 50

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

Attachment: Patch v3.0

So, we should take the first approach. That doesn't change the most panels z-index. But adding 'notopmost' attr to panel element. This patch only changes the behavior of bookmark edit panel. I think this is lowest risk approach.

Comment 51

[Neil Deakin]

Comment on attachment 322300 [details] [diff] [review]
Patch v3.0

Looks OK, although I don't like this approach. It means that someone has to do something extra just to get ime working properly. Which most developers won't do.

Does this bug actually occur the same way on all platforms? I'm a bit surprised by that. I would expect at least one  platform to know to draw the ime popup on top of other popups.

Comment 52

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

(In reply to comment #51)
> (From update of attachment 322300 [details] [diff] [review])
> Looks OK, although I don't like this approach. It means that someone has to do
> something extra just to get ime working properly. Which most developers won't
> do.

Yes. But I don't have better idea now...

> Does this bug actually occur the same way on all platforms? I'm a bit surprised
> by that. I would expect at least one  platform to know to draw the ime popup on
> top of other popups.

On Windows and Mac, I confirmed this bug. On Linux, the candidate window is positioned wrong position, so, we cannot test this now. (I'll work on the bug, but it's not for Gecko1.9.0.)

Comment 53

[Mike Beltzner [:beltzner, not reading bugmail]]

This patch is untested, slightly contested by the reviewer, and I'm not sure we want to muck about here while under the gun from time pressure. We didn't get a lot of reports or dupes, and the primary functionality isn't broken, and the workaround exists (go into the Library and rename things there).

Sucks, but we're going to defer this for now.

Comment 54

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

(In reply to comment #53)
> This patch is untested,

no. I tested on Win/Mac and Linux. On Win/Mac, that works fine. And on Linux, the regression of the previous checking-in is gone.

Comment 55

[Mike Beltzner [:beltzner, not reading bugmail]]

The feeling at today's Firefox call, again, was that as per comment 51 and comment 53, we're not going to take this patch for RC2 due to the potential risk. We'll relnote and get this in a branch stability release.

Comment 56

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The relnote for this at http://www.mozilla.com/en-US/firefox/3.0rc2/releasenotes/ :

The IME input tool used to enter Japanese, Korean, Chinese and Indic characters covers the "Add Bookmark" panel. Users can use IME for input in the Library window by selecting "Organize Bookmarks..." from the Bookmarks menu (bug 433340)

It should read that the IME input tool "gets covered by" the Add Bookmark panel, instead of "covers". (See screenshots above)

Comment 57

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Relnote has been corrected as of 05 June 2008 to reflect the current situation. Thanks Beltzner!

Comment 58

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

Should I land the patch to trunk first?

Comment 59

[wellofsouls]

so will this bug be fixed in the RC3? (or at least before the final?)

this bug is really annoying as I'm using Chinese IME.

Comment 60

[Jesse Ruderman]

No, this isn't fixed in RC3.  It's unlikely to be fixed before Firefox 3.0.0 because any patch would be risky.

Comment 61

[Jesse Ruderman]

Masayuki, you should land this patch on trunk soon, assuming Neil Deakin is ok with that.  The more time trunk nightly users have to bang on it, the more likely it is we can get the patch into Firefox 3.0.1.

Comment 62

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

(In reply to comment #61)
> Masayuki, you should land this patch on trunk soon, assuming Neil Deakin is ok
> with that.  The more time trunk nightly users have to bang on it, the more
> likely it is we can get the patch into Firefox 3.0.1.

ok, Deakin already r+ed, so, I'll land that to trunk tomorrow. Thank you.

Comment 63

[Neil Deakin]

Actually, no, I still don't like the approach. It means that a developer can make an application/extension and then has to set a flag to say 'oh, by the way, make it work in East Asian locales as well', rather than what it should do, which is to just work to begin with.

Since we aren't as time constrained to make 3.0 any more, we should try to find a more optimal solution.

Comment 64

[Jesse Ruderman]

Would the "more optimal solution" be able to land for 3.0.1 as well?

Comment 65

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

Deakin:

I don't like such point too. If we don't use the current approach, we should back to the previous approach. And there are two ways.

1. Fix the gtk2 focus issue; the owner window of the gtk2 popup window lost the focus at popup window that is not a topmost. If we can fix this issue, this way may be best approach. But this approach may be difficult for me...

2. Add topmost="true" to all popup elements excepting the edit bookmark dialog. This way is risky. Because some extensions may be broken on gtk2.

Somebody have other ideas?

Comment 66

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

Oh, if we can fix this bug on widget level, it should be most safe way.

I'll try this way, first.

Comment 67

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

Note for myself:
http://developer.apple.com/technotes/tn2005/tn2128.html#TNTAG1

Comment 68

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

Comment on attachment 322300 [details] [diff] [review]
Patch v3.0

pending to request the approval. I'll try to new approach.

Comment 69

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

Attachment: widget patch v1.0 (win32 only)

This patch fixes the issue that the users cannot use IME on topmost window. However, ATOK (Japanese famous IME of a third party) doesn't work fine with this patch. (MS-IME and Chinese IME works fine for me.)

I think that this approach is "correct". However, if we cannot fix this problem completely with this approach, we also need the previous approach. I'll try to contact to the developer of ATOK.

Comment 70

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

Attachment: widget patch v2.0 (win32 + MacOS)

This is simplest patch for both Win32 and MacOSX. (Linux should be separated to another bug, the problem is not same this bug.)

Win32 part is not changed from the previous patch.

MacOSX part works fine for me with Kotoeri on 10.5. However, with ATOK 2006 for Mac, this patch doesn't work fine. But after testing with Kotoeri, ATOK works fine too. I think that ATOK may ignore the property value :-(

Comment 71

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

Attachment: widget patch v2.0 (win32 + MacOS)

oops, sorry. I attached wrong file.

Comment 72

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

Attachment: widget patch v2.1 (win32 + MacOS)

This patch works fine with ATOK 2006 on 10.5. I added a hack that is to notify the changing the value of tag by re-activate the TSMDocument.

I need to test with another IMEs and on 10.4.

Comment 73

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

test builds:

https://build.mozilla.org/tryserver-builds/2008-06-22_17:[e-mail]-326222/

Comment 74

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

feedback from a JP tester, following result is already known:

MS-IME2007 on Vista OK
MS-IME2003 on Vista OK

ATOK2007 on Vista NG
ATOK17 on Vista NG

Kotoeri on 10.4 OK
Kotoeri on 10.5 OK
ATOK2007 on 10.5 OK

Comment 75

[Atsushi Sakai]

MS-IME 2003 on XP SP2 NG:
Candidate window is covered by the edit bookmark panel. 

MS-IME 2002 on XP SP2 NG:
The edit bookmark panel closes(!) when candidate window is opening.

Comment 76

[Atsushi Sakai]

MS-IME 2000 on Win2000 SP4 NG:
The edit bookmark panel closes(!) when candidate window is opening.

Comment 77

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

Oh, it's strange... Thank you, Sakai-san.

Comment 78

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

Kimura-san, do you know why our topmost window is over the candidate window of IMEs?

Comment 79

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

Oh, the IME events in popup window is handled by parent window. This may be the cause of this bug on Windows...

Comment 80

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

mmmm.... the behavior of the window on Win32 is strange.

Even if the actual focus (the result of ::GetFocus()) is in the bookmark panel, the native focused window is same as the window that is focused on the parent chrome.

I.e., the native focus is set to same window in the following cases.
1. At the editors of bookmark panel has the focus.
2. At the search bar of the parent browser window.

I think that the widget on the panel has to have the native focus for IMEs.

Comment 81

[Masatoshi Kimura [:emk]]

IMO, attachment #321102 [details] [diff] [review] used the best approach. We should avoid topmost panel whenever possible.
Unfortunately, I don't know much about gtk2 focus isuue, too...

Comment 82

[Mike Beltzner [:beltzner, not reading bugmail]]

So the current status here is that we have a test build but it's not fully working?

This is wanted, but not sure that it's blocking 1.9.0.1. Blocks major-update, though.

Comment 83

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

(In reply to comment #82)
> So the current status here is that we have a test build but it's not fully
> working?

Yes.
# I'm still thinking what way is best for trunk and stable branch.

> This is wanted, but not sure that it's blocking 1.9.0.1. Blocks major-update,
> though.

I'll do my best. But I'm not sure I can do for 1.9.0.1. b1.9.0.1- and w1.9.0.1+ is best flags for now. But if I can fix this before 1.9.0.1, we should take this.

Comment 84

[dynamis (Tomoya ASAI)]

One more problem:
Bookmark dialog will cover dashboard widget too on Mac, not only IME candidate window.

I think this is the same bug and I wrote here but if not, please file another bug.

Comment 85

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

dynamis:

That is out of scope of this bug. Please file a new bug (or it may be filed already).

If we land the first patch, the bug is also gone. However, the patch changes the XUL spec, and the real issue -- IME cannot be used on top most window -- is not fixed. Therefore, I'm trying to fix the real issue. On Mac and Linux, I succeeded to fix the bugs on bookmark dialog in my local. Therefore, I don't want to change the XUL spec, now.

Comment 86

[dynamis (Tomoya ASAI)]

masayuki:

OK. Sorry for my confusion about the scope of this bug.
I found the bug about dashboard and added comment on bug 404131.
Thanks.

Comment 87

[Samuel Sidler (old account; do not CC)]

Masayuki-san, any update on your work for an improved patch?

Comment 88

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

Samuel:

I'll post a new patch next week.

Comment 89

[Samuel Sidler (old account; do not CC)]

Would really like to get this in 1.9.0.2...

Comment 90

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

(In reply to comment #89)
> Would really like to get this in 1.9.0.2...

I don't think this bug should block 1.9.0.2. Because the new approach will change lot lines. And unfortunately, I don't have much time for 1.9.0.2.

Comment 91

[Samuel Sidler (old account; do not CC)]

Sadly, this bug blocks major update, so if we don't get it for 3.0.2, we can't do major update until the next version...

Comment 92

[Mike Beltzner [:beltzner, not reading bugmail]]

Sam, I'm not sure I'd make that assertion. Have we heard of a lot of people with IME finding this to be a significant problem?

Comment 93

[Samuel Sidler (old account; do not CC)]

(In reply to comment #92)
> Sam, I'm not sure I'd make that assertion. Have we heard of a lot of people
> with IME finding this to be a significant problem?

I was basing that assertion on the MU+ in the whiteboard that you added a month ago. ;)

Comment 94

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

Attachment: widget patch v3.0 (win32 + MacOS)

OK:
MS-IME (the default version of Vista) on Vista
zh-CN IME on Vista
zh-TW IME on Vista
ATOK 2006 on 10.5
egbridge2 on 10.5
Kotoeri on 10.5
ITABC on 10.5
Pinyin on 10.5

NG:
ATOK2008 on Vista

Unfortunately, there are no rules that deciding the window of IMEs level on Windows, maybe...

Comment 95

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

testbuilds:

https://build.mozilla.org/tryserver-builds/2008-07-23_00:[e-mail]-330898/

Comment 96

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

OK:
ATOK 2006 on 10.4
egbridge on 10.4
Kotoeri on 10.4
ITABC on 10.4
Pinyin on 10.4

I think the mac part is safe, and works fine. If nobody can find problems, I'll separate the mac part to another bug and landing it ASAP.

Comment 97

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

Ugh... win32 build is failed on try server :-(

Comment 98

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

I filed bug 447635 for mac. And the bug already has the patch that is separated from attachment 330898 [details] [diff] [review]. We should fix this bug on Mac, first.

Comment 99

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

http://developer.mozilla.org/en/docs/XUL:panel

It seems that <panel> elements don't need to be top-most window. The spec said that "When open, it floats above the window and may extend outside the border of the main window.". So, <panel> should be only over the main window rather than the all other windows.

Therefore, we may be able to change the window level of the widget for <panel> to normal but that is owned by the main window.

Comment 100

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

Attachment: Patch of adhoc approach v1.0

I found a *nice* bug of <panel> element.

The window level of <panel> element is set *only* when it is created. So, we can create non-top-most panel but auto-hide <panel> by following steps.

1. <panel> element defined with |noautohide="true"|.
2. onpopupshowing, the noautohide attribute is set to "false" from script.

By this way, we can fix this bug *without* any XUL spec changes. So, we can fix this bug on Fx3.0 with very low risk.

Of course, we should *not* use this way on trunk, however, I think that we should land this patch to trunk first for testing by nightly build testers.

After this bug, I'll change the <panel> element spec that support "topmost" attribute *only* on trunk and backing out this patch from trunk.

Deakin:

I hope that you agree this approach.
# Of course, after you +'ing this patch, I'll request to the additional review to browser reviewer.

Comment 101

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

testers:

I posted the patch to try server. Please test them.
# tag is attachment331908 [details] [diff] [review]
# https://build.mozilla.org/tryserver-builds/?C=M;O=D

Comment 102

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

https://build.mozilla.org/tryserver-builds/2008-08-01_00:[e-mail]-attachment331908 [review]/

Comment 103

[Mike Beltzner [:beltzner, not reading bugmail]]

I discussed this bug at the Summit with Masayuki and others, and we decided that it would not be sensible to rush patches here. Further we decided that this was not required for major update (MU) and so I'm marking this wanted-but-not-blocking 1.9.0.2, and MU-.

Comment 104

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

I filed bug 448929 for the nice bug.

Comment 105

[Neil Deakin]

Comment on attachment 331908 [details] [diff] [review]
Patch of adhoc approach v1.0

Seems OK for now, but use removeAttribute instead of setting noautohide to false

Comment 106

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

Attachment: Patch of adhoc approach v1.1

Thank you, Deakin.

Dietrich:

Would you review this change? This patch changes to that the bookmark dialog will be non-topmost window but auto-hide mode. See comment 100 for the detail.

Comment 107

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

pushed to trunk.

Comment 108

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

Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Let's fix this on 1.9.0.x too.

Comment 109

[Samuel Sidler (old account; do not CC)]

Since the patch ended up in mozilla/browser, can we get an automated test for this before landing it in 1.9.0.x?

Comment 110

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

(In reply to comment #109)
> Since the patch ended up in mozilla/browser, can we get an automated test for
> this before landing it in 1.9.0.x?

We cannot get the actual window level. So, we need new API in nsIWidget and also we need new path for access to the API from JS code.

Comment 111

[Samuel Sidler (old account; do not CC)]

Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Approved for 1.9.0.2. Please land in CVS. a=ss

Masayuki, are there bugs on file for the new APIs required to test this?

Comment 112

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

checked-in, and I filed bug 450999 for the new APIs.

Comment 113

[Neil Deakin]

Don't you still want to make a better fix for panels in general, as suggested in comment 100?

Comment 114

[Neil Deakin]

Ah, looks like that is bug 451015.

Comment 115

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

Yes.

Comment 116

[Alfred Kayser]

This has probably caused bug 452385.

Comment 117

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

Backed out on CVS HEAD to fix bug 452385.

Comment 118

[Samuel Sidler (old account; do not CC)]

Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Removing approval since this was backed out.

Comment 119

[Alice0775 White]

Extension(https://addons.mozilla.org/en-US/firefox/search?q=taboo) popup also covers candidate window when using IME.

Comment 120

[Atsushi Sakai]

(In reply to comment #119)
> Extension(https://addons.mozilla.org/en-US/firefox/search?q=taboo) popup also
> covers candidate window when using IME.

It's Bug 451015.

Comment 121

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

Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

let's land this with the patch of bug 452385.

Comment 122

[Daniel Veditz [:dveditz]]

Not blocking in that we'll rip it back out if there's problems, but we'll look at approving the patch.

Comment 123

[Daniel Veditz [:dveditz]]

Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 124

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

checked in to 1.9.0 branch.

Comment 125

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

backed-out the patch from 1.9.0. Because dropdown list in the bookmark dialog is positioned to wrong point on some window manager on Linux. See following screenshot:

http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3905

I'll post new patch soon.

Comment 126

[Samuel Sidler (old account; do not CC)]

Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Minussing this patch then.

Comment 127

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

Attachment: Patch for 1.9.0 branch v2.0

We should apply the new code only for non-Linux platforms. On trunk, we don't change the behavior on Linux, so, this patch should make same behavior as trunk.

Comment 128

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

Attachment: Patch for 1.9.0 branch v2.0

sorry for the spam, I uploaded a wrong file.

Comment 129

[Daniel Veditz [:dveditz]]

Comment on attachment 342261 [details] [diff] [review]
Patch for 1.9.0 branch v2.0

Temporarily approved for 1.9.0.4, a=dveditz for release-drivers -- approval will expire next week because this patch makes us nervous given past regressions. Please land this weekend or early Monday in Asian timezones.

Comment 130

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

checked-in.

Comment 131

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot on Mac 10.5.5 Firefox 3.0.4 build 1

Verified on Mac 10.5.5 Firefox 3.0.4 build 1.

Comment 132

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv