Closed Bug 1965654 Opened 1 year ago Closed 1 year ago

Take2 perma comm/mail/components/addrbook/test/browser/browser_edit_photo.js | Uncaught exception in test bound test_local - waiting for photo dialog to change state - timed out after 50 tries.

Categories

(Thunderbird :: Address Book, defect)

Product:
Thunderbird
Component:
Address Book
Type:
defect

Tracking

(thunderbird_esr128 unaffected, thunderbird139 unaffected)

Status:
RESOLVED FIXED
Milestone:
140 Branch
Tracking Flags:
Tracking Status
thunderbird_esr128 --- unaffected
thunderbird139 --- unaffected

People

(Reporter: intermittent-bug-filer, Assigned: mkmelin)

References

(Regression)

Details

(Keywords: intermittent-failure, intermittent-testcase, regression, Whiteboard: [stockwell needswork:owner])

Attachments

(1 file, 1 obsolete file)

Bug 1965654 - Use keydown instead of keypress listener in contact edit form. r=#thunderbird-reviewers
48 bytes, text/x-phabricator-request
Details | Review
See Also: → 1964029

Haven't verified but would assume this is a regression from bug 1965052.

https://hg-edge.mozilla.org/mozilla-central/pushloghtml?changeset=72ee50ceade744f48d24107ff9e5acacfafba90b

Ah, its working with dom.dialog.light-dismiss.enabled false. So from bug 1964077
I assume this should be working the same way with that enabled as when disabled. (It's just a normal dialog, no closedby set)

Flags: needinfo?(mozilla)
Keywords: regression
Regressed by: 1964077

Set release status flags based on info from the regressing bug 1964077

status-firefox138: --- → unaffected
status-firefox139: --- → unaffected
status-firefox140: --- → affected
status-firefox-esr128: --- → unaffected
Flags: needinfo?(mozilla)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED

The other option is to fix the test in https://searchfox.org/comm-central/rev/0db84d7b2365b6eb0ff568ba1404d1bad03845bf/mail/components/addrbook/test/browser/browser_edit_photo.js#566-567, e.g.

let p = waitForDialogOpenState(false);
EventUtils.synthesizeKey("VK_ESCAPE", {}, abWindow);
await p;

Magnus, besides the test failure, is there any other issue if the dialog is closed synchronously on the keydown event?

Flags: needinfo?(mkmelin+mozilla)

It's ultimately not directly a test issue. What it's testing is this:

In Thunderbird address book edit a contact
Click the picture to open image editing dialog
ESC to close this dialog (and it closes)
Since we have ESC set to to close editing, the whole editing session also closes. Previously the ESC to close dialog was not "leaked" outside.

Flags: needinfo?(mkmelin+mozilla)

Ah, I misunderstood that. Thanks for the clarification!

I worry this might come up again if spec explicitly define the behavior, e.g. dialog should be closed synchronously after keydown event.
How easy would it be for Thunderbird’s address book to address this?

With dom.dialog.light-dismiss.enabled enabled, you can prevent the browser from closing the dialog by adding closedby="none" to the dialog.
With dom.dialog.light-dismiss.enabled disabled closedby="none" doesn't work, but we keep the original behavior that closes dialog asynchronously (bug 1964029), allowing Thunderbird’s address book to handle the event (keypress?) first.
So adding closedby="none" should work for both case now.

Or Thunderbird’s address book can preventDefault the keydown event for ESC which can also prevent us from handing the ESC.

WDYT?

Well,

  • we want ESC to close the dialog
  • we want ESC to close the editing (but not when the dialog is open)

I'm not sure that is possible the way things are atm. And I think it should, it doesn't sound like an unusual thing to do.

With dom.dialog.light-dismiss.enabled enabled, if I add closedby="none" ESC doesn't close the dialog. But I also do not get the keypress event in the listener outside the dialog.

If a listener can be added to the document that calls stopImmediatePropagation() when the dialog is open would that prevent the double closure?

The thunderbird code seems to rely on non-conforming behaviour, which worked prior (I'm not exactly sure why/how though).

The code relies on a keypress event where event.key == 'Escape'. According to https://w3c.github.io/uievents/#event-type-keypress keypress events should only be dispatched for keys which print a character, or as the spec puts it:

If supported by a user agent, this event MUST be dispatched when a key is pressed down, if and only if that key normally produces a character value.

I think the solution here is for Thunderbird to instead use the keydown event to listen for Esc - while also adding stopPropagation() when the Esc comes from the modal dialog. Native engine code does the same (listens for keydown): https://searchfox.org/mozilla-central/source/layout/base/PresShell.cpp#9087

Attachment #9486998 - Attachment is obsolete: true

Thanks Keith!

Assignee: mozilla → mkmelin+mozilla
status-firefox138: unaffected → ---
status-firefox139: unaffected → ---
status-firefox140: affected → ---
status-firefox-esr128: unaffected → ---
Component: DOM: Core & HTML → Address Book
Product: Core → Thunderbird

We were relying on non-conforming behaviour as keypress events should be dispatched only for keys that normally produces a character value - which ESC obviously does not.

Fixes perma comm/mail/components/addrbook/test/browser/browser_edit_photo.js | Uncaught exception in test bound test_local - waiting for photo dialog to change state - timed out after 50 tries.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/515784f7097e
Use keydown instead of keypress listener in contact edit form. r=babolivier

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 140 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: