New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 674649 link

Starred by 2 users
Status: Started
Owner:
kkhorimoto@chromium.org
Cc:
mard...@chromium.org
mrefaat@chromium.org
█ ghendel@chromium.org
eugene...@chromium.org
pkl@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Feature



Sign in to add a comment

Implement non-modal JavaScript dialogs

Project Member Reported by kkhorimoto@chromium.org, Dec 15 2016 
Under the current UIAlertController-based implementation of AlertCoordinator, JavaScript dialogs are displayed modally.  This means that when a dialog is displayed, no other actions in the app can occur until it is dismissed.  This is a sub-optimal solution because a user may want to switch Tabs or close the Tab presenting the dialog.  Safari on iOS displays JavaScript dialogs that are only modal for the content area of the responsible Tab, meaning that you can still interact with other controls in the UI.

To fix this we need to:
1) Contact UX for both iOS and Android to verify that this behavior is indeed desired for mobile Chrome.  The current implementation simply switches the current Tab to be the one presenting the dialog, then shows the dialog modally over the whole app.
2) Update MDC dialogs to be displayed non-modally, so we can present them as child view controllers for the content area of the Tab attempting to show the dialog.
3) Rearchitect our UIViewController hierarchy such that a view controller is used to display the web page's content area.  Currently, a single UIViewController is used to display the entire browser UI, including toolbars and tab switcher buttons.
4) Display non-modal MDC dialogs as child view controllers of the content area view controller.

Comment 1 by kkhorimoto@chromium.org, Dec 15 2016 

 Issue 673178  has been merged into this issue.

Comment 2 by eugene...@chromium.org, Mar 22 2017

Components: UI>Browser

Comment 3 by kkhorimoto@chromium.org, May 17 2017

Cc: kkhorimoto@chromium.org
Owner: mrefaat@chromium.org

Comment 4 by kkhorimoto@chromium.org, May 17 2017

Labels: -Type-Bug -Pri-3 Pri-2 Type-Feature

Comment 5 by eugene...@chromium.org, May 30 2017

Components: -UI>Browser UI>Browser>Core

Comment 6 by eugene...@chromium.org, Sep 20

Cc: pkl@chromium.org

Comment 7 by pkl@chromium.org, Sep 21

Cc: -kkhorimoto@chromium.org ghendel@chromium.org mrefaat@chromium.org mard...@chromium.org
Owner: kkhorimoto@chromium.org
kkhorimoto@ will be working on this next.
Apparently Desktop and Clank have already done that.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 16 

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

commit 46038d1d5c7d81ec3e439a6a23fe0ab2fa815cb0
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Nov 16 00:13:07 2018

[iOS] Define BCVC presentation context if non-modal dialogs are enabled.

This will allow UIViewControllers presented on the BCVC to be presented
non-modally, leaving the browser controls in the toolbars unaffected.

Bug: 674649
Change-Id: Ib1c256965c4821074c544f5a87792203e2648208
Reviewed-on: https://chromium-review.googlesource.com/c/1334554
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608589}
[modify] https://crrev.com/46038d1d5c7d81ec3e439a6a23fe0ab2fa815cb0/ios/chrome/browser/ui/browser_container/BUILD.gn
[modify] https://crrev.com/46038d1d5c7d81ec3e439a6a23fe0ab2fa815cb0/ios/chrome/browser/ui/browser_container/browser_container_view_controller.mm
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 30 

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

commit feb4d6bc3a2d1809007c278c0bb1c821e2873faf
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Nov 30 03:09:31 2018

[iOS] Expose the viewport insets in FullscreenController.

FullscreenModel knows enough information to calculate these insets, so
it makes sense for the calculation to occur there.  Exposing the
viewport insets in FullscreenController will allow chrome/ layer UI
code to easily account for viewport adjustments.

This API will be used to mask non-modal UIAlertController presentation.

Bug: 674649
Change-Id: I469cb133ef5d27bffcd4de5070bd12820514740e
Reviewed-on: https://chromium-review.googlesource.com/c/1339460
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612538}
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/fullscreen_controller.h
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/fullscreen_controller_impl.h
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/fullscreen_controller_impl.mm
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/fullscreen_model.h
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/fullscreen_model_unittest.mm
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/fullscreen_web_view_resizer.mm
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller.h
[modify] https://crrev.com/feb4d6bc3a2d1809007c278c0bb1c821e2873faf/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller.mm

Comment 11 by kkhorimoto@chromium.org, Dec 3

Status: Started (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 3 

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

commit e2513d16ede8ecff2f0bee7856cafaacd0790b57
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Mon Dec 03 23:20:59 2018

[iOS] Added FullscreenViewportInsetRangeChanged().

This callback can be used to notify FullscreenControllerObservers of
changes to the viewport inset range.  It can be used to trigger a
viewport adjustment for new toolbar heights.

Bug: 674649
Change-Id: I4ecdc374c1017716518118a27d4b9c5e0f1db48f
Reviewed-on: https://chromium-review.googlesource.com/c/1340953
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613313}
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/fullscreen_controller_observer.h
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/fullscreen_mediator.h
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/fullscreen_mediator.mm
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/fullscreen_mediator_unittest.mm
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/fullscreen_model.mm
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/fullscreen_model_observer.h
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller_observer.h
[modify] https://crrev.com/e2513d16ede8ecff2f0bee7856cafaacd0790b57/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller_observer.mm
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 3 

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

commit c46887657098b9d83e3b029dfc9217177e6ebe37
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Mon Dec 03 23:47:26 2018

[iOS] Add AlertCoordinator initializer that takes a BrowserState.

This will allow AlertCoordinators to access FullscreenController, which
will provide the necessary insets to use for non-modal alert
presentation.

This CL also removes the unnecessary empty initializers, as this is
handled by the NS_UNAVAILABLE annotation now.

Bug: 674649
Change-Id: I0d1b5efbcf0266103090e879b078cef64df42593
Reviewed-on: https://chromium-review.googlesource.com/c/1340602
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613327}
[modify] https://crrev.com/c46887657098b9d83e3b029dfc9217177e6ebe37/ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h
[modify] https://crrev.com/c46887657098b9d83e3b029dfc9217177e6ebe37/ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm
[modify] https://crrev.com/c46887657098b9d83e3b029dfc9217177e6ebe37/ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h
[modify] https://crrev.com/c46887657098b9d83e3b029dfc9217177e6ebe37/ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 14 

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

commit 0db12a5791138dff507e3868349c7b33df03d117
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Dec 14 19:03:34 2018

[iOS] Add coordinator for non-modal UIAlertController presentation.

This coordinator disables fullscreen while started and creates a
NonModalAlertPresentationController.  The non-modal presentation is
achieved by adding a mask to the presentation container view and
forwarding touches that occur outside that mask to the BVC.

Also in this CL:
- Updates AlertCoordinator to define the lazy instantiation
  selector in the Subclassing category.
- Updates some FullscreenUIElement selectors to be optional.

Bug: 674649
Change-Id: Ie0df9a993871ebe1a88fda9578093abc38da57c3
Reviewed-on: https://chromium-review.googlesource.com/c/1360273
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616773}
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/BUILD.gn
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/BUILD.gn
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_coordinator.h
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_coordinator.mm
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_presentation_updater.h
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_presentation_updater.mm
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_presentation_updater_unittest.mm
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_touch_forwarder.h
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_touch_forwarder.mm
[add] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/dialogs/non_modal/non_modal_alert_touch_forwarder_unittest.mm
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/fullscreen_controller_observer.h
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/fullscreen_mediator.mm
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/fullscreen_ui_element.h
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/fullscreen_ui_updater.h
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/fullscreen_ui_updater.mm
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/fullscreen_ui_updater_unittest.mm
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller_observer.h
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller_observer.mm
[modify] https://crrev.com/0db12a5791138dff507e3868349c7b33df03d117/ios/chrome/test/BUILD.gn
Project Member

Comment 15 by cr-audit...@appspot.gserviceaccount.com, Jan 7

Labels: CommitLog-Audit-Violation Merge-Without-Approval M-72
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required --
Project Member

Comment 16 by cr-audit...@appspot.gserviceaccount.com, Jan 7

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0

Commit: a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0
Author: kkhorimoto@chromium.org
Commiter: kkhorimoto@chromium.org
Date: 2019-01-07 19:04:38 +0000 UTC

[iOS] Expose the viewport insets in FullscreenController.

FullscreenModel knows enough information to calculate these insets, so
it makes sense for the calculation to occur there.  Exposing the
viewport insets in FullscreenController will allow chrome/ layer UI
code to easily account for viewport adjustments.

This API will be used to mask non-modal UIAlertController presentation.

Bug: 674649
Change-Id: I469cb133ef5d27bffcd4de5070bd12820514740e
Reviewed-on: https://chromium-review.googlesource.com/c/1339460
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612538}(cherry picked from commit feb4d6bc3a2d1809007c278c0bb1c821e2873faf)
Reviewed-on: https://chromium-review.googlesource.com/c/1398781
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#596}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 7

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0

commit a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Mon Jan 07 19:04:38 2019

[iOS] Expose the viewport insets in FullscreenController.

FullscreenModel knows enough information to calculate these insets, so
it makes sense for the calculation to occur there.  Exposing the
viewport insets in FullscreenController will allow chrome/ layer UI
code to easily account for viewport adjustments.

This API will be used to mask non-modal UIAlertController presentation.

Bug: 674649
Change-Id: I469cb133ef5d27bffcd4de5070bd12820514740e
Reviewed-on: https://chromium-review.googlesource.com/c/1339460
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612538}(cherry picked from commit feb4d6bc3a2d1809007c278c0bb1c821e2873faf)
Reviewed-on: https://chromium-review.googlesource.com/c/1398781
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#596}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/fullscreen_controller.h
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/fullscreen_controller_impl.h
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/fullscreen_controller_impl.mm
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/fullscreen_model.h
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/fullscreen_model_unittest.mm
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/fullscreen_web_view_resizer.mm
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller.h
[modify] https://crrev.com/a4094ddf1987dd6bfbeb2509cff16fbc82f9e0e0/ios/chrome/browser/ui/fullscreen/test/test_fullscreen_controller.mm

Comment 18 by kkhorimoto@chromium.org, Jan 7

Labels: -CommitLog-Audit-Violation -Merge-Without-Approval
crrev.com/c/1339460 was merged with approval from  Issue 902271 , as it was required to fix a merge conflict.
►

Sign in to add a comment