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

Issue 760492 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression


Participants' hotlists:
Fixing-touch


Sign in to add a comment

Regression: Glimpse of shelf is seen on the virtual keyboard while opening

Project Member Reported by rkalavakuntla@chromium.org, Aug 30 2017

Issue description

Chrome Version:62.0.3199.0/9892.0.0 dev-channel Daisy,Kip,Minnie
OS:Chrome OS

What steps will reproduce the problem?
(1)Sign in to user >> Launch Chrome and Open chrome://settings/languages >>Input method
(2)Turn On the toggle button of 'Show input options in the shelf'
(3)In Input methods overlay, click on Emoji palette and Observe(Kindly refer video) 

Actual:Glimpse of shelf is seen while opening the virtual keyboard
Expected: No such shelf glimpse should be seen while opening the virtual keyboard

This is a Regression issue as same is working fine in 62.0.3198.0/9887.0.0 dev channel Minnie

Note: Issue is not seen in Linux,Windows OS


 
Actual.webm
1.1 MB View Download
Expected.webm
656 KB View Download
Owner: oka@chromium.org
Status: Assigned (was: Untriaged)
This might happen in M-61.

Comment 3 by oka@chromium.org, Sep 4 2017

Status: Started (was: Assigned)

Comment 4 by oka@chromium.org, Sep 5 2017

OK. It's ShelfLayoutManager::OnKeyboardBoundsChanging, which is moving the shelf position. https://cs.chromium.org/chromium/src/ash/shelf/shelf_layout_manager.cc?type=cs&sq=package:chromium&l=464
Before the change to SHOWING status, OnKeyboardBoundsChanging event was sent after animation finished, but after the change, it happens before animation. Thus glimpse of shelf is seen.

Actually I'm not sure if ShelfLayoutManager::OnKeyboardBoundsChanging needed any longer.

I'm going to just remove this if there's no objection.

Comment 5 by oka@chromium.org, Sep 13 2017

I experimented #4, and several test failed. I investigate what's going wrong.

ash_unittests:
ShelfLayoutManagerKeyboardTest.ShelfChangeWorkAreaInNonStickyMode
AshPopupAlignmentDelegateTest.KeyboardShowing
ShelfLayoutManagerKeyboardTest.ShelfIgnoreWorkAreaChangeInNonStickyMode
LockActionHandlerLayoutManagerTest.KeyboardBounds

browser_tests:
VirtualKeyboardAppWindowTest.DisableOverscrollForImeWindow


Comment 7 by oka@chromium.org, Sep 13 2017

Failure reasons:

ShelfLayoutManagerKeyboardTest.ShelfChangeWorkAreaInNonStickyMode - we can remove it.
AshPopupAlignmentDelegateTest.KeyboardShowing - notification should be show above the keyboard.
ShelfLayoutManagerKeyboardTest.ShelfIgnoreWorkAreaChangeInNonStickyMode
- sticky mode should change the workspace size.
LockActionHandlerLayoutManagerTest.KeyboardBounds
- ditto

VirtualKeyboardAppWindowTest.DisableOverscrollForImeWindow
- ditto

Comment 8 by oka@chromium.org, Sep 13 2017

Memo: refactoring idea: Split ShelfIgnoreWorkAreaChangeInNonStickyMode, and have ShelfShouldChangeWorkAreaInStickyMode

Comment 9 by oka@chromium.org, Sep 13 2017

I think target_bounds->shelf_bounds_in_root is miscalculated in CalculateTargetBounds if keyboard is opening.

I think we don't have to change the value to subtract here.
https://cs.chromium.org/chromium/src/ash/shelf/shelf_layout_manager.cc?type=cs&sq=package:chromium&l=724
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 14 2017

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

commit 781e88b47dc0ae3dcdb28e78851942ff32b83506
Author: oka@chromium.org <oka@chromium.org>
Date: Thu Sep 14 01:16:57 2017

Refactoring: split a test into two.

Split ShelfIgnoreWorkAreaChangeInNonStikyMode and have
ShelfShouldChangeWorkAreaInStickyMode.

BUG= 760492 
TEST=try

Change-Id: I1c149469c5210712c5c4d1893b7e17dfb627fbb9
Reviewed-on: https://chromium-review.googlesource.com/664897
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Keigo Oka <oka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501826}
[modify] https://crrev.com/781e88b47dc0ae3dcdb28e78851942ff32b83506/ash/shelf/shelf_layout_manager_unittest.cc

Comment 11 by oka@chromium.org, Sep 14 2017

I made the change with #9's approach.
https://chromium-review.googlesource.com/c/chromium/src/+/650233

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 18 2017

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

commit db5fa649f1af559c8704caf771ae2efebd8629d9
Author: oka@chromium.org <oka@chromium.org>
Date: Mon Sep 18 22:56:29 2017

Don't move the shelf position when virtual keyboard opens.

Bug:  760492 
Test: try
Change-Id: I77aabf9ded805cea1099bd1cfb7e6d91d92936a4
Reviewed-on: https://chromium-review.googlesource.com/650233
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Keigo Oka <oka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502705}
[modify] https://crrev.com/db5fa649f1af559c8704caf771ae2efebd8629d9/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/db5fa649f1af559c8704caf771ae2efebd8629d9/ash/shelf/shelf_layout_manager_unittest.cc

Comment 13 by oka@chromium.org, Sep 18 2017

Labels: Merge-Request-61
Status: Fixed (was: Started)

Comment 14 by oka@chromium.org, Sep 18 2017

Status: Assigned (was: Fixed)
Labels: Merge-Approved-62
This needs to be merged into 62 before we consider 61. 
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 20 2017

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

commit ab4d32f375cc21141ea27630535e9875c8894a2f
Author: oka@chromium.org <oka@chromium.org>
Date: Wed Sep 20 01:45:42 2017

Don't move the shelf position when virtual keyboard opens.

TBR=oka@chromium.org

(cherry picked from commit db5fa649f1af559c8704caf771ae2efebd8629d9)

Bug:  760492 
Test: try
Change-Id: I77aabf9ded805cea1099bd1cfb7e6d91d92936a4
Reviewed-on: https://chromium-review.googlesource.com/650233
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Keigo Oka <oka@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502705}
Reviewed-on: https://chromium-review.googlesource.com/674363
Reviewed-by: Keigo Oka <oka@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#343}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/ab4d32f375cc21141ea27630535e9875c8894a2f/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/ab4d32f375cc21141ea27630535e9875c8894a2f/ash/shelf/shelf_layout_manager_unittest.cc

Comment 17 by oka@chromium.org, Sep 26 2017

bhthompson@
Can we merge this to M61?  It's marked as a stable blocker.
Labels: -Merge-Request-61 Merge-Approved-61
Approving merge to M61.
Project Member

Comment 19 by bugdroid1@chromium.org, Oct 3 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5bfd81b612dde2e9afb601000a8f0440bb580310

commit 5bfd81b612dde2e9afb601000a8f0440bb580310
Author: oka@chromium.org <oka@chromium.org>
Date: Tue Oct 03 22:39:19 2017

Don't move the shelf position when virtual keyboard opens.

TBR=oka@chromium.org

(cherry picked from commit db5fa649f1af559c8704caf771ae2efebd8629d9)

Bug:  760492 
Test: try
Change-Id: I77aabf9ded805cea1099bd1cfb7e6d91d92936a4
Reviewed-on: https://chromium-review.googlesource.com/650233
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Keigo Oka <oka@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502705}
Reviewed-on: https://chromium-review.googlesource.com/699270
Reviewed-by: Keigo Oka <oka@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1306}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/5bfd81b612dde2e9afb601000a8f0440bb580310/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/5bfd81b612dde2e9afb601000a8f0440bb580310/ash/shelf/shelf_layout_manager_unittest.cc

Comment 20 by oka@chromium.org, Oct 3 2017

Status: Fixed (was: Assigned)

Sign in to add a comment