Block background UI interaction for dialogs #1211
Conversation
|
Fixes #1195 |
There was a problem hiding this comment.
👍 thanks for working on this. besides the DRY code comments, I noticed some issues with the UI elements when trying to click outside the active widget's clickable regions.
while the Settings was opened, by clicking on the URL bar, I was able to get the Tray to disappear and unable to come back. and the UI became responsive until it resulted in a crash (see logcat), which I was not prompted for upon relaunching FxR.
- open the Settings.
- try to click on the URL bar.
- notice the Tray goes missing.
- none of the UI becomes responsive anymore.
| @@ -55,6 +55,7 @@ public CrashDialogWidget(Context aContext, AttributeSet aAttrs, int aDefStyle) { | |||
| private void initialize(Context aContext) { | |||
| inflate(aContext, R.layout.crash_dialog, this); | |||
|
|
|||
| mIsDialog = true; | |||
There was a problem hiding this comment.
instead, in UIWidget's parent initialize method can we set mIsDialog = true; (or setIsDialog(true);)?
| } | ||
|
|
||
| private boolean isWidgetInputEnabled(Widget aWidget) { | ||
| return mActiveDialog == null || (mActiveDialog == aWidget); |
There was a problem hiding this comment.
nit: can remove the parens: (, )
|
|
||
| if (aWidget.isVisible()) { | ||
| mActiveDialog = aWidget; | ||
| } else if (aWidget == mActiveDialog) { |
There was a problem hiding this comment.
could combine this to be a single if statement
There was a problem hiding this comment.
Good catch, I was missing a condition in the else branch
MortimerGoro
force-pushed
the
modal_dialog
branch
2 times, most recently
from
f236b24
to
b40bac0
Compare
| @@ -23,7 +23,7 @@ | |||
|
|
|||
| import java.net.URI; | |||
|
|
|||
| public class PermissionWidget extends UIWidget implements WidgetManagerDelegate.FocusChangeListener { | |||
| public class PermissionWidget extends UIDialog implements WidgetManagerDelegate.FocusChangeListener { | |||
There was a problem hiding this comment.
Is this a typo? Since UIDialog is derived from FocusChangeListener, does PermissionWidget need to be as well?
There was a problem hiding this comment.
👍 thanks for working on this. it looks like you did a significant amount of work to address the original issue: #1195.
I noticed a few regressions:
- keyboard input no longer works when attempting to edit values in the input fields in the Settings sub-menus (e.g., Display and Developer Options).
- pressing on the Tray no longer dismisses the Settings UI.
- pressing on the Navigation Bar no longer dismisses the Settings UI (but, per fixing issue Settings UI dims background but background UI can still be interacted with #1195, the presses are not propagated; nice work 👍).
- the hover state of the Settings button lingers, unlike it did before.
I've captured videos of after vs. before. see my annotations in the videos.
see this branch modal_dialog, modal_dialog rebased against release-1.2:
https://vimeo.com/user41954276/review/336277352/594b11e75c
vs.
master, release-1.2:
https://vimeo.com/user41954276/review/336277083/1f95da588d
I think given the issues mentioned above (sorted by severity IMO), we should wait before these items can be properly addressed before landing.
thanks again. much appreciated!
MortimerGoro
force-pushed
the
modal_dialog
branch
from
b40bac0
to
88b9589
Compare
|
Ready for review again. Now it makes it possible to click on the keyboard or click on any disabled widget to trigger the same dismiss action as clicking in the empty space of the world. |
There was a problem hiding this comment.
Tested against the regressions noted by @cvan and looks good to land in 1.2.
|
I'm merging this so we can generate another release candidate today, @cvan if you find something else create a follow-up issue |
No description provided.