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

Issue 690361 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



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 description

Chrome 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.
 
Actual_FirstRun.mp4
548 KB View Download
Expected_FirstRun.mp4
611 KB View Download
Act_Exp_FirstRun_bubble.png
38.8 KB View Download
Labels: hasbisect-per-revision
Owner: tmartino@chromium.org
Status: Assigned (was: Unconfirmed)
Using the per-revision bisect providing the bisect results,
Good Build -- 58.0.2999.0  (revision : 447413)
Bad Build  -- 58.0.3000.0  (revision : 447669)

You are probably looking for a change made after 447607 (known good), but no later than 447608 (first known bad).
CHANGELOG URL:
 https://chromium.googlesource.com/chromium/src/+log/e027f6a77853952285de14c8b9009c208b4c2c99..f9a9ef14dfdb45e70a898024b5c690ac4bf3a5e0

@tmartino -- Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.
Note: This looks similar to Bug Id: 689888.

Thanks!
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?
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.
win-690361.mp4
706 KB View Download

Comment 4 by ew...@chromium.org, Feb 21 2017

Cc: ew...@chromium.org rpop@chromium.org
Labels: -M-58 M-57

Comment 5 by rpop@chromium.org, Feb 21 2017

Cc: tmartino@chromium.org f...@chromium.org emilyschechter@chromium.org jdonnelly@chromium.org
Owner: ----
Status: Available (was: Assigned)
+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?

Comment 6 by ew...@chromium.org, Feb 21 2017

Note: this is related to our new welcome experience for Win10 users (Issue 658434 is the relevant launch bug).
Cc: pkasting@chromium.org mea...@chromium.org
+peter may have ideas, or +meacer who wrote the "chrome" chip.
Owner: tapted@chromium.org
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).

Comment 9 by tapted@chromium.org, Feb 22 2017

Cc: tapted@chromium.org
Owner: tmartino@chromium.org
Status: Assigned (was: Available)
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)
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?
Owner: tapted@chromium.org
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... 
Status: Started (was: Assigned)
https://codereview.chromium.org/2705323004/

Comment 13 by ew...@chromium.org, Feb 23 2017

Thanks Trent!
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Marking this Fixed. Verification needed when it hits Canary. But if anyone watching wants this merged please move back to Started.

Comment 16 by ew...@chromium.org, Feb 24 2017

Status: Started (was: Fixed)
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 :)

Comment 17 by ew...@chromium.org, Feb 24 2017

Labels: Merge-Request-57
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!
Project Member

Comment 18 by sheriffbot@chromium.org, Feb 25 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
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
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.
Project Member

Comment 20 by bugdroid1@chromium.org, Feb 26 2017

Labels: -merge-approved-57 merge-merged-2987
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

Project Member

Comment 21 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: TE-Verified-57.0.2987.88 TE-Verified-M57
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