Last Comment Bug 433340 - bookmark dialog covers candidate window when using IME
: bookmark dialog covers candidate window when using IME
Status: VERIFIED FIXED
[MU-]
: fixed1.9.0.4, inputmethod, intl, jp-critical, relnote, verified1.9.1
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- critical with 3 votes (vote)
: Firefox 3.1a2
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
:
:
Mentors:
Depends on: 448927 450999 435603 447635 447945 451015 452385
Blocks: 448929
  Show dependency treegraph
 
Reported: 2008-05-12 09:32 PDT by Masahiro YAMADA
Modified: 2010-06-18 19:00 PDT (History)
43 users (show)
mconnor: blocking‑firefox3-
mbeltzner: blocking‑firefox3.5+
mbeltzner: blocking1.9.0.1-
mbeltzner: blocking1.9.0.2-
mbeltzner: wanted1.9.0.x+
mbeltzner: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (5.15 KB, patch)
2008-05-12 10:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch v1.0.1 (5.76 KB, patch)
2008-05-12 11:10 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
screenshot, input method example (59.87 KB, image/png)
2008-05-13 18:29 PDT, John Daggett (:jtd)
no flags Details
screenshot, error of input chinese character in ko IME (72.23 KB, image/jpeg)
2008-05-13 19:06 PDT, Channy Yun [:channy]
no flags Details
Screenshot, Chinses IME of Sogou Pinyin (28.00 KB, image/jpeg)
2008-05-13 22:02 PDT, Jia Mi [:mijia]
no flags Details
Screenshot, Chinese IME, Guge(Google.cn) Pinyin (60.00 KB, image/jpeg)
2008-05-13 22:06 PDT, Jia Mi [:mijia]
no flags Details
Patch v2.0 (4.75 KB, patch)
2008-05-14 00:11 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
masayuki: review-
Details | Diff | Splinter Review
Patch v2.1 (4.95 KB, patch)
2008-05-14 00:45 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch v2.1.1 (4.95 KB, patch)
2008-05-14 08:22 PDT, Atsushi Sakai
no flags Details | Diff | Splinter Review
Patch v2.2 (5.07 KB, patch)
2008-05-14 09:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
enndeakin: review+
Details | Diff | Splinter Review
Patch v2.2.1 (4.95 KB, patch)
2008-05-14 09:48 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
masayuki: review+
neil: superreview+
Details | Diff | Splinter Review
Patch v2.2.2 (4.95 KB, patch)
2008-05-15 10:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
screenshot (33.51 KB, image/png)
2008-05-16 00:08 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Patch v3.0 (6.03 KB, patch)
2008-05-23 14:31 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
enndeakin: review+
neil: superreview+
Details | Diff | Splinter Review
widget patch v1.0 (win32 only) (2.81 KB, patch)
2008-06-14 03:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
widget patch v2.0 (win32 + MacOS) (1.14 KB, patch)
2008-06-22 16:31 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
widget patch v2.0 (win32 + MacOS) (6.86 KB, patch)
2008-06-22 16:33 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
widget patch v2.1 (win32 + MacOS) (7.02 KB, patch)
2008-06-22 17:02 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
widget patch v3.0 (win32 + MacOS) (36.74 KB, patch)
2008-07-23 00:31 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch of adhoc approach v1.0 (1.33 KB, patch)
2008-08-01 00:27 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
enndeakin: review+
Details | Diff | Splinter Review
Patch of adhoc approach v1.1 (1.33 KB, patch)
2008-08-04 10:37 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
gavin.sharp: review+
samuel.sidler+old: approval1.9.0.2-
samuel.sidler+old: approval1.9.0.4-
Details | Diff | Splinter Review
Patch for 1.9.0 branch v2.0 (1.33 KB, patch)
2008-10-08 09:36 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch for 1.9.0 branch v2.0 (1.85 KB, patch)
2008-10-08 09:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
gavin.sharp: review+
dveditz: approval1.9.0.4+
Details | Diff | Splinter Review
screenshot on Mac 10.5.5 Firefox 3.0.4 build 1 (41.87 KB, image/png)
2008-11-10 22:50 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details

Description Masahiro YAMADA 2008-05-12 09:32:12 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-12 09:36:30 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-12 10:56:43 PDT
Created attachment 320559 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-12 11:02:18 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-12 11:10:49 PDT
Created attachment 320564 [details] [diff] [review]
Patch v1.0.1

adding comments.
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2008-05-13 11:00:04 PDT
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 (away until Oct 3) 2008-05-13 12:09:40 PDT
Why are you only fixing this for the 'bookmark dialog'? Wouldn't the same bug apply to all popups?
Comment 7 John Daggett (:jtd) 2008-05-13 18:29:21 PDT
Created attachment 320841 [details]
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] 2008-05-13 18:42:13 PDT
(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] 2008-05-13 18:48:53 PDT
(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] 2008-05-13 18:51:52 PDT
(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: gavin@gavinsharp.com] 2008-05-13 18:54:50 PDT
Is this a recent regression? Why did we only discover it now?
Comment 12 Gen Kanai [:gen] 2008-05-13 19:01:49 PDT
(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] 2008-05-13 19:06:59 PDT
Created attachment 320843 [details]
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] 2008-05-13 20:37:48 PDT
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) 2008-05-13 21:09:25 PDT
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] 2008-05-13 21:31:59 PDT
(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] 2008-05-13 21:55:44 PDT
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] 2008-05-13 22:02:37 PDT
Created attachment 320852 [details]
Screenshot, Chinses IME of Sogou Pinyin
Comment 19 Jia Mi [:mijia] 2008-05-13 22:06:17 PDT
Created attachment 320853 [details]
Screenshot, Chinese IME, Guge(Google.cn) Pinyin
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-13 23:17:32 PDT
(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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-13 23:19:28 PDT
(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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-14 00:11:04 PDT
Created attachment 320859 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-14 00:12:31 PDT
Comment on attachment 320859 [details] [diff] [review]
Patch v2.0

oops, this may break many features.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-14 00:45:15 PDT
Created attachment 320864 [details] [diff] [review]
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 (away until Oct 3) 2008-05-14 05:47:28 PDT
> 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 2008-05-14 07:31:01 PDT
(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 2008-05-14 08:22:15 PDT
Created attachment 320912 [details] [diff] [review]
Patch v2.1.1

Remove unnecessary ')' from Patch v2.1.
This works for me.
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-14 09:08:31 PDT
Created attachment 320922 [details] [diff] [review]
Patch v2.2

Thank you for your advice!
Comment 29 Neil Deakin (away until Oct 3) 2008-05-14 09:20:02 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-14 09:48:20 PDT
Created attachment 320928 [details] [diff] [review]
Patch v2.2.1

Thank you!
Comment 31 neil@parkwaycc.co.uk 2008-05-15 05:05:02 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-15 10:08:25 PDT
Created attachment 321102 [details] [diff] [review]
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] 2008-05-16 00:08:48 PDT
Created attachment 321216 [details]
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] 2008-05-16 00:21:27 PDT
(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] 2008-05-16 11:33:44 PDT
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] 2008-05-16 11:36:20 PDT
(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] 2008-05-16 11:38:04 PDT
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] 2008-05-20 12:07:26 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-20 19:34:59 PDT
(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] 2008-05-22 13:54:02 PDT
(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 (away until Oct 3) 2008-05-22 13:55:57 PDT
It makes <panel> elements not be topmost windows, and gives them a parent/owner window at the OS level.
Comment 42 Dave Townsend [:mossop] 2008-05-22 13:59:33 PDT
(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 2008-05-22 18:15:24 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-22 21:48:01 PDT
(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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-23 04:42:09 PDT
(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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-23 06:18:09 PDT
Ugh... thinderbox is  all green now. Maybe, widget code of linux has a bug at focus handling....
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-23 07:09:02 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-23 07:13:16 PDT
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 2008-05-23 11:32:35 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-23 14:31:53 PDT
Created attachment 322300 [details] [diff] [review]
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 (away until Oct 3) 2008-05-26 04:56:38 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-26 07:34:07 PDT
(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] 2008-05-26 13:45:48 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-05-26 21:38:26 PDT
(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] 2008-05-27 16:24:38 PDT
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] 2008-06-04 22:42:56 PDT
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] 2008-06-05 16:44:12 PDT
Relnote has been corrected as of 05 June 2008 to reflect the current situation. Thanks Beltzner!
Comment 58 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-10 11:05:27 PDT
Should I land the patch to trunk first?
Comment 59 wellofsouls 2008-06-11 00:11:13 PDT
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 2008-06-11 12:25:19 PDT
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 2008-06-11 12:26:59 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-11 14:04:27 PDT
(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 (away until Oct 3) 2008-06-11 15:28:47 PDT
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 2008-06-11 16:04:52 PDT
Would the "more optimal solution" be able to land for 3.0.1 as well?
Comment 65 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-11 22:00:54 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-11 22:37:49 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-11 23:02:42 PDT
Note for myself:
http://developer.apple.com/technotes/tn2005/tn2128.html#TNTAG1
Comment 68 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-14 00:34:41 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-14 03:08:09 PDT
Created attachment 325085 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-22 16:31:45 PDT
Created attachment 326217 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-22 16:33:02 PDT
Created attachment 326218 [details] [diff] [review]
widget patch v2.0 (win32 + MacOS)

oops, sorry. I attached wrong file.
Comment 72 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-22 17:02:21 PDT
Created attachment 326222 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-22 17:51:33 PDT
test builds:

https://build.mozilla.org/tryserver-builds/2008-06-22_17:14-masayuki@d-toybox.com-326222/
Comment 74 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-22 23:49:01 PDT
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 2008-06-23 01:26:02 PDT
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 2008-06-23 03:38:21 PDT
MS-IME 2000 on Win2000 SP4 NG:
The edit bookmark panel closes(!) when candidate window is opening.
Comment 77 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-23 06:08:15 PDT
Oh, it's strange... Thank you, Sakai-san.
Comment 78 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-23 06:50:59 PDT
Kimura-san, do you know why our topmost window is over the candidate window of IMEs?
Comment 79 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-23 11:48:57 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-23 13:09:54 PDT
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] 2008-06-23 13:15:19 PDT
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] 2008-06-23 22:06:52 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-06-24 07:39:49 PDT
(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) 2008-07-15 17:42:56 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-15 21:21:46 PDT
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) 2008-07-16 01:00:36 PDT
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) 2008-07-17 15:56:45 PDT
Masayuki-san, any update on your work for an improved patch?
Comment 88 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-17 21:17:01 PDT
Samuel:

I'll post a new patch next week.
Comment 89 Samuel Sidler (old account; do not CC) 2008-07-21 18:08:43 PDT
Would really like to get this in 1.9.0.2...
Comment 90 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-21 21:28:42 PDT
(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) 2008-07-22 00:11:42 PDT
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] 2008-07-22 10:17:57 PDT
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) 2008-07-22 13:14:31 PDT
(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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-23 00:31:19 PDT
Created attachment 330898 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-23 01:17:47 PDT
testbuilds:

https://build.mozilla.org/tryserver-builds/2008-07-23_00:38-masayuki@d-toybox.com-330898/
Comment 96 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-23 01:38:01 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-23 01:47:04 PDT
Ugh... win32 build is failed on try server :-(
Comment 98 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-23 09:28:53 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-07-23 22:25:34 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-01 00:27:59 PDT
Created attachment 331908 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-01 00:34:08 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-01 08:03:28 PDT
https://build.mozilla.org/tryserver-builds/2008-08-01_00:32-masayuki@d-toybox.com-attachment331908 [review]/
Comment 103 Mike Beltzner [:beltzner, not reading bugmail] 2008-08-03 01:25:58 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-03 04:58:02 PDT
I filed bug 448929 for the nice bug.
Comment 105 Neil Deakin (away until Oct 3) 2008-08-04 06:50:13 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-04 10:37:35 PDT
Created attachment 332223 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-04 21:58:05 PDT
pushed to trunk.
Comment 108 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-08 08:13:30 PDT
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) 2008-08-11 15:30:57 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-14 10:27:28 PDT
(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) 2008-08-15 14:26:18 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-17 22:47:54 PDT
checked-in, and I filed bug 450999 for the new APIs.
Comment 113 Neil Deakin (away until Oct 3) 2008-08-18 11:37:01 PDT
Don't you still want to make a better fix for panels in general, as suggested in comment 100?
Comment 114 Neil Deakin (away until Oct 3) 2008-08-18 11:40:50 PDT
Ah, looks like that is bug 451015.
Comment 115 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-08-18 13:57:38 PDT
Yes.
Comment 116 Alfred Kayser 2008-08-28 00:09:46 PDT
This has probably caused bug 452385.
Comment 117 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-08-28 15:12:35 PDT
Backed out on CVS HEAD to fix bug 452385.
Comment 118 Samuel Sidler (old account; do not CC) 2008-08-28 15:15:37 PDT
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Removing approval since this was backed out.
Comment 119 Alice0775 White 2008-09-06 06:55:23 PDT
Extension(https://addons.mozilla.org/en-US/firefox/search?q=taboo) popup also covers candidate window when using IME.
Comment 120 Atsushi Sakai 2008-09-06 07:32:59 PDT
(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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-09-24 09:47:55 PDT
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] 2008-10-03 11:56:03 PDT
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] 2008-10-03 11:56:21 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-10-06 09:21:34 PDT
checked in to 1.9.0 branch.
Comment 125 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-10-07 20:53:34 PDT
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) 2008-10-07 20:59:42 PDT
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Minussing this patch then.
Comment 127 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-10-08 09:36:45 PDT
Created attachment 342260 [details] [diff] [review]
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-10-08 09:39:02 PDT
Created attachment 342261 [details] [diff] [review]
Patch for 1.9.0 branch v2.0

sorry for the spam, I uploaded a wrong file.
Comment 129 Daniel Veditz [:dveditz] 2008-10-17 10:25:46 PDT
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] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-10-18 01:46:28 PDT
checked-in.
Comment 131 Gary Kwong [:gkw] [:nth10sd] 2008-11-10 22:50:30 PST
Created attachment 347472 [details]
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] 2009-11-26 05:56:50 PST
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

Note You need to log in before you can comment on or make changes to this bug.