Closed Bug 1331798 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Version:
47 Branch
Platform:
All
Windows 10
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox53 + fixed

People

(Reporter: alice0775, Assigned: masayuki)

References

Details

(Keywords: jp-critical, regression, ux-control)

Attachments

(2 files)

Bug 1331798 Bookmark panel for new bookmark shouldn't be closed automatically during composition and after text input without keyboard events nor composition events
59 bytes, text/x-review-board-request
jaws
: review+
Details
Patch for 52
7.95 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
[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.
Priority: P2 → --
Summary: "Page Bookmarked" menu is unexpectedly closed when typing test with IME → "Page Bookmarked" menu is unexpectedly closed when typing text with IME
Step3 is not necessary.
Flags: needinfo?(jaws)
(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.
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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.
Attachment #8827854 - Flags: review?(jaws) → review+
Tracking 52/53+ for this IME issue which prevents text input in the bookmark panel.
tracking-firefox52: ? → +
tracking-firefox53: ? → +
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.
https://hg.mozilla.org/mozilla-central/rev/b69accee20b4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
This missed the boat for 51, but please request Aurora approval on this when you're comfortable doing so.
status-firefox51: affected → wontfix
Flags: needinfo?(masayuki)
Will be beta 52 now.
Attached patch Patch for 52 — — Splinter Review 
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.
Flags: needinfo?(masayuki)
Attachment #8830660 - Flags: approval-mozilla-beta?
Comment on attachment 8830660 [details] [diff] [review]
Patch for 52

fix regression in bookmark panel when using IME, beta52+
Attachment #8830660 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.