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

Issue 769379 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

Content Suggestion article content os covered by Toolbar

Project Member Reported by eugene...@chromium.org, Sep 27 2017

Issue description

App Version (from "Chrome Settings > About Chrome"): 63.0.3221.0 dev
iOS Version: iOS 11
Device: iPhone SE

Steps to reproduce: 
1.) Open NTP
2.) Scroll to the bottom to reveal content suggestions article
3.) Long press on the article
4.) Tap "Open in New Tab"
5.) Switch to the newly opened Tab by side-swiping the toolbar

Observed behavior: 
Top part of the page content is covered by the toolbar permanently. 

Expected behavior: 
Page content should not be hidden behind the toolbar.

Additional comments: 
Does not reproduce on every web site. Reproducible on kotaku.com f.e.
 
Components: UI>Browser>Toolbar
Owner: kkhorimoto@chromium.org
Status: Assigned (was: Untriaged)
I think this is related to the URL (it only happens with amp pages) more than than the swipe gesture/NTP. Can you check if opening them in a new tab, then navigating using the stack view gives the same result? It does for me.
Probably more related to toolbar, adding Kurt to see if it is related to fullscreen.
Components: UI>Browser>FullScreen
The bug is probably somewhere in Fullscreen code.
Labels: zine-triaged
Cc: linds...@chromium.org
Hi,
This is listed as a P1 for M62. Are there any updates for this?
Thanks,
Yeah this looks like a fullscreen issue.  I'll take a look at it today or tomorrow.
This issue is still reproduced as of M66.0.3351.0 canary
Tested on iPhone7 plus, 10.3.3 iPhoneX iOS11.3.
Status: Started (was: Assigned)

Comment 9 Deleted

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 23 2018

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

commit e2ec670f6f93cf2994334587b4ab03aa0a9bdca6
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Feb 23 22:33:55 2018

[iOS] Move content below header the first time a WebState is activated.

This CL also renames UpdateFullscreenWebViewProxyForReplacedScrollView()
to MoveContentBelowHeader(), as it is used in additional places other
than just for replaced scroll views.

Bug:  769379 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I330e20e33501429140a8ea3dfd9e8afa34d47fcf
Reviewed-on: https://chromium-review.googlesource.com/933921
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538925}
[modify] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/BUILD.gn
[add] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/fullscreen_content_adjustment_util.h
[rename] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/fullscreen_content_adjustment_util.mm
[modify] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/fullscreen_web_state_list_observer.h
[modify] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/fullscreen_web_state_list_observer.mm
[modify] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/fullscreen_web_state_list_observer_unittest.mm
[modify] https://crrev.com/e2ec670f6f93cf2994334587b4ab03aa0a9bdca6/ios/chrome/browser/ui/fullscreen/fullscreen_web_view_proxy_observer.mm
[delete] https://crrev.com/9f37416d6fef266dcaeed0b09d0c79584a3d71ef/ios/chrome/browser/ui/fullscreen/fullscreen_web_view_scroll_view_replacement_util.h

Cc: cma...@chromium.org
Labels: Merge-Request-65
Status: Fixed (was: Started)
+cmasso for merge decision.  This only reproduces when using the toolbar side swipe gesture to switch tabs, so it might be enough of an edge case that we would not want to merge this to M65.
Project Member

Comment 12 by sheriffbot@chromium.org, Feb 23 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: Less than 7 days to go before AppStore submit on M65
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 13 by cmasso@google.com, Feb 26 2018

Is the fix risky?
No, I think this is pretty low risk.  The CL is mostly a function rename.  The new logic just moved page content below the header when a tab is activated for the first time, but the implementation of MoveContentBelowHeader() uses the current fullscreen progress to perform its logic so even if there is a bug and it's called mistakenly, it should never result in a situation where the top of the page's content is out of sync with the toolbar.

Comment 15 by cmasso@google.com, Feb 26 2018

Labels: -Hotlist-Merge-Review -Merge-Review-65 Merge-Rejected-65
Since this is not new in M65, let's ship it with M66 instead. Thanks for the fix Kurt!
Status: Verified (was: Fixed)
Issue verified 
Version: Chrome Canary 66.0.3356.0
Device: iPhone SE 
iOS: 11.2.5

Page content is correctly displayed for content suggestions articles. Not sure how to generate an content suggestions article for kotaku.com, but it works correctly as most visited icon.
https://drive.google.com/open?id=1I5FdSoD-efOTFmvxK2aBj-IEzN7xEGUt

Sign in to add a comment