1331798 - "Page Bookmarked" menu is unexpectedly closed when typing text with IME

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Unable input Japanese text for bookmark name in "Page Bookmarked" arrow panel.

+++ This bug was initially created as a clone of Bug #1290011 +++

This is critical issue for use using IME.

Reproducible : 100%

Steps To reproduce:
1. IME on the Firefox textbox(serchbar or input field of any web page)
2. Click Star Button
3. Move mouse pointer downward and then rightward through empty area of the popup.
4. Type koredehatotemotukaetamonojanai (now the typed text is under conversion mode)
   ---- Bug: The popup is suddenly/unexpectedly closed

Actual Results:
The popup is suddenly/unexpectedly closed.
The following typed texts appear in previous focused input field. ---- This means, webpage can steal typed text.

Expected Results:
The popup should not close until press [ESC] key or click outside of the popup



I think, Bug 1219794 should be backed out.

Comment 1

[Alice0775 White]

Step3 is not necessary.

Comment 2

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

(In reply to Alice0775 White from comment #0)
> [Tracking Requested - why for this release]: Unable input Japanese text for
> bookmark name in "Page Bookmarked" arrow panel.
> 
> +++ This bug was initially created as a clone of Bug #1290011 +++
> 
> This is critical issue for use using IME.
> 
> Reproducible : 100%
> 
> Steps To reproduce:
> 1. IME on the Firefox textbox(serchbar or input field of any web page)
> 2. Click Star Button

This is not enough. Click star button when the page is not bookmarked.

> 3. Move mouse pointer downward and then rightward through empty area of the
> popup.
> 4. Type koredehatotemotukaetamonojanai (now the typed text is under
> conversion mode)


So, the problem is, the panel doesn't cancel the auto-close at compositionstart event.

Comment 3

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

Oh, I think that it's not enough with compositionstart event. For example, some text might be pasted with menu. So, we need to listen input event too.

Taking this.

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68687ea194b0

Comment 5

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

Attachment: Bug 1331798 Bookmark panel for new bookmark shouldn't be closed automatically during composition and after text input without keyboard events nor composition events

Bookmark panel for a new bookmark shouldn't be closed automatically during composition even if mouse cursor is moved outside the panel because user may click in IME's UI.

Additionally, editor might be modified without keyboard events nor composition events.  In such case, the bookmark panel shouldn't be closed automatically too.

Review commit: https://reviewboard.mozilla.org/r/105456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105456/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8827854 [details]
Bug 1331798 Bookmark panel for new bookmark shouldn't be closed automatically during composition and after text input without keyboard events nor composition events

https://reviewboard.mozilla.org/r/105456/#review106328

Thank you for the patch! While reviewing this I think I found a part of the fix for bug 1327938.

::: browser/base/content/test/general/browser_bookmark_popup.js:213
(Diff revision 1)
> +      yield new Promise(resolve => setTimeout(resolve, 400));
> +      is(bookmarkPanel.state, "open", "Panel should still be open on compositionstart");
> +    },
> +    shouldAutoClose: false,

Since you have 'shouldAutoClose: false' you shouldn't need to have this 'yield new Promise ...' code above since having shouldAutClose=false will cause test_bookmarks_popup to yield for 400ms.

::: browser/base/content/test/general/browser_bookmark_popup.js:275
(Diff revision 1)
> +      yield new Promise(resolve => setTimeout(resolve, 400));
> +      is(bookmarkPanel.state, "open", "Panel should still be open on mousemove");

Same here, this shouldn't be necessary since you have shouldAutoClose:false.

Comment 7

[Marcia Knous [:marcia]]

Tracking 52/53+ for this IME issue which prevents text input in the bookmark panel.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a250bc2a641e

Comment 9

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

Comment on attachment 8827854 [details]
Bug 1331798 Bookmark panel for new bookmark shouldn't be closed automatically during composition and after text input without keyboard events nor composition events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105456/diff/1-2/

Comment 10

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

Comment on attachment 8827854 [details]
Bug 1331798 Bookmark panel for new bookmark shouldn't be closed automatically during composition and after text input without keyboard events nor composition events

https://reviewboard.mozilla.org/r/105456/#review106328

> Since you have 'shouldAutoClose: false' you shouldn't need to have this 'yield new Promise ...' code above since having shouldAutClose=false will cause test_bookmarks_popup to yield for 400ms.

Thanks, removed.

> Same here, this shouldn't be necessary since you have shouldAutoClose:false.

Removed too.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b69accee20b4

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This missed the boat for 51, but please request Aurora approval on this when you're comfortable doing so.

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=106d5166ba45

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Will be beta 52 now.

Comment 15

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

Attachment: Patch for 52

Approval Request Comment
[Feature/Bug causing the regression]: regression of bug 1219794
[User impact if declined]: Using IME for inputting bookmark title or tags and use mouse to operate something (especially when operating IME's UI), auto close timer closes the bookmark panel if adding bookmark.
[Is this code covered by automated tests?]: Including new automated tests and a lot of behavior is also tested by the same automated test file.
[Has the fix been verified in Nightly?]: Yes, landed and now in Aurora too.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just preventing to close the panel automatically during "compositionstart" and "compositionend". And XUL <panel> can close automatically when user tries to move focus outside of it, so, no risk of becoming impossible to close the panel.
[String changes made/needed]: No.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8830660 [details] [diff] [review]
Patch for 52

fix regression in bookmark panel when using IME, beta52+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7fbb9a873771