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

Issue 839458 link

Starred by 10 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

[User Feedback - Beta] User Resolution Preference Reset After Update

Project Member Reported by craigtumblison@chromium.org, May 3 2018

Issue description

Chrome Version: 67.0.3396.26

Steps To Reproduce:
(1) Be on M66, change default "Resolution" preference in settings.
(2) Update to M67, notice preference has been reset.
(3) Look for "Resolution" in settings, find that it's now "Display Size"


Expected Result:
Preference should remain / be converted into the new percentage format.


Actual Result:
Preference is reset and users are unable to find the new setting easily.


This appears to be happening following yesterday's M67 beta push.
 
Components: -UI UI>HighDPI
Updating component based on bug 823922.
Cc: ovanieva@chromium.org afakhry@chromium.org kaznacheev@chromium.org osh...@chromium.org abodenha@chromium.org
Owner: malaykeshav@chromium.org
This is expected. Zoom factor uses a completely different approach than ui scale.

Adding a converter between ui scale and zoom factor, though possible to some extent, increases the surface for error.

>Preference is reset and users are unable to find the new setting easily.
I find this strange. If a user was able to modify the ui scale, then they should easily be able to find the new setting as well. (They are exactly at the same location)
malaykeshav@ Thanks for confirming this is intended!

Just to clarify, does this mean we'll expect to see this preference reset across all users (on high DPI devices) when this feature hits the stable channel?

As for the confusion, it's probably *not* a big deal, but it is jarring if the user had previously modified that preference and then upon noticing it was reset, cannot find it by the same name.

Thanks again!
> does this mean we'll expect to see this preference reset across all users (on high DPI devices) when this feature hits the stable channel?
Yes. Any change in the resolution on the internal display will be reset to the default.

However I think users might atleast want to see the effective resolution somewhere. Initial feedbacks tend to point to this.
Summary: [User Feedback - Beta] User Resolution Preference Reset After Update (was: User Resolution Preference Reset After Update)
Labels: -Pri-3 ReleaseBlock-Stable M-68 Pri-1
This needs to block rollout of the display scale feature.
Labels: M-67
Adding tag for M67 as this needs to be disabled for this version. 
Labels: Merge-Request-67
Adding merge request before landing the change to disable display zoom directly on M67.
Project Member

Comment 12 by sheriffbot@chromium.org, May 11 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Is the M67 merge request to disable the feature as opposed to a code change? CL?
Yes. It is only to disable the feature. The CL will land directly on M67.
Sounds good.  Possible to test prior to the merge?
I am testing the same. The only possible bug in this that might occur is for beta or dev users who have already got it enabled. I am testing to ensure that disabling this feature doesnt break anything. So far I am not able to find any bugs.
Tested it on eve multiple times, no bug confirmed.
ping
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 Chrome OS.

Project Member

Comment 21 by bugdroid1@chromium.org, May 15 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4859206689324de5bc045a67619dede9129b158a

commit 4859206689324de5bc045a67619dede9129b158a
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Tue May 15 21:57:59 2018

Disable Display Zoom by default on M67

This patch disables display zoom by default on M67 to find a better user
experience during the transition and also to prevent the breaking of any
kiosk stations.

Bug:  839458 
Change-Id: I367f27d7904d380a295d9435077ab379db25bb94
Component: Display zoom, flag
Reviewed-on: https://chromium-review.googlesource.com/1056215
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#610}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/4859206689324de5bc045a67619dede9129b158a/ui/display/display_switches.cc

Status: Fixed (was: Unconfirmed)
M67 stable users will no longer face this issue of sudden resolution change on update.

However, beta users who are using display zoom and have changed the zoom level from anything other than 100% will see a change on their next M67 beta update. 
Cc: kbleicher@chromium.org
Hi, this got merged to 67.0.3396.49.  Can the change be tested against that rev?  I have reason to want to be sure.

Re: #22, if beta users see the change would it be cosmetic only?
Cc: vaandres@chromium.org abod...@chromium.org
Tested on 10575.40.0, 67.0.3396.49 build.
Display zoom by default is disabled.

vaandres@, Please confirm with kiosk mode.
abodeti@, Please confirm after AU.
@sontis Thanks for the update!

Can the test team please ensure the following 2 scenarios are testesd and WAI:

Scenario 1

 Step 1) Be on any version less than M67. (M66 or M65 or M64)
 Step 2) Set the internal display resolution to something other than Best or native.
 Step 3) Update to a beta version where display zoom is enabled by default.
 Step 4) Change the display zoom to something other than 100%
 Step 5) Update to 67.0.3396.49 where display zoom is disabled again.
 Step 6) Change the ui scale to something other than best or native.
 Step 7) Open browser and go to gmail.com.

Expected:
You can interact with gmail. Click buttons and hover over UI elements within gmail.

Scenario 2

 Step 1) Be on any version less than 67.0.3396.49 where display zoom is enabled by default.
 Step 2) Disable the display zoom from chrome://flags
 Step 3) Change the display resolution to something other than best or native.
 Step 4) Update to 67.0.3396.49 where display zoom is disabled by default.
 Step 5) Open gmail on browser and interact with the website.

Expected:
Nothing changes on update. the resolution should stay as is. And click and tap events must be working as expected in gmail.
#23
If the beta users had fiddled with the zoom level, it would be lost. And upon update they would be at the resolution tagged as "best". Which could be smaller or larger based on what they had set the zoom level to before the update.
Display size setting on my eve device was enabled by default (from chrome://flags) and it was set to 130% before the AU. Then, performed AU from 67.0.3396.41--> 67.0.3396.49. After the AU, I'm seeing resolution property being set at 1200*800. It doesn't seem like appropriate mapping from display size to resolution. Is this intended?
Yes. This is intended. 
I am seeing the same behavior as mkarkada@. Using a kiosk device flashed with 10575.39.0. I changed the zoom to be 130%, then proceed to auto update to 10575.40.0. Once the auto update completes, I seeing the screen resolution is now at 1200x800(Best)

Touch interactions with the kiosk device seem to be working fine.
Yes. This is intended. This change is to prevent kiosks from breaking when they upgrade from M66 to M67 stable and suddenly have their preset resolutions changed. 

However for kiosks that are already on M67 beta/dev, might have a different experience after the update. (as pointed out in #27 and #29)
Status: Verified (was: Fixed)
Both the scenarios in c#25 is WAI. Verified on M67 (10575.40.0, 67.0.3396.49) coral device.
verified both scenarios using veyron device flashed with 10575.40.0, 67.0.3396.49
Cc: malaykeshav@chromium.org
 Issue 840867  has been merged into this issue.
Project Member

Comment 34 by bugdroid1@chromium.org, May 23 2018

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

commit c95deeda7112e9622192e36a2df2e4e972342978
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Wed May 23 03:06:00 2018

Convert ui scale to the corresponding display zoom scale

Currently users have the choice to change the ui scale on their internal
displays via the display resolution slider in settings. Upon updating to
a version of chrome that has display zoom enabled by default, the users
will experience a sudden change in their logical resolutions on first
boot. This is bad user experience. It may also lead to kiosk stations
that were using ui scale to break.

This patch prevents this by converting the ui scale to the corresponding
display zoom scale and also ensures that the current zoom scale is
present in the list of available zoom levels.

NOTE - The display zoom scale set after this conversion may not be in
the list of available zoom levels for that display. This means that if
a users tries to change the zoom level(either from the settings page or
via the keyboard shortcut), they may not be able to come back to this
zoom level again.

Bug:  839458 
Change-Id: I4d5bd761506c6ec14dc92b3d3a120b87e02e2cb7
Component: Display Manager, display zoom, ui scale
Reviewed-on: https://chromium-review.googlesource.com/1056192
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560911}
[modify] https://crrev.com/c95deeda7112e9622192e36a2df2e4e972342978/ash/display/cros_display_config.cc
[modify] https://crrev.com/c95deeda7112e9622192e36a2df2e4e972342978/ash/display/display_manager_unittest.cc
[modify] https://crrev.com/c95deeda7112e9622192e36a2df2e4e972342978/ui/display/manager/display_manager.cc
[modify] https://crrev.com/c95deeda7112e9622192e36a2df2e4e972342978/ui/display/manager/display_util.cc
[modify] https://crrev.com/c95deeda7112e9622192e36a2df2e4e972342978/ui/display/manager/display_util.h
[modify] https://crrev.com/c95deeda7112e9622192e36a2df2e4e972342978/ui/display/manager/display_utils_unittest.cc

Project Member

Comment 35 by bugdroid1@chromium.org, May 23 2018

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

commit fe197c07d16f7bed354454ad63991bb9b4faa526
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Wed May 23 23:21:56 2018

Minor bug fix for incorrect DCHECK in display util

This patch fixes an incorrect DCHECK that was landed in the previous
patch. The DHCECK_LT should instead be DCHECK_GT.

Bug:  839458 
Change-Id: I4b45aae9a1cb1c3870774b4c3e2d654ffbf7bb4f
OriginalChange: I4d5bd761506c6ec14dc92b3d3a120b87e02e2cb7
Component: Display util, dsf, zoom levels
Reviewed-on: https://chromium-review.googlesource.com/1070619
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561305}
[modify] https://crrev.com/fe197c07d16f7bed354454ad63991bb9b4faa526/ui/display/manager/display_util.cc
[modify] https://crrev.com/fe197c07d16f7bed354454ad63991bb9b4faa526/ui/display/manager/display_utils_unittest.cc

Project Member

Comment 36 by bugdroid1@chromium.org, May 24 2018

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

commit a2b54520f8ec0bc7f928b8fa21f83f094eb4f86b
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Thu May 24 04:53:06 2018

Revert "Minor bug fix for incorrect DCHECK in display util"

This reverts commit fe197c07d16f7bed354454ad63991bb9b4faa526.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 561305 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2ZlMTk3YzA3ZDE2ZjdiZWQzNTQ0NTRhZDYzOTkxYmI5YjRmYWE1MjYM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/27583

Sample Failed Step: display_unittests

Original change's description:
> Minor bug fix for incorrect DCHECK in display util
> 
> This patch fixes an incorrect DCHECK that was landed in the previous
> patch. The DHCECK_LT should instead be DCHECK_GT.
> 
> Bug:  839458 
> Change-Id: I4b45aae9a1cb1c3870774b4c3e2d654ffbf7bb4f
> OriginalChange: I4d5bd761506c6ec14dc92b3d3a120b87e02e2cb7
> Component: Display util, dsf, zoom levels
> Reviewed-on: https://chromium-review.googlesource.com/1070619
> Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561305}

Change-Id: I803ccaa9ecb839f2efc4499c9f78dfbf346578ea
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  839458 
Reviewed-on: https://chromium-review.googlesource.com/1070892
Cr-Commit-Position: refs/heads/master@{#561387}
[modify] https://crrev.com/a2b54520f8ec0bc7f928b8fa21f83f094eb4f86b/ui/display/manager/display_util.cc
[modify] https://crrev.com/a2b54520f8ec0bc7f928b8fa21f83f094eb4f86b/ui/display/manager/display_utils_unittest.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Jun 4 2018

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

commit f8617dddbbdf3735052ea5787af1589221279306
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Mon Jun 04 19:08:46 2018

(Reland) Minor bug fix for incorrect DCHECK in display util

This patch fixes an incorrect DCHECK that was landed in the previous
patch. The DHCECK_LT should instead be DCHECK_GT.

This is a direct reland. The original bug which was due to an
invalid memory access is already fixed and landed:
https://chromium-review.googlesource.com/c/chromium/src/+/1069561/17/ui/display/manager/display_util.cc#193

Bug:  839458 
Change-Id: Ie87781875e5c4a2df15db8bf2106efaa4806f070
OriginalChange: I4d5bd761506c6ec14dc92b3d3a120b87e02e2cb7
Component: Display util, dsf, zoom levels
Reviewed-on: https://chromium-review.googlesource.com/1081279
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564176}
[modify] https://crrev.com/f8617dddbbdf3735052ea5787af1589221279306/ui/display/manager/display_util.cc
[modify] https://crrev.com/f8617dddbbdf3735052ea5787af1589221279306/ui/display/manager/display_utils_unittest.cc

Comment 38 by wfh@chromium.org, Jun 23 2018

Labels: -M-67
Status: Assigned (was: Verified)
This happened again to me

link, Updating to 68.0.3440.34 (Official Build) beta (64-bit)

See  issue 844498  for my original report, should I reopen that, or can it be handled here?

Comment 39 by wfh@chromium.org, Jun 23 2018

it looks like it was updating from .25

[0620/110702:INFO:omaha_request_action.cc(1099)] Omaha request response: <?xml version="1.0" encoding="UTF-8"?><response protocol="3.0" server="prod"><daystart elapsed_days="4188" elapsed_seconds="40022"/><app appid="{F26D159B-52A3-491A-AE25-B23670A66B32}" cohort="1:3:" cohortname="link_beta" status="ok"><ping status="ok"/><updatecheck status="ok"><urls><url codebase="http://dl.google.com/chromeos/link/10718.22.0/beta-channel/"/><url codebase="https://dl.google.com/chromeos/link/10718.22.0/beta-channel/"/></urls><manifest version="10718.22.0"><actions><action event="update" run="chromeos_10575.54.0-10718.22.0_link_beta-channel_delta_mp-v5.bin-38c5c5ac9bbcf649f8e6bf05c8a1cd2d.signed"/><action ChromeOSVersion="10718.22.0" ChromeVersion="68.0.3440.25" IsDelta="true" IsDeltaPayload="true" MaxDaysToScatter="14" MetadataSignatureRsa="ZWhnbzylmRzQLJHo8LzgRBUuZsltZiyo5utom+8OPWS7+Z4/oDdE1/K2P9AUVKakADvF1MCYZWxrZeqDR5U4sWztmGys3KfQAo/Fro4N6NbnW1CNL9p5egnMW4jA/UUFwLjUPRkUWUYCIl5gJRQgTlN62a4LRrjCK7jtf7oMBACEXPj3yKP1DjLpyE9lNao92VYKhcXLey4+xNojNwNjxxa5rw0j9diFwUyD/F1yivZrRwIMIrlcWg5pSsL7Vs6Dc8T3TsqzKEq1PdhkrdAw2ROB6c3KQpTgGDqpwUEoas1FH2iYWYfy5h6E3bHhM307nNwGg8BculL9CXwSIf8OGg==" MetadataSize="604373" event="postinstall" sha256="0QjLL9C9VcuSlieDYW4x8VYK3vjWJACsGSsxW+vXySQ="/></actions><packages><package fp="1.d108cb2fd0bd55cb92962783616e31f1560adef8d62400ac192b315bebd7c924" hash="0Ya7NnLH053SAZfW0QarjEnHxPU=" hash_sha256="d108cb2fd0bd55cb92962783616e31f1560adef8d62400ac192b315bebd7c924" name="chromeos_10575.54.0-10718.22.0_link_beta-channel_delta_mp-v5.bin-38c5c5ac9bbcf649f8e6bf05c8a1cd2d.signed" required="true" size="69308738"/></packages></manifest></updatecheck><event status="ok"/></app></response>
[0620/110702:INFO:omaha_request_action.cc(935)] Found 1 <app>.

There's quite a lot of CLs related to screen resolution in the change lists between those two versions

https://chromium.googlesource.com/chromium/src/+log/68.0.3440.25..68.0.3440.34?pretty=fuller&n=10000

Comment 40 by wfh@chromium.org, Jun 25 2018

This also happened today on another machine (chell) - now running 68.0.3440.34 - the resolution was reset. Looking at chrome://system logs, I'm not sure what this machine was running previously.
Status: Fixed (was: Assigned)
This was expected on beta channels. The transition code was meant for transitioning to M68 from a pre M68 version. Which means people who were already on M68 beta may see a change in their display size on an update to 68.0.3440.34 beta. 

Sign in to add a comment