New issue
Advanced search Search tips

Issue 899084 link

Starred by 1 user
Status: Fixed
Owner:
msw@chromium.org
Closed: Nov 7
Cc:
sky@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

KSV app textfield context menu misplaced with display scaling

Project Member Reported by msw@chromium.org, Oct 25 
KSV app textfield context menu misplaced with display scaling

On an eve dev device at 71.0.3565.0 or on linux-chromeos on ToT @ #600836
(1) Run chrome with a non-native display scale:
    (eg. on eve pick a chrome://settings/display Display Size not "Tiny", or use Ctrl-Shift-[+/-])
    (on linux-chromeos run "chrome --ash-host-window-bounds=1200x800*1.2")
(2) Open the Keyboard Shortcut Viewer with Ctrl-Alt-/ or search "Shortcuts" in the app list
(3) Right click the search textfield to show the context menu
Expected: context menu appears near the cursor
Actual: context menu appears far from the cursor (low and right)

This likely has to do with pixels/dips for context window placement, I'll take a closer look.
Hopefully we can merge a fix to M-70 or at least M-71, not sure if it's a release blocker...
KSV_context_misplaced_120_percent.png
495 KB View Download

Comment 1 by sky@chromium.org, Oct 25 

I can't imagine this scenario happens all that often. I don't think this should block 70.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 6 

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

commit e0e48885d523be60a79913ae9e0e122217188deb
Author: Mike Wasserman <msw@chromium.org>
Date: Tue Nov 06 00:40:52 2018

ws: Fix scaling in DesktopWindowTreeHostMus during Init

Initialize the cached display device scale factor before usage.
Otherwise, the scale factor is 1 and the window is misplaced/sized.

I'll land a unit test separately, to make merging easier.

Bug:  899084 , 665965
Test: KSV (Ctrl-Alt-/) search context menu supports display & UI scales.
Change-Id: Ie74cb70af33110aa09022d4dbb5c6d0069b6cc7c
Reviewed-on: https://chromium-review.googlesource.com/c/1311080
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605543}
[modify] https://crrev.com/e0e48885d523be60a79913ae9e0e122217188deb/ui/aura/window_tree_host.cc
[modify] https://crrev.com/e0e48885d523be60a79913ae9e0e122217188deb/ui/aura/window_tree_host.h
[modify] https://crrev.com/e0e48885d523be60a79913ae9e0e122217188deb/ui/views/mus/desktop_window_tree_host_mus.cc

Comment 3 by msw@chromium.org, Nov 6

Labels: Merge-Request-70 Merge-Request-71
Requesting merge of the fix in comment #2 for M-70 and M-71
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 6 

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

commit e2efb1befa172205b2de0bd0abe1e0d29b54925f
Author: Mike Wasserman <msw@chromium.org>
Date: Tue Nov 06 07:27:03 2018

ws: Add a DesktopWindowTreeHostMus test for setting bounds during init

Adds a unit test split out from https://crrev.com/c/1311080 (PS9)
Landing separately to make release branch merging easier.

Bug:  899084 
Test: Automated
Change-Id: Icadd82f2490bc4f3bbbdd1c73f8cdbe584f03289
Reviewed-on: https://chromium-review.googlesource.com/c/1318363
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605623}
[modify] https://crrev.com/e2efb1befa172205b2de0bd0abe1e0d29b54925f/ui/aura/test/mus/test_window_tree.cc
[modify] https://crrev.com/e2efb1befa172205b2de0bd0abe1e0d29b54925f/ui/aura/test/mus/test_window_tree.h
[modify] https://crrev.com/e2efb1befa172205b2de0bd0abe1e0d29b54925f/ui/views/mus/desktop_window_tree_host_mus_unittest.cc

Comment 5 by geo...@google.com, Nov 7

Labels: -Merge-Request-70 -Merge-Request-71 Merge-Rejected-70 Merge-Approved-71
Approved for M71 ChromeOS.

Rejecting for M70 as we have already rolled out M70 Stable. If this is critical for M70, please add justification and reapply M70 merge request. Thanks.
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 7

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25883ff3a49a2777e53788bde64f8a32b57ecee4

commit 25883ff3a49a2777e53788bde64f8a32b57ecee4
Author: Mike Wasserman <msw@chromium.org>
Date: Wed Nov 07 01:50:45 2018

ws: Fix scaling in DesktopWindowTreeHostMus during Init

Initialize the cached display device scale factor before usage.
Otherwise, the scale factor is 1 and the window is misplaced/sized.

I'll land a unit test separately, to make merging easier.

Bug:  899084 , 665965
Test: KSV (Ctrl-Alt-/) search context menu supports display & UI scales.
Change-Id: Ie74cb70af33110aa09022d4dbb5c6d0069b6cc7c
Reviewed-on: https://chromium-review.googlesource.com/c/1311080
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#605543}(cherry picked from commit e0e48885d523be60a79913ae9e0e122217188deb)
Reviewed-on: https://chromium-review.googlesource.com/c/1321867
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#556}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/25883ff3a49a2777e53788bde64f8a32b57ecee4/ui/aura/window_tree_host.cc
[modify] https://crrev.com/25883ff3a49a2777e53788bde64f8a32b57ecee4/ui/aura/window_tree_host.h
[modify] https://crrev.com/25883ff3a49a2777e53788bde64f8a32b57ecee4/ui/views/mus/desktop_window_tree_host_mus.cc
Project Member

Comment 7 by cr-audit...@appspot.gserviceaccount.com, Nov 7

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/25883ff3a49a2777e53788bde64f8a32b57ecee4

Commit: 25883ff3a49a2777e53788bde64f8a32b57ecee4
Author: msw@chromium.org
Commiter: msw@chromium.org
Date: 2018-11-07 01:50:45 +0000 UTC

ws: Fix scaling in DesktopWindowTreeHostMus during Init

Initialize the cached display device scale factor before usage.
Otherwise, the scale factor is 1 and the window is misplaced/sized.

I'll land a unit test separately, to make merging easier.

Bug:  899084 , 665965
Test: KSV (Ctrl-Alt-/) search context menu supports display & UI scales.
Change-Id: Ie74cb70af33110aa09022d4dbb5c6d0069b6cc7c
Reviewed-on: https://chromium-review.googlesource.com/c/1311080
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#605543}(cherry picked from commit e0e48885d523be60a79913ae9e0e122217188deb)
Reviewed-on: https://chromium-review.googlesource.com/c/1321867
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#556}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Comment 8 by msw@chromium.org, Nov 7

Status: Fixed (was: Started)
Okay, it's a bummer we missed this before M-70 stable, but it's not so severe that I'd push back.
Please help verify that this is fixed on the next release build of M-71, thanks.
►

Sign in to add a comment