AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window dimension is small

AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when status bubble (height 22) touches the bottom of work area. If we adjust bounds using bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted white space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different bug and will be dealt with later by filing another bug with it.
add unittest coverage

Committed: https://crrev.com/e17639909d925fcde4f0c3425c261d166036aa5d
Cr-Commit-Position: refs/heads/master@{#414782}

[Qiang(Joe) Xu, 2016-07-22 17:01:22]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

BUG=624806
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

There are two cases here:
(1) if user session is blocked by overlying UI, then ignore possible setting
display area caused by animation.
(2) In chromeos, user switch animator does TransitionWallpaper,
TransitionUserShelf, TransitionWindows in each animation step. The user shelf
transition may cause work area change anyway as (a) In
ANIMATION_STEP_HIDE_OLD_USER step, it will set shelf hidden (b) if two users
have different shelf alignment.

For case (1) add the check for IsUserSessionBlocked. For case (2) expose a
public method for reposition status bubble, and call it in
ANIMATION_STEP_FINALIZE step.

BUG=624806
==========

[Qiang(Joe) Xu, 2016-07-22 17:01:23]

warx@chromium.org changed reviewers:
+ skuhne@chromium.org, sky@chromium.org

[Qiang(Joe) Xu, 2016-07-22 17:02:10]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

There are two cases here:
(1) if user session is blocked by overlying UI, then ignore possible setting
display area caused by animation.
(2) In chromeos, user switch animator does TransitionWallpaper,
TransitionUserShelf, TransitionWindows in each animation step. The user shelf
transition may cause work area change anyway as (a) In
ANIMATION_STEP_HIDE_OLD_USER step, it will set shelf hidden (b) if two users
have different shelf alignment.

For case (1) add the check for IsUserSessionBlocked. For case (2) expose a
public method for reposition status bubble, and call it in
ANIMATION_STEP_FINALIZE step.

BUG=624806
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

There are two cases here:
(1) if user session is blocked by overlying UI, then ignore possible setting
display area caused by animation.
(2) In chromeos, user switch animator does TransitionWallpaper,
TransitionUserShelf, TransitionWindows in each animation step. The user shelf
transition may cause work area change anyway as (a) In
ANIMATION_STEP_HIDE_OLD_USER step, it will set shelf hidden (b) if two users
have different shelf alignment.

For case (1) add the check for IsUserSessionBlocked. For case (2) expose a
public method for reposition status bubble, and call it in
ANIMATION_STEP_FINALIZE step.

BUG=624806
TEST=Device test
==========

[Qiang(Joe) Xu, 2016-07-22 17:04:14]

Hi sky@, skuhne@, could you help review on this? tks.

[Qiang(Joe) Xu, 2016-07-22 18:28:46]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 18:29:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 19:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 19:33:58]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-25 15:23:40]

https://codereview.chromium.org/2175833002/diff/1/chrome/browser/ui/ash/multi...
File chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc (right):

https://codereview.chromium.org/2175833002/diff/1/chrome/browser/ui/ash/multi...
chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc:382: // The
user shelf transition animation may cause browser's status
Seems like you are working around a bug in the shelf. Why isn't the shelf
updating things correctly?

[Qiang(Joe) Xu, 2016-07-25 21:19:36]

https://codereview.chromium.org/2175833002/diff/1/chrome/browser/ui/ash/multi...
File chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc (right):

https://codereview.chromium.org/2175833002/diff/1/chrome/browser/ui/ash/multi...
chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc:382: // The
user shelf transition animation may cause browser's status
On 2016/07/25 15:23:40, sky wrote:
> Seems like you are working around a bug in the shelf. Why isn't the shelf
> updating things correctly?

Yes. The reason is shelf animation between transition users. This is one of the
solution, but not solving the root cause. I think showing new users' windows
should start after shelf transition animation finished. It needs more work to
do.

I don't like this solution actually. Let me work more on this.

[Mr4D (OOO till 08-26), 2016-08-15 20:58:45]

Just checking - is this patch still valid?

[Qiang(Joe) Xu, 2016-08-15 21:08:05]

On 2016/08/15 20:58:45, Mr4D wrote:
> Just checking - is this patch still valid?

No yet. I am recently working on other issues. May return on this when I have
time. Thanks!

[Qiang(Joe) Xu, 2016-08-23 03:57:49]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 03:58:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2016-08-23 04:04:18]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

There are two cases here:
(1) if user session is blocked by overlying UI, then ignore possible setting
display area caused by animation.
(2) In chromeos, user switch animator does TransitionWallpaper,
TransitionUserShelf, TransitionWindows in each animation step. The user shelf
transition may cause work area change anyway as (a) In
ANIMATION_STEP_HIDE_OLD_USER step, it will set shelf hidden (b) if two users
have different shelf alignment.

For case (1) add the check for IsUserSessionBlocked. For case (2) expose a
public method for reposition status bubble, and call it in
ANIMATION_STEP_FINALIZE step.

BUG=624806
TEST=Device test
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble views is set to ignored_by_shelf here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/status_bubble_vi...,

Shelf layout may cause the event: WM_EVENT_WORKAREA_BOUNDS_CHANGED, we should
add a check to avoid the bubble view's bounds change animation.

BUG=624806
TEST=Device test
==========

[commit-bot: I haz the power, 2016-08-23 04:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 04:37:23]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2016-08-23 16:51:14]

Hi All, here comes the update which is described in description. I think now
only one ash/ owner review is needed. Thanks!

[sky, 2016-08-23 17:30:25]

Nice! How about test coverage?

[Qiang(Joe) Xu, 2016-08-24 00:01:36]

On 2016/08/23 17:30:25, sky wrote:
> Nice! How about test coverage?

Hi sky, I think I can add a test here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/shelf_browsertest....

Just want to know how could we check the status bubble's position since
https://cs.chromium.org/chromium/src/chrome/browser/ui/status_bubble.h?sq=pac...
doesn't expose the method to check that. Should we add an API in status_bubble.h
or is there any other way? Thanks!

[sky, 2016-08-24 02:08:13]

As the fix is entirely in ash I would expect the test coverage there.
It's ok for duplicate test coverage (if you so desire), but a focused
test is generally best.

  -Scott

On Tue, Aug 23, 2016 at 5:01 PM,  <warx@chromium.org> wrote:
> On 2016/08/23 17:30:25, sky wrote:
>> Nice! How about test coverage?
>
> Hi sky, I think I can add a test here:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/shelf_browsertest....
>
> Just want to know how could we check the status bubble's position since
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/status_bubble.h?sq=pac...
> doesn't expose the method to check that. Should we add an API in
> status_bubble.h
> or is there any other way? Thanks!
>
> https://codereview.chromium.org/2175833002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Qiang(Joe) Xu, 2016-08-24 23:59:21]

Hi Scott, unittest is added. I test one case which is the same root reason.

[sky, 2016-08-25 15:19:30]

sky@chromium.org changed reviewers:
+ oshima@chromium.org

[sky, 2016-08-25 15:19:30]

+oshima

https://codereview.chromium.org/2175833002/diff/60001/ash/common/wm/default_s...
File ash/common/wm/default_state.cc (right):

https://codereview.chromium.org/2175833002/diff/60001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:480: !window_state->ignored_by_shelf())
ignored_by_shelf means:

  // True if the window is ignored by the shelf layout manager for
  // purposes of darkening the shelf.

You're changing the meaning to include don't adjust bounds on workspace area
changed. I don't think ignored_by_shelf should mean these two things.

I see two options:
1. ash shouldn't change the bounds of anything on a resize. This way client code
can do what it wants and we don't have race conditions. 
2. Add a property with the meaning of what you want, don't resize on workspace
change.
3. Only run this code on windows of type WINDOW_TYPE_NORMAL.

3 seems right to me, but there may be subtlety.

[oshima, 2016-08-25 17:10:54]

On 2016/08/25 15:19:30, sky wrote:
> +oshima
> 
>
https://codereview.chromium.org/2175833002/diff/60001/ash/common/wm/default_s...
> File ash/common/wm/default_state.cc (right):
> 
>
https://codereview.chromium.org/2175833002/diff/60001/ash/common/wm/default_s...
> ash/common/wm/default_state.cc:480: !window_state->ignored_by_shelf())
> ignored_by_shelf means:
> 
>   // True if the window is ignored by the shelf layout manager for
>   // purposes of darkening the shelf.
> 
> You're changing the meaning to include don't adjust bounds on workspace area
> changed. I don't think ignored_by_shelf should mean these two things.
> 
> I see two options:
> 1. ash shouldn't change the bounds of anything on a resize. This way client
code
> can do what it wants and we don't have race conditions. fff
> 2. Add a property with the meaning of what you want, don't resize on workspace
> change.
> 3. Only run this code on windows of type WINDOW_TYPE_NORMAL.
> 
> 3 seems right to me, but there may be subtlety.

I looked at the bug, it sounds lie the shelf layout manager is computing the
work area using animating bounds, instead of target bounds?
Can you look check if the different work area is applied to adjust the bounds?

[Qiang(Joe) Xu, 2016-08-26 00:30:05]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble views is set to ignored_by_shelf here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/status_bubble_vi...,

Shelf layout may cause the event: WM_EVENT_WORKAREA_BOUNDS_CHANGED, we should
add a check to avoid the bubble view's bounds change animation.

BUG=624806
TEST=Device test
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

BUG=624806
TEST=Device test
==========

[Qiang(Joe) Xu, 2016-08-26 00:31:47]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 00:33:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2016-08-26 00:38:51]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

BUG=624806
TEST=Device test
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

BUG=624806
TEST=Device test
add unittest
==========

[Qiang(Joe) Xu, 2016-08-26 00:39:04]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

BUG=624806
TEST=Device test
add unittest
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

BUG=624806
TEST=Device test
add unittest coverage
==========

[Qiang(Joe) Xu, 2016-08-26 00:51:23]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

BUG=624806
TEST=Device test
add unittest coverage
==========

to

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the screen bottom.

BUG=624806
TEST=Device test
add unittest coverage
==========

[commit-bot: I haz the power, 2016-08-26 01:18:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 01:18:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2016-08-26 05:12:25]

Patchset #5 (id:80001) has been deleted

[Qiang(Joe) Xu, 2016-08-26 05:15:26]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 05:15:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2016-08-26 05:18:49]

Description was changed from

==========
Status bubble is vertically offset while user session is blocked and switching
users

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the screen bottom.

BUG=624806
TEST=Device test
add unittest coverage
==========

to

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the screen bottom. If we adjust bounds using
bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted white
space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage
==========

[Qiang(Joe) Xu, 2016-08-26 05:19:27]

Description was changed from

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the screen bottom. If we adjust bounds using
bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted white
space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage
==========

to

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the bottom of work area. If we adjust bounds
using bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted
white space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage
==========

[commit-bot: I haz the power, 2016-08-26 06:02:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 06:02:04]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2016-08-26 17:01:41]

New patch is part of the fix in the crbug.com/624806.
AdjustBoundsToEnsureMinimumWindowVisibility may cause problem when window
dimension is small and locates near the bounds of work area. Per discussion with
oshima@, comment 8 in the bug link will be filed as new bug. Thanks!

[oshima, 2016-08-26 17:04:15]

lgtm

[Mr4D (OOO till 08-26), 2016-08-26 17:57:55]

lgtm

[sky, 2016-08-26 19:32:58]

LGTM

[Qiang(Joe) Xu, 2016-08-26 19:34:49]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2016-08-26 19:35:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 19:41:41]

Description was changed from

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the bottom of work area. If we adjust bounds
using bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted
white space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage
==========

to

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the bottom of work area. If we adjust bounds
using bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted
white space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage
==========

[commit-bot: I haz the power, 2016-08-26 19:41:42]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-26 19:45:14]

Description was changed from

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the bottom of work area. If we adjust bounds
using bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted
white space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage
==========

to

==========
AdjustBoundsToEnsureMinimumWindowVisibility may cause unwanted shift if window
dimension is small

Status bubble height may be smaller than kMinimumOnScreenArea, which is 25. The
current logic in AdjustBoundsToEnsureWindowVisibility fails in this case.

For example, bounds->y() > visible_area.bottom() - min_height is satisfied when
status bubble (height 22) touches the bottom of work area. If we adjust bounds
using bounds->set_y(visible_area.bottom() - min_height), it will cause unwanted
white space.

BUG=624806
TEST=Device test, fixing the reporter's bug. The bug in comment 8 is a different
bug and will be dealt with later by filing another bug with it.
add unittest coverage

Committed: https://crrev.com/e17639909d925fcde4f0c3425c261d166036aa5d
Cr-Commit-Position: refs/heads/master@{#414782}
==========

[commit-bot: I haz the power, 2016-08-26 19:45:15]

Patchset 5 (id:??) landed as
https://crrev.com/e17639909d925fcde4f0c3425c261d166036aa5d
Cr-Commit-Position: refs/heads/master@{#414782}