New issue
Advanced search Search tips

Issue 876638 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

BaseDialog doesn't correctly handle being hidden immediately after being shown

Project Member Reported by sa...@chromium.org, Aug 22

Issue description

BaseDialog.show_() uses setTimeout to defer some work. If hide() is invoked after show_() is, but before the deferred function is, focus will end up on |initialFocusElement_| instead of |previousActiveElement_|.

It only seems possible in automatic tests. However, it has caused some test flakes for the files app so it would be nice to fix.
 
Owner: joelhockey@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/91d69eb18ca7c3f2a3c0a3523ebdbb73b2462ecc

commit 91d69eb18ca7c3f2a3c0a3523ebdbb73b2462ecc
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Sep 12 11:06:58 2018

Fix dialog show/hide races

When the dialog is shown and hidden twice in quick succession,
it is possible that the dialog is not shown the second time.

Both show() and hide() use setTimeout.  In particular, hide()
removes the child container element from its parent inside
setTimeout.

When calling show1 / hide1/ show2, it is possible that the delayed
hide1 removal of container happens after show2 is called.  In this case,
the dialog will not be shown.

Removed comments introduced in https://codereview.chromium.org/7764011
which are no longer relevant.

Bug:  876638 
Change-Id: Iec3c8e156154a481842fb0b1026c77f873f7eacc
Reviewed-on: https://chromium-review.googlesource.com/1220875
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590643}
[modify] https://crrev.com/91d69eb18ca7c3f2a3c0a3523ebdbb73b2462ecc/ui/webui/resources/js/cr/ui/dialogs.js

Status: Fixed (was: Available)

Sign in to add a comment