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

Issue 768319 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: 'People,On-startup & Advanced' links are seen missing in settings main menu.

Project Member Reported by jbanavatu@chromium.org, Sep 25 2017

Issue description

Chrome Version: 63.0.3222.0/9972.0.0 dev-channel Gnawty,Reks and Peppy
OS: Chrome OS

What steps will reproduce the problem?
(1)Sign in to chrome >> Navigate to chrome://settings page >> Click on settings main menu icon and Observe

Expected: All the links should be displayed in settings main menu.
Actual: Instead, 'People,On-startup & Advanced' links are seen missing.

This is regression issue as all links are seen in 63.0.3218.0/9963.0.0 dev channel Peppy.

Attaching screen-shots for reference.
 
Actual.png
52.3 KB View Download
Expected.png
620 KB View Download
Cc: michae...@chromium.org x...@chromium.org dpa...@chromium.org hcarmona@chromium.org
Owner: minch@chromium.org
This was caused by:
https://chromium-review.googlesource.com/c/chromium/src/+/661070

I am surprised we do not have a test to catch this :( I kind of assumed we did in the review. We should definitely add one.

We should revert the CL right away then add a test that would catch this and fix the CL.



Comment 2 by minch@chromium.org, Sep 25 2017

Sorry for this, will revert this cl now, fix it and add a test for it.

Comment 3 by dpa...@chromium.org, Sep 25 2017

+1 on adding tests for this. Also the way we use pageVisibility is a bit confusing (but performant), and it is explained by this comment in the code

"All pages are visible when not set because polymer only notifies after a property is set."

Basically (IIRC):
 - if pageVisibility is defined, then booleans for *all* values should be also defined inside pageVisibility, otherwise sections will be erroneously hidden. 
 - if pageVisibility is undefined, then everything is visible.

Comment 4 by dpa...@chromium.org, Sep 25 2017

Cc: scottchen@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 28 2017

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

commit 1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055
Author: MinChen <minch@chromium.org>
Date: Thu Sep 28 00:39:09 2017

[Reland]Make it can't open wallpaper manager if wallpaper is policy controlled.

Changes,
1. Show the wallpaper setting row only for login, whitelist types and active users
(should satisfy the whole conditions at the same time).
2. Only can open the wallpaper manager if the wallpaper is not policy controlled.
3. Added wallpaper policy indicator. Show it if the wallpaper is policy
controlled.

Bug:  749755 , 768319 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I07ffb372bf511411188b847c969cdc2341658bfa
Reviewed-on: https://chromium-review.googlesource.com/683221
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: min c <minch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504820}
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/resources/settings/appearance_page/appearance_page.html
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/resources/settings/appearance_page/appearance_page.js
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/resources/settings/page_visibility.js
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/resources/settings/settings_menu/settings_menu.html
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/ui/webui/settings/appearance_handler.cc
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/browser/ui/webui/settings/appearance_handler.h
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/test/data/webui/settings/appearance_page_test.js
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/test/data/webui/settings/settings_main_test.js
[modify] https://crrev.com/1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055/chrome/test/data/webui/settings/settings_menu_test.js

Comment 6 by minch@chromium.org, Sep 28 2017

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 13 2017

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

commit 5e5407114f7169b3f14aae42b02fa8835da7604e
Author: MinChen <minch@chromium.org>
Date: Fri Oct 13 01:04:39 2017

[Merge to M62][Reland]Make it can't open wallpaper manager if wallpaper is policy controlled.

TBR=minch@chromium.org

Changes,
1. Show the wallpaper setting row only for login, whitelist types and active users
(should satisfy the whole conditions at the same time).
2. Only can open the wallpaper manager if the wallpaper is not policy controlled.
3. Added wallpaper policy indicator. Show it if the wallpaper is policy
controlled.

(cherry picked from commit 1c8d60574cb3aa2db0dc9bb96010aa52ec4bd055)

Bug:  749755 , 768319 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I07ffb372bf511411188b847c969cdc2341658bfa
Reviewed-on: https://chromium-review.googlesource.com/683221
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: min c <minch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#504820}
Reviewed-on: https://chromium-review.googlesource.com/717716
Reviewed-by: min c <minch@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#676}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/resources/settings/appearance_page/appearance_page.html
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/resources/settings/appearance_page/appearance_page.js
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/resources/settings/page_visibility.js
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/resources/settings/settings_menu/settings_menu.html
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/ui/webui/settings/appearance_handler.cc
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/browser/ui/webui/settings/appearance_handler.h
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/test/data/webui/settings/appearance_page_test.js
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/test/data/webui/settings/settings_main_test.js
[modify] https://crrev.com/5e5407114f7169b3f14aae42b02fa8835da7604e/chrome/test/data/webui/settings/settings_menu_test.js

Status: Verified (was: Fixed)
10032.29.0, 63.0.3239.37

Sign in to add a comment