Issue metadata
Sign in to add a comment
|
Regression : On NTP, the 'Firstrun' bubble is not aligned properly with the 'Search' icon in omnibox.
Reported by
yfulgaon...@etouch.net,
Feb 9 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version : 58.0.3007.0 (Official Build) 0e09de60e479fe2ee105fb731b403c729fce3ac1-refs/heads/master@{#449173} 32/64 bit OS : Windows 10 What steps will reproduce the problem? 1. Freshly install chrome and launch it (by default user lands on chrome://welcome-win10 page). 2. Scroll down the page, click on 'CONTINUE' button and observe the 'FirstRun' bubble. Actual : 'FirstRun' bubble is not aligned properly with the 'Search' icon in omnibox. Expected : 'FirstRun' bubble should be aligned properly with the 'Search' icon in omnibox. This is a regression issue broken in ‘M-58’, below is the Manual Regression range and will soon update other info. Good build : 58.0.2999.0 Bad build : 58.0.3000.0 Note : This is Windows 10 specific issue and the same is working fine in other OS.
,
Feb 15 2017
I landed a fix for crbug.com/689888 , and it was released in Canary and Dev 58.0.3007.3. I believe that fix should also resolve this; can you re-test?
,
Feb 21 2017
still able to reproduce the issue on windows-10 using chrome version 58.0.3018.0 with the steps mentioned in comment#0. please find the attached screen-cast for reference. Thanks.
,
Feb 21 2017
,
Feb 21 2017
+emily, jdonnelly, felt this may have been caused by the new FRE, or by the "Chrome" omnibox treatment for URLs. Can someone from the omnibox team or security indicators team take a look at this positioning bug?
,
Feb 21 2017
Note: this is related to our new welcome experience for Win10 users (Issue 658434 is the relevant launch bug).
,
Feb 21 2017
+peter may have ideas, or +meacer who wrote the "chrome" chip.
,
Feb 22 2017
Trent, I think you're overloaded right now, but this is closely connected to the "fix position of permission bubbles anchored off the padlock" change you sent me a week and a half ago -- I bet we just need to find and fix (for both Harmony and pre-Harmony) the anchor target of this bubble. Feel free to pawn this off on someone else (bsep@ may have less load but less context).
,
Feb 22 2017
I think the fix for Issue 689888 (#c2) in r449119 is not working (fully) as intended. r449119 is "Suppressing First Run bubble when viewing chrome://welcome-win10". It looks like it is suppressed while the welcome page is showing. But while that page is closing the bubble is told to show, and it anchors "immediately", while the chrome:// chip is still present in the omnibox. r449119 uses first_run.cc::IsOnWelcomePage() which is called on the following triggers: FirstRunBubbleLauncher::FirstRunBubbleLauncher() { registrar_.Add(this, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME, content::NotificationService::AllSources()); // This notification is required to observe the switch between the sync setup // page and the general settings page. registrar_.Add(this, chrome::NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED, content::NotificationService::AllSources()); } These seem to be tuned for the install flow coming from sync, but not the install flow coming from chrome://welcome-win10 I think either - chrome://welcome-win10 needs to adapt so that the flow is closer to existing first-run stuff, or - r449119 needs a tweak to also check that the WebContents passed in to `IsOnWelcomePage(content::WebContents* contents)` is currently in a visible tab, or - we need to use a different notification setup for the first run bubble. (or some combination)
,
Feb 22 2017
Even if the chrome:// chip is still present in the omnibox, we'd still want to anchor correctly to the icon within it (and not the middle or trailing edge of the chip). It seems like fixing that might obviate the need for the changes suggested in comment 9?
,
Feb 22 2017
ahh, it just might.
void LocationBarView::ShowFirstRunBubbleInternal() {
// First run bubble doesn't make sense for Chrome OS.
#if !defined(OS_CHROMEOS)
WebContents* web_contents = delegate_->GetWebContents();
if (!web_contents)
return;
Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
if (browser)
FirstRunBubble::ShowBubble(browser, location_icon_view_);
#endif
}
fix: s/location_icon_view_/location_icon_view_->GetImageView()/
What could go wrong...
,
Feb 22 2017
,
Feb 23 2017
Thanks Trent!
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96752f23762f0ad4320bb4aa0737857b5fe582f3 commit 96752f23762f0ad4320bb4aa0737857b5fe582f3 Author: tapted <tapted@chromium.org> Date: Fri Feb 24 03:04:59 2017 Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Use LocationBarView::GetSecurityBubbleAnchorView(), same as other bubbles. The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG= 690361 Review-Url: https://codereview.chromium.org/2705323004 Cr-Commit-Position: refs/heads/master@{#452728} [modify] https://crrev.com/96752f23762f0ad4320bb4aa0737857b5fe582f3/chrome/browser/ui/views/location_bar/location_bar_view.cc
,
Feb 24 2017
Marking this Fixed. Verification needed when it hits Canary. But if anyone watching wants this merged please move back to Started.
,
Feb 24 2017
Once we verify, I would like to request a merge to 57, since this is an important bit of polish for our new first run experience :)
,
Feb 24 2017
I verified this on Canary with the following steps: 1. Cleared user data directory to simulate fresh install 2. Opened Canary. chrome://welcome appeared (I believe that chrome://welcome appeared instead of chrome://welcome-win10 because Canary can't be set as default. But I imagine the same issue applies, since it also has the "Chrome" chip) 3. Clicked "No thanks" to continue onto the NTP 4. Noticed the DSE bubble was properly anchored to the search icon on the left of the omnibox Requesting a merge. Thanks Trent!
,
Feb 25 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 25 2017
Please merge your change to M57 branch 2987 by 5:00 PM PT Monday (02/27) so we can take it in for next week last M57 Desktop Beta release before Stable promotion. Thank you.
,
Feb 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbe3e139935ab2dc813669ae8ac2d2de8ed3715f commit fbe3e139935ab2dc813669ae8ac2d2de8ed3715f Author: Trent Apted <tapted@chromium.org> Date: Sun Feb 26 23:01:58 2017 [merge-m57] Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Use LocationBarView::GetSecurityBubbleAnchorView(), same as other bubbles. The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG= 690361 Review-Url: https://codereview.chromium.org/2705323004 Cr-Commit-Position: refs/heads/master@{#452728} (cherry picked from commit 96752f23762f0ad4320bb4aa0737857b5fe582f3) Review-Url: https://codereview.chromium.org/2716173002 . Cr-Commit-Position: refs/branch-heads/2987@{#690} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/fbe3e139935ab2dc813669ae8ac2d2de8ed3715f/chrome/browser/ui/views/location_bar/location_bar_view.cc
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb276cc794bf3fb3e7273e4ef2aab7c12622ef36 commit fb276cc794bf3fb3e7273e4ef2aab7c12622ef36 Author: Trent Apted <tapted@chromium.org> Date: Mon Feb 27 03:02:13 2017 [Fix m57 branch merge from] Anchor the first run bubble off the location bar icon's ImageView. Compile fails with 'GetSecurityBubbleAnchorView': identifier not found. Use GetImageView() instead. BUG=696379, 690361 Review-Url: https://codereview.chromium.org/2705323004 Cr-Commit-Position: refs/heads/master@{#452728} (cherry picked from commit 96752f23762f0ad4320bb4aa0737857b5fe582f3) Review-Url: https://codereview.chromium.org/2712373002 . Cr-Commit-Position: refs/branch-heads/2987@{#692} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/fb276cc794bf3fb3e7273e4ef2aab7c12622ef36/chrome/browser/ui/views/location_bar/location_bar_view.cc
,
Feb 27 2017
,
Mar 1 2017
Rechecked this issue on chrome version 57.0.2987.88 on Windows 10, fix is working as intended. 'FirstRun' bubble is aligned properly with the 'Search' icon in omnibox. Adding TE-Verified labels. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by msrchandra@chromium.org
, Feb 9 2017Owner: tmartino@chromium.org
Status: Assigned (was: Unconfirmed)