New Download Manager UI covers adaptive toolbar |
||||||||
Issue descriptionApp Version (from "Chrome Settings > About Chrome"): M66 iOS Version: All Device: All Steps to reproduce: 1.) Enable Adaptive Toolbar with #ui-refresh-phase-1 flag 2.) Enable Download Manager with #new-file-download flag 3.) Load https://www.barebones.com/products/bbedit/download.html 4. Tap Download Observed behavior: Download Manager UI covers bottom toolbar Expected behavior: Is this expected behavior?
,
Mar 5 2018
I guess the question here: is this a bug or WAI?
,
Mar 6 2018
This should be treated as a bug. The initial proposal for Bijou was to have infobars, snackbars, etc., to be presented from underneath the toolbar, shown here: https://docs.google.com/presentation/d/1PmopKVCKBfkngGTw7oiIVTEDflnC-cdXW0ZzkahaMVY/edit#slide=id.g2936d69693_4_4
,
Mar 6 2018
,
Mar 27 2018
The problem is that VerticalAnimationContainer presents the view above the toolbar and anchors that presented view to the bottom of base view (which is BVC). In order to fix the problem I propose the following steps: 1.) Create View controller for BrowserViewController.contentArea 2.) Use that new controller as base view controller for DownloadManagerCoordinator prototype: crrev.com/c/981684 3.) Fix BrowserViewController.contentArea frame (crbug.com/826130) This will anchor presented view to the bottom of web contents view (same location as toolbar's top anchor) and will animate download manager behind the toolbar. Mark, Gauthier, does this plan sounds good to you?
,
Mar 27 2018
It sounds OK. Please note that the BVC currently doesn't really handle well the rotations with the adaptive toolbar (see issue 823712).
,
Apr 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/687567cfbdd4c129191bae29f56e31fd3f364349 commit 687567cfbdd4c129191bae29f56e31fd3f364349 Author: Eugene But <eugenebut@google.com> Date: Mon Apr 02 16:07:32 2018 Always use _contentArea as superview for Tab's view. Previously BVC used self.view as superview for tab.view to animate NTP opening. This disallowed to create a view controller for _contentArea because views hierarchy should always match view controller's hierarchy. To allow view controller creation, this CL always uses _contentArea as superview for tab.view. In order to avoid animation issues, _contentArea is temporary resized during NTP opening animation. Doing this resize is fine in the short term, because _contentArea always hosts NTP for this specific case. In the long term this code will be removed because we will need a different NTP opening animation. A separate _contentArea view controller will allow to fix download manager presentation. That view controller will be a base view controller for download manager coordinator, so download manager will be presented in _contentArea behind the bottom toolbar. _contentArea is created in a separate CL: crrev.com/c/981684 Bug: 818264 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I6e642d9e8fd856ef4c1a0fceb1cf7615dad050a7 Reviewed-on: https://chromium-review.googlesource.com/985150 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#547442} [modify] https://crrev.com/687567cfbdd4c129191bae29f56e31fd3f364349/ios/chrome/browser/ui/browser_view_controller.mm
,
Apr 6 2018
Issue 829947 has been merged into this issue.
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cebbb3329f5e29b780a4b9072d67d67646615a8 commit 4cebbb3329f5e29b780a4b9072d67d67646615a8 Author: Eugene But <eugenebut@google.com> Date: Fri Apr 13 20:33:16 2018 Add BrowserContainerViewController class to manage BrowserContainerView. This view controller will be used as a base view controller for Download Manager presentation. Download Manager must be presented under toolbar view, so using BVC as a base view controller does not work. This CL replaces BrowserContainerView with BrowserContainerViewController which should not cause any functional changes. Bug: 818264 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I93d66906c671a8647589db8d9717ab2dcd26cd51 Reviewed-on: https://chromium-review.googlesource.com/981684 Reviewed-by: Mark Cogan <marq@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#550747} [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/BUILD.gn [delete] https://crrev.com/c430ae59f895b5d2d1516f9285fc7cda4e589bd3/ios/chrome/browser/ui/browser_container_view.h [add] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_container_view_controller.h [rename] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_container_view_controller.mm [add] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_container_view_controller_unittest.mm [delete] https://crrev.com/c430ae59f895b5d2d1516f9285fc7cda4e589bd3/ios/chrome/browser/ui/browser_container_view_unittest.mm [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_view_controller.h [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_view_controller_unittest.mm
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cebbb3329f5e29b780a4b9072d67d67646615a8 commit 4cebbb3329f5e29b780a4b9072d67d67646615a8 Author: Eugene But <eugenebut@google.com> Date: Fri Apr 13 20:33:16 2018 Add BrowserContainerViewController class to manage BrowserContainerView. This view controller will be used as a base view controller for Download Manager presentation. Download Manager must be presented under toolbar view, so using BVC as a base view controller does not work. This CL replaces BrowserContainerView with BrowserContainerViewController which should not cause any functional changes. Bug: 818264 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I93d66906c671a8647589db8d9717ab2dcd26cd51 Reviewed-on: https://chromium-review.googlesource.com/981684 Reviewed-by: Mark Cogan <marq@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#550747} [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/BUILD.gn [delete] https://crrev.com/c430ae59f895b5d2d1516f9285fc7cda4e589bd3/ios/chrome/browser/ui/browser_container_view.h [add] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_container_view_controller.h [rename] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_container_view_controller.mm [add] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_container_view_controller_unittest.mm [delete] https://crrev.com/c430ae59f895b5d2d1516f9285fc7cda4e589bd3/ios/chrome/browser/ui/browser_container_view_unittest.mm [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_view_controller.h [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/4cebbb3329f5e29b780a4b9072d67d67646615a8/ios/chrome/browser/ui/browser_view_controller_unittest.mm
,
Apr 23 2018
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26 commit 49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26 Author: Eugene But <eugenebut@google.com> Date: Tue May 22 17:22:43 2018 Correct Download Manager presentation with UI Refresh flag enabled. This CL presents Download Manager from _browserContainerViewController, which means that UI shows up between Web Content and Secondary Toolbar on z-axe. This is only done if UI Refresh flag is enabled. Also extended Download Manager coordinator and view controller to support bottomMarginHeightAnchor property. This property allows adding bottom margin to download manager UI in order to present Download Manager UI above Secondary Toolbar on y-axe. Bug: 818264 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I373ecb6e9892bdc20ba720742d653471ff0a9bad Reviewed-on: https://chromium-review.googlesource.com/1024641 Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#560672} [modify] https://crrev.com/49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26/ios/chrome/browser/ui/download/download_manager_coordinator.h [modify] https://crrev.com/49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26/ios/chrome/browser/ui/download/download_manager_coordinator.mm [modify] https://crrev.com/49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26/ios/chrome/browser/ui/download/download_manager_view_controller.h [modify] https://crrev.com/49b3ffb2e83ea50de3f931d4b12c5a0d26ccee26/ios/chrome/browser/ui/download/download_manager_view_controller.mm
,
May 22 2018
,
Aug 20
Issue verified Version: Chrome Beta 69.0.3497.50 Device: iPhone 8 iOS: 12.0 No overlapping https://drive.google.com/open?id=1NlZTBtlioNQfkl0wF08X140aQRdFWyg0 |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by pkl@chromium.org
, Mar 5 2018Owner: eugene...@chromium.org
Status: Assigned (was: Untriaged)