Closed Bug 433340 Opened 16 years ago Closed 16 years ago

bookmark dialog covers candidate window when using IME

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3.1a2

People

(Reporter: masa141421356, Assigned: masayuki)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [MU-])

Attachments

(7 files, 17 obsolete files)

screenshot, input method example
59.87 KB, image/png
Details
screenshot, error of input chinese character in ko IME
72.23 KB, image/jpeg
Details
Screenshot, Chinses IME of Sogou Pinyin
28.00 KB, image/jpeg
Details
Screenshot, Chinese IME, Guge(Google.cn) Pinyin
60.00 KB, image/jpeg
Details
screenshot
33.51 KB, image/png
Details
Patch for 1.9.0 branch v2.0
1.85 KB, patch
Gavin
: 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
Details 
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
Version: unspecified → Trunk
I can reproduce this bug on Mac too. This is too bad accessibility bug for Japanese users. The candidate window is too important.
Status: UNCONFIRMED → NEW
Component: Bookmarks → Places
Ever confirmed: true
Flags: blocking-firefox3?
Keywords: jp-critical
OS: Windows XP → All
QA Contact: bookmarks → places
Hardware: PC → All
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
If we add a new attribute for panel element, we can fix this bug with very low risk.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #320559 - Flags: superreview?(neil)
Attachment #320559 - Flags: review?(neil)
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.
Attached patch Patch v1.0.1 (obsolete) — — Splinter Review 
adding comments.
Attachment #320559 - Attachment is obsolete: true
Attachment #320564 - Flags: superreview?(neil)
Attachment #320564 - Flags: review?(neil)
Attachment #320559 - Flags: superreview?(neil)
Attachment #320559 - Flags: review?(neil)
Attachment #320564 - Flags: review?(neil) → review?(enndeakin)
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?
Flags: wanted1.9.0.x+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Whiteboard: [RC2?]
Why are you only fixing this for the 'bookmark dialog'? Wouldn't the same bug apply to all popups?
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.
Flags: blocking-firefox3- → blocking-firefox3?
Severity: normal → critical
(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.


(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.

(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.

Is this a recent regression? Why did we only discover it now?
(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.
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?
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. 
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? 

(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 :)

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.
(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.
(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.
Attached patch Patch v2.0 (obsolete) — — Splinter Review 
This is ideal patch, I think.

I have an worry. I'm not sure this is needed: |!mInContentShell|. Why this is needed in IsNoAutoHide()?
Attachment #320564 - Attachment is obsolete: true
Attachment #320859 - Flags: superreview?(neil)
Attachment #320859 - Flags: review?(enndeakin)
Attachment #320564 - Flags: superreview?(neil)
Attachment #320564 - Flags: review?(enndeakin)
Comment on attachment 320859 [details] [diff] [review]
Patch v2.0

oops, this may break many features.
Attachment #320859 - Flags: superreview?(neil)
Attachment #320859 - Flags: review?(enndeakin)
Attachment #320859 - Flags: review-
Attached patch Patch v2.1 (obsolete) — — Splinter Review 
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()">
Attachment #320859 - Attachment is obsolete: true
Attachment #320864 - Flags: review?(enndeakin)
> 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.

(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.
Attached patch Patch v2.1.1 (obsolete) — — Splinter Review 
Remove unnecessary ')' from Patch v2.1.
This works for me.
Attached patch Patch v2.2 (obsolete) — — Splinter Review 
Thank you for your advice!
Attachment #320864 - Attachment is obsolete: true
Attachment #320912 - Attachment is obsolete: true
Attachment #320922 - Flags: review?(enndeakin)
Attachment #320864 - Flags: review?(enndeakin)
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.
Attachment #320922 - Flags: review?(enndeakin) → review+
Attached patch Patch v2.2.1 (obsolete) — — Splinter Review 
Thank you!
Attachment #320922 - Attachment is obsolete: true
Attachment #320928 - Flags: superreview?(neil)
Attachment #320928 - Flags: review+
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.)
Attachment #320928 - Flags: superreview?(neil) → superreview+
Attached patch Patch v2.2.2 (obsolete) — — Splinter Review 
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.
Attachment #320928 - Attachment is obsolete: true
Attachment #321102 - Flags: approval1.9?
Attached image 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.
(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.
Keywords: intl
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.
(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
This is breaking a key feature for most IME users.  We'll have to do an RC2, this will not stop RC1 ship.
Flags: blocking-firefox3? → blocking-firefox3+
Whiteboard: [RC2?] → [RC2]
Whiteboard: [RC2] → [RC2?]
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).
Flags: blocking1.9.0.1+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
(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.)
(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?
It makes <panel> elements not be topmost windows, and gives them a parent/owner window at the OS level.

(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.
Whiteboard: [RC2?] → [RC2?][RC2+]
Comment on attachment 321102 [details] [diff] [review]
Patch v2.2.2

a+ schrep for 3.0.1 or RC2 please land on cvs trunk.
Attachment #321102 - Flags: approval1.9? → approval1.9+
(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.
(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.

Ugh... thinderbox is  all green now. Maybe, widget code of linux has a bug at focus handling....
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.
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 on attachment 321102 [details] [diff] [review]
Patch v2.2.2

just removing the approval since we backed this out and need a different patch
Attachment #321102 - Flags: approval1.9+
Attached patch Patch v3.0 (obsolete) — — Splinter Review 
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.
Attachment #321102 - Attachment is obsolete: true
Attachment #322300 - Flags: review?(enndeakin)
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.
Attachment #322300 - Flags: review?(enndeakin) → review+
(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.)
Attachment #322300 - Flags: superreview?(neil)
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.
Keywords: relnote
Whiteboard: [RC2?][RC2+] → [RC2?]
Depends on: 435603
(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.
Attachment #322300 - Flags: superreview?(neil) → superreview+
Attachment #322300 - Flags: approval1.9?
Flags: in-testsuite?
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.
Whiteboard: [RC2?]
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)
Relnote has been corrected as of 05 June 2008 to reflect the current situation. Thanks Beltzner!
Should I land the patch to trunk first?
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.
No, this isn't fixed in RC3.  It's unlikely to be fixed before Firefox 3.0.0 because any patch would be risky.
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.
(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.
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.

Would the "more optimal solution" be able to land for 3.0.1 as well?
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?
Oh, if we can fix this bug on widget level, it should be most safe way.

I'll try this way, first.
Comment on attachment 322300 [details] [diff] [review]
Patch v3.0

pending to request the approval. I'll try to new approach.
Attachment #322300 - Flags: approval1.9?
Attached patch widget patch v1.0 (win32 only) (obsolete) — — Splinter Review 
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.
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 :-(
Attachment #325085 - Attachment is obsolete: true
oops, sorry. I attached wrong file.
Attachment #326217 - Attachment is obsolete: true
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.
Attachment #326218 - Attachment is obsolete: true
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
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.
MS-IME 2000 on Win2000 SP4 NG:
The edit bookmark panel closes(!) when candidate window is opening.
Oh, it's strange... Thank you, Sakai-san.
Kimura-san, do you know why our topmost window is over the candidate window of IMEs?
Oh, the IME events in popup window is handled by parent window. This may be the cause of this bug on Windows...
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.
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...
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.
Flags: blocking1.9.0.1-
Flags: blocking1.9.0.1+
Flags: blocking-firefox3.1+
Whiteboard: [MU+]
(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.
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.
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.
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.
Masayuki-san, any update on your work for an improved patch?
Samuel:

I'll post a new patch next week.
Would really like to get this in 1.9.0.2...
Flags: blocking1.9.0.2?
(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.
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...
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?
(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. ;)
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...
Attachment #326222 - Attachment is obsolete: true
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.
Ugh... win32 build is failed on try server :-(
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.
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.
Attachment #330898 - Attachment is obsolete: true
Attached patch Patch of adhoc approach v1.0 (obsolete) — — Splinter Review 
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.
Attachment #322300 - Attachment is obsolete: true
Attachment #331908 - Flags: review?(enndeakin)
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
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-.
Flags: blocking1.9.0.2? → blocking1.9.0.2-
Whiteboard: [MU+] → [MU-]
I filed bug 448929 for the nice bug.
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
Attachment #331908 - Flags: review?(enndeakin) → review+
Attached patch Patch of adhoc approach v1.1 (obsolete) — — Splinter Review 
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.
Attachment #331908 - Attachment is obsolete: true
Attachment #332223 - Flags: review?(dietrich)
Attachment #332223 - Flags: review?(dietrich) → review+
pushed to trunk.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1a2
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Let's fix this on 1.9.0.x too.
Attachment #332223 - Flags: approval1.9.0.2?
Since the patch ended up in mozilla/browser, can we get an automated test for this before landing it in 1.9.0.x?
(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 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?
Attachment #332223 - Flags: approval1.9.0.2? → approval1.9.0.2+
checked-in, and I filed bug 450999 for the new APIs.
Keywords: fixed1.9.0.2
Don't you still want to make a better fix for panels in general, as suggested in comment 100?
Ah, looks like that is bug 451015.
This has probably caused bug 452385.
Backed out on CVS HEAD to fix bug 452385.
Keywords: fixed1.9.0.2
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Removing approval since this was backed out.
Attachment #332223 - Flags: approval1.9.0.2+ → approval1.9.0.2-
Flags: blocking1.9.0.3?
Extension(https://addons.mozilla.org/en-US/firefox/search?q=taboo) popup also covers candidate window when using IME.
(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.
Attachment #332223 - Flags: approval1.9.0.3?
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

let's land this with the patch of bug 452385.
Not blocking in that we'll rip it back out if there's problems, but we'll look at approving the patch.
Flags: blocking1.9.0.4?
Attachment #332223 - Flags: approval1.9.0.4? → approval1.9.0.4+
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Approved for 1.9.0.4, a=dveditz for release-drivers
checked in to 1.9.0 branch.
Keywords: fixed1.9.0.4
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.
Keywords: fixed1.9.0.4
Comment on attachment 332223 [details] [diff] [review]
Patch of adhoc approach v1.1

Minussing this patch then.
Attachment #332223 - Flags: approval1.9.0.4+ → approval1.9.0.4-
Attached patch Patch for 1.9.0 branch v2.0 (obsolete) — — Splinter Review 
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.
Attachment #332223 - Attachment is obsolete: true
Attachment #342260 - Flags: review?(gavin.sharp)
sorry for the spam, I uploaded a wrong file.
Attachment #342260 - Attachment is obsolete: true
Attachment #342261 - Flags: review?(gavin.sharp)
Attachment #342260 - Flags: review?(gavin.sharp)
Attachment #342261 - Flags: review?(gavin.sharp) → review+
Attachment #342261 - Flags: approval1.9.0.4?
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.
Attachment #342261 - Flags: approval1.9.0.4? → approval1.9.0.4+
checked-in.
Keywords: fixed1.9.0.4
Verified on Mac 10.5.5 Firefox 3.0.4 build 1.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.1
Keywords: fixed1.9.1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: