Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549543002:
Clamp dialog bounds to be fully visible on the nearest display (Closed)
DescriptionClamp dialog bounds to be fully visible on the nearest display
If a displayed modal dialog is partially or entirely off-screen, the
user may be unable to interact with it or even be aware of its
presence. This change ensures that the dialog is moved to the
nearest display so that it is fully visible to the user.
BUG=668734
Committed: https://crrev.com/51116b9a03e97df9962af7b22e0f3661dbd6efb3
Cr-Commit-Position: refs/heads/master@{#439127}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixed a nit #Patch Set 3 : Moved MoveScreenRectToNearestDisplay() back to constrained window #Patch Set 4 : Addressed all the feedback #
Total comments: 4
Patch Set 5 : Moved code inline and incorporated suggestions #Patch Set 6 : Added unittest for clamping dialog #
Total comments: 4
Patch Set 7 : Better unit test #
Total comments: 4
Patch Set 8 : Make the dialog smaller than the display #
Messages
Total messages: 48 (20 generated)
Description was changed from ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ========== to ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ==========
kylixrd@chromium.org changed reviewers: + gbillock@chromium.org, marcheu@chromium.org
Description was changed from ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ========== to ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ==========
Description was changed from ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ========== to ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ==========
kylixrd@chromium.org changed reviewers: + msw@chromium.org - gbillock@chromium.org
marcheu@ - please review/verify new display::Screen::MoveScreenRectToNearestDisplay() method msw@ - please validate use of the aforementioned method in UpdateModalDialogPosition. Thanks.
https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode38 ui/display/screen.cc:38: //static nit: space after slashes https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode39 ui/display/screen.cc:39: gfx::Rect Screen::MoveScreenRectToNearestDisplay( Will this be used elsewhere? Might it make sense to inline this in UpdateModalDialogPosition, or move it as a helper to constrained_window_views.cc? https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode47 ui/display/screen.cc:47: if (!work_area.Contains(display_rect)) { q: do we care about making minimal changes to the position? otherwise, would it make sense to use gfx::Rect::ClampToCenteredSize or similar?
https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode39 ui/display/screen.cc:39: gfx::Rect Screen::MoveScreenRectToNearestDisplay( On 2016/12/12 23:04:35, msw wrote: > Will this be used elsewhere? Might it make sense to inline this in > UpdateModalDialogPosition, or move it as a helper to > constrained_window_views.cc? I'd originally put this code in that file but this function should be universally useful. I don't have any immediate example at this point. I spent a while looking for a function such as this. The logical location was on the screen/display classes. Should this be put back into constrained_window_views.cc, the next person may not locate it there and proceed to write a similar function. By placing this where it logically belongs, it increases the chance it will be used by the next person needing such a function. https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode47 ui/display/screen.cc:47: if (!work_area.Contains(display_rect)) { On 2016/12/12 23:04:35, msw wrote: > q: do we care about making minimal changes to the position? otherwise, would it > make sense to use gfx::Rect::ClampToCenteredSize or similar? If the rect were partially visible, it would be somewhat jarring to the user for it to "jump" to the center of the screen.
Can you add some tests (for constrained windows)? https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode38 ui/display/screen.cc:38: //static On 2016/12/12 23:04:35, msw wrote: > nit: space after slashes Ping https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode39 ui/display/screen.cc:39: gfx::Rect Screen::MoveScreenRectToNearestDisplay( On 2016/12/13 14:17:44, kylix_rd wrote: > On 2016/12/12 23:04:35, msw wrote: > > Will this be used elsewhere? Might it make sense to inline this in > > UpdateModalDialogPosition, or move it as a helper to > > constrained_window_views.cc? > > I'd originally put this code in that file but this function should be > universally useful. I don't have any immediate example at this point. > > I spent a while looking for a function such as this. The logical location was on > the screen/display classes. Should this be put back into > constrained_window_views.cc, the next person may not locate it there and proceed > to write a similar function. By placing this where it logically belongs, it > increases the chance it will be used by the next person needing such a function. > I'm also a little surprised that I can't find a similar function. Still, this doesn't belong here without another existing or imminent user (unless a ui/display owner disagrees). https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode47 ui/display/screen.cc:47: if (!work_area.Contains(display_rect)) { On 2016/12/13 14:17:44, kylix_rd wrote: > On 2016/12/12 23:04:35, msw wrote: > > q: do we care about making minimal changes to the position? otherwise, would > it > > make sense to use gfx::Rect::ClampToCenteredSize or similar? > > If the rect were partially visible, it would be somewhat jarring to the user for > it to "jump" to the center of the screen. Hopefully this positioning fixup is applied before the dialog is actually painted, so it never actually renders partially offscreen then 'jumps'; can you confirm that? Regardless, it's fine to minimize the positioning delta, but this code doesn't handle (center) rects larger than the work area in either dimension, yet another reason this shouldn't be a generalized function. https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode50 ui/display/screen.cc:50: if (display_rect.right() > work_area.right()) else if? (or handle rects larger than the work area?) https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode54 ui/display/screen.cc:54: if (display_rect.bottom() > work_area.bottom()) else if? (or handle rects larger than the work area?)
Tests will be forthcoming. https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode38 ui/display/screen.cc:38: //static On 2016/12/13 22:52:43, msw wrote: > On 2016/12/12 23:04:35, msw wrote: > > nit: space after slashes > > Ping Done. https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode39 ui/display/screen.cc:39: gfx::Rect Screen::MoveScreenRectToNearestDisplay( On 2016/12/13 22:52:43, msw wrote: > On 2016/12/13 14:17:44, kylix_rd wrote: > > On 2016/12/12 23:04:35, msw wrote: > > > Will this be used elsewhere? Might it make sense to inline this in > > > UpdateModalDialogPosition, or move it as a helper to > > > constrained_window_views.cc? > > > > I'd originally put this code in that file but this function should be > > universally useful. I don't have any immediate example at this point. > > > > I spent a while looking for a function such as this. The logical location was > on > > the screen/display classes. Should this be put back into > > constrained_window_views.cc, the next person may not locate it there and > proceed > > to write a similar function. By placing this where it logically belongs, it > > increases the chance it will be used by the next person needing such a > function. > > > > I'm also a little surprised that I can't find a similar function. Still, this > doesn't belong here without another existing or imminent user (unless a > ui/display owner disagrees). My point was that should this be put elsewhere, it would increase the likelihood of it being missed and the same or similar function being written again. I think we should at least try and make things a little easier on those that come in behind us. I also expect this function to be used in the near future and the extra code churn seems a little excessive. In the end, I'm not going to "go to the mat" for this :)... I'm just trying to state my case. https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode47 ui/display/screen.cc:47: if (!work_area.Contains(display_rect)) { On 2016/12/13 22:52:44, msw wrote: > On 2016/12/13 14:17:44, kylix_rd wrote: > > On 2016/12/12 23:04:35, msw wrote: > > > q: do we care about making minimal changes to the position? otherwise, would > > it > > > make sense to use gfx::Rect::ClampToCenteredSize or similar? > > > > If the rect were partially visible, it would be somewhat jarring to the user > for > > it to "jump" to the center of the screen. > > Hopefully this positioning fixup is applied before the dialog is actually > painted, so it never actually renders partially offscreen then 'jumps'; can you > confirm that? Regardless, it's fine to minimize the positioning delta, but this > code doesn't handle (center) rects larger than the work area in either > dimension, yet another reason this shouldn't be a generalized function. Yes, this is done prior to the dialog is initially made visible. So, are you saying that the size should also be clamped to the screen size as well? IMO, that would be a different operation. The name of the function is *Move*ScreenRectToNearestDisplay, so it shouldn't be confusing. I could add a parameter to this function or add a different function to do that. https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode50 ui/display/screen.cc:50: if (display_rect.right() > work_area.right()) On 2016/12/13 22:52:44, msw wrote: > else if? (or handle rects larger than the work area?) See above comment.
https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode39 ui/display/screen.cc:39: gfx::Rect Screen::MoveScreenRectToNearestDisplay( On 2016/12/14 17:35:10, kylix_rd wrote: > On 2016/12/13 22:52:43, msw wrote: > > On 2016/12/13 14:17:44, kylix_rd wrote: > > > On 2016/12/12 23:04:35, msw wrote: > > > > Will this be used elsewhere? Might it make sense to inline this in > > > > UpdateModalDialogPosition, or move it as a helper to > > > > constrained_window_views.cc? > > > > > > I'd originally put this code in that file but this function should be > > > universally useful. I don't have any immediate example at this point. > > > > > > I spent a while looking for a function such as this. The logical location > was > > on > > > the screen/display classes. Should this be put back into > > > constrained_window_views.cc, the next person may not locate it there and > > proceed > > > to write a similar function. By placing this where it logically belongs, it > > > increases the chance it will be used by the next person needing such a > > function. > > > > > > > I'm also a little surprised that I can't find a similar function. Still, this > > doesn't belong here without another existing or imminent user (unless a > > ui/display owner disagrees). > > My point was that should this be put elsewhere, it would increase the likelihood > of it being missed and the same or similar function being written again. I think > we should at least try and make things a little easier on those that come in > behind us. > > I also expect this function to be used in the near future and the extra code > churn seems a little excessive. > > In the end, I'm not going to "go to the mat" for this :)... I'm just trying to > state my case. Finding code to reuse later isn't too hard. I searched a little more and found a few related functions, and I encourage you to extract some common functionality: AppWindow::AdjustBoundsToBeVisibleOnScreen, GetWindowBoundsForClientBounds (in ui/platform_window/win/win_window.cc), BubbleFrameView::OffsetArrowIfOffScreen, hopefully you can find even more. I'd defer to the advice of a ui/display owner, but in general, we try to avoid pushing client-specific code to widely used/included base libraries. This operation can be performed with the publicly-accessible display/screen interfaces, which strive to strike a balance between minimalism and universal applicability. This operation is not an intrinsic/essential part of display::Screen, it's more fitting in a display/screen/window utility file of grab-bag helpers, and I'd only suggest that if the function had sufficient complexity or applicability to more than one user (otherwise, it belongs with its solitary client). https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode47 ui/display/screen.cc:47: if (!work_area.Contains(display_rect)) { On 2016/12/14 17:35:10, kylix_rd wrote: > On 2016/12/13 22:52:44, msw wrote: > > On 2016/12/13 14:17:44, kylix_rd wrote: > > > On 2016/12/12 23:04:35, msw wrote: > > > > q: do we care about making minimal changes to the position? otherwise, > would > > > it > > > > make sense to use gfx::Rect::ClampToCenteredSize or similar? > > > > > > If the rect were partially visible, it would be somewhat jarring to the user > > for > > > it to "jump" to the center of the screen. > > > > Hopefully this positioning fixup is applied before the dialog is actually > > painted, so it never actually renders partially offscreen then 'jumps'; can > you > > confirm that? Regardless, it's fine to minimize the positioning delta, but > this > > code doesn't handle (center) rects larger than the work area in either > > dimension, yet another reason this shouldn't be a generalized function. > > Yes, this is done prior to the dialog is initially made visible. > > So, are you saying that the size should also be clamped to the screen size as > well? IMO, that would be a different operation. The name of the function is > *Move*ScreenRectToNearestDisplay, so it shouldn't be confusing. I could add a > parameter to this function or add a different function to do that. I'm saying that if a window were larger than the display's work area, this function would clamp the rect such that the leading (left or top) edge would be offscreen, rather than centering or shrinking the rect within the work area. Even if we didn't want to center/shrink the rect, I'd imagine that we'd want to show the leading/top edge (it's more likely to have the window controls on top, but perhaps dialog response buttons are on the right so show the trailing horizontal edge, which depends upon the RTL/LTR system locale, etc.). These kinds of considerations are especially important if you want to make this a generalized, widely-applicable solution.
Apparently Chrome OS already clamps dialogs onscreen, where's that code? https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode44 ui/display/screen.cc:44: display_rect.CenterPoint()); It might make sense to use the display closest to the center point of the parent window, rather than the new dialog, though they're probably often the same thing (unless a window is split vertically) https://codereview.chromium.org/2549543002/diff/1/ui/display/screen.cc#newcode47 ui/display/screen.cc:47: if (!work_area.Contains(display_rect)) { On 2016/12/14 18:22:06, msw wrote: > On 2016/12/14 17:35:10, kylix_rd wrote: > > On 2016/12/13 22:52:44, msw wrote: > > > On 2016/12/13 14:17:44, kylix_rd wrote: > > > > On 2016/12/12 23:04:35, msw wrote: > > > > > q: do we care about making minimal changes to the position? otherwise, > > would > > > > it > > > > > make sense to use gfx::Rect::ClampToCenteredSize or similar? > > > > > > > > If the rect were partially visible, it would be somewhat jarring to the > user > > > for > > > > it to "jump" to the center of the screen. > > > > > > Hopefully this positioning fixup is applied before the dialog is actually > > > painted, so it never actually renders partially offscreen then 'jumps'; can > > you > > > confirm that? Regardless, it's fine to minimize the positioning delta, but > > this > > > code doesn't handle (center) rects larger than the work area in either > > > dimension, yet another reason this shouldn't be a generalized function. > > > > Yes, this is done prior to the dialog is initially made visible. > > > > So, are you saying that the size should also be clamped to the screen size as > > well? IMO, that would be a different operation. The name of the function is > > *Move*ScreenRectToNearestDisplay, so it shouldn't be confusing. I could add a > > parameter to this function or add a different function to do that. > > I'm saying that if a window were larger than the display's work area, this > function would clamp the rect such that the leading (left or top) edge would be > offscreen, rather than centering or shrinking the rect within the work area. > Even if we didn't want to center/shrink the rect, I'd imagine that we'd want to > show the leading/top edge (it's more likely to have the window controls on top, > but perhaps dialog response buttons are on the right so show the trailing > horizontal edge, which depends upon the RTL/LTR system locale, etc.). These > kinds of considerations are especially important if you want to make this a > generalized, widely-applicable solution. Perhaps the right function for this job is gfx::Rect::AdjustToFit; other functions I pointed out could probably use that too.
On 2016/12/14 18:45:12, msw wrote: > Apparently Chrome OS already clamps dialogs onscreen, where's that code? I thought that as well. In fact I was a more than a little perplexed that it happened. However in the system modal case described in the linked bug it can clearly be displayed off screen and the parent window will not respond to being moved. There are other instances, where the dialog is tab-modal and the top-level Chrome window can still be dragged back on screen, including the dialog. As for where this code should live, I'm coming around to your point. I'll move the function back and look around a little more. I just needed to figure out the right terms to search for.
Patchset #3 (id:40001) has been deleted
kylixrd@chromium.org changed reviewers: - marcheu@chromium.org
Please address my comments regarding using the parent's bounds, using Rect::AdjustToFit, and please consider changing the function return type if only the origin is needed.
This should, hopefully, address your concerns. It's using the host widget's center point to determine the display to use. It is also using Rect::AdjustToFit instead of it's own code.
This seems much better; thanks. https://codereview.chromium.org/2549543002/diff/80001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2549543002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:86: gfx::Point MoveRectToNearestHostDisplay(const gfx::Rect& screen_rect, Inline this function or rename it, since it doesn't actually move (mutate) a rect (eg. GetOriginForRectToFitOnDisplay or similar). https://codereview.chromium.org/2549543002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:89: const display::Display* display = display::FindDisplayNearestPoint( Use Screen::GetDisplayNearestWindow(dialog_host->GetHostView()). https://codereview.chromium.org/2549543002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:91: DCHECK(display); optional nit: DCHECK immediately before deference isn't necessary. https://codereview.chromium.org/2549543002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:128: // If the dialog extends partially off any display, clamp its position to This should be a function comment on the helper (unless you inline it).
Patchset #5 (id:100001) has been deleted
kylixrd@chromium.org changed reviewers: + derat@chromium.org
msw@ review incorporated suggestions derat@ please verify addition of +ui/display to DEPS
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
DEPS lgtm. can you add tests for this to the _unittest.cc file?
The code lgtm, I can take another pass for unit tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Unit test added.
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2549543002/diff/140001/components/constrained... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2549543002/diff/140001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:245: const gfx::Rect bounds = display.bounds(); use Rect::Union (or skip and just assert that there's only one display) https://codereview.chromium.org/2549543002/diff/140001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:261: // Precalculate the expected location of the dialog. just check that the one display's bounds contains the dialog bounds? Otherwise, this test falls into the "change detector" trap of duplicating the production code and checking that both yield the same behavior, versus checking the basic important behavior (ie. even if the host if off-screen, the dialog will be shown completely on-screen)
Update test based on feedback. It now only ensures that the dialog is, in fact, moved to intersect with the display. Based on my reading of the harness, the screen and displays are synthesized such that there is one display, which appears to be 800x600. This is regardless on what system the tests are run. The test now asserts that there is only 1 display. https://codereview.chromium.org/2549543002/diff/140001/components/constrained... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2549543002/diff/140001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:245: const gfx::Rect bounds = display.bounds(); On 2016/12/15 20:46:09, msw wrote: > use Rect::Union (or skip and just assert that there's only one display) Done. https://codereview.chromium.org/2549543002/diff/140001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:261: // Precalculate the expected location of the dialog. On 2016/12/15 20:46:09, msw wrote: > just check that the one display's bounds contains the dialog bounds? Otherwise, > this test falls into the "change detector" trap of duplicating the production > code and checking that both yield the same behavior, versus checking the basic > important behavior (ie. even if the host if off-screen, the dialog will be shown > completely on-screen) Yes, that code seemed a little silly... I've reworked. Next patch.
This looks pretty good, just one question, hoping that we can do a more rigorous bounds check using Rect::Contains. https://codereview.chromium.org/2549543002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2549543002/diff/160001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:262: // visible. Due to the constraints set, it may extend off the display. Can you explain what constraints you're referring to here? It seems like we should expect the display's work area to contain the dialog bounds, unless the dialog is bigger than the work area? (Or they share an edge and that doesn't count as 'contains'...)
https://codereview.chromium.org/2549543002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2549543002/diff/160001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:262: // visible. Due to the constraints set, it may extend off the display. On 2016/12/15 21:44:56, msw wrote: > Can you explain what constraints you're referring to here? It seems like we > should expect the display's work area to contain the dialog bounds, unless the > dialog is bigger than the work area? (Or they share an edge and that doesn't > count as 'contains'...) The default dialog constraints (min/max size) setup in the surrounding testing class. The min size is slightly larger, vertically, than the display.
https://codereview.chromium.org/2549543002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2549543002/diff/160001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:262: // visible. Due to the constraints set, it may extend off the display. On 2016/12/15 21:57:56, kylix_rd wrote: > On 2016/12/15 21:44:56, msw wrote: > > Can you explain what constraints you're referring to here? It seems like we > > should expect the display's work area to contain the dialog bounds, unless the > > dialog is bigger than the work area? (Or they share an edge and that doesn't > > count as 'contains'...) > > The default dialog constraints (min/max size) setup in the surrounding testing > class. The min size is slightly larger, vertically, than the display. It's a little silly that the test's SetUp makes the dialog too big to fit on the default test display... Anyway, in this specific test fixture, can we do something like this to ensure the bounds are contained? // Set a small preferred size that fits in the display's work area. contents()->set_preferred_size(gfx::Size(200, 100)); ... EXPECT_TRUE(extents.Contains(dialog_bounds));
https://codereview.chromium.org/2549543002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2549543002/diff/160001/components/constrained... components/constrained_window/constrained_window_views_unittest.cc:262: // visible. Due to the constraints set, it may extend off the display. On 2016/12/15 22:17:40, msw wrote: > On 2016/12/15 21:57:56, kylix_rd wrote: > > On 2016/12/15 21:44:56, msw wrote: > > > Can you explain what constraints you're referring to here? It seems like we > > > should expect the display's work area to contain the dialog bounds, unless > the > > > dialog is bigger than the work area? (Or they share an edge and that doesn't > > > count as 'contains'...) > > > > The default dialog constraints (min/max size) setup in the surrounding testing > > class. The min size is slightly larger, vertically, than the display. > > It's a little silly that the test's SetUp makes the dialog too big to fit on the > default test display... Anyway, in this specific test fixture, can we do > something like this to ensure the bounds are contained? > // Set a small preferred size that fits in the display's work area. > contents()->set_preferred_size(gfx::Size(200, 100)); > ... > EXPECT_TRUE(extents.Contains(dialog_bounds)); I agree. However, out of the abundance of caution, I didn't make that kind of change in case there were some other oddity I'd missed. I'll verify that it is indeed only that change that is needed.
Setting the preferred size of the dialog contents works. Thanks for the tip.
lgtm
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2549543002/#ps180001 (title: "Make the dialog smaller than the display")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481903108848980,
"parent_rev": "e008376e033edc4a10dae30f7055eee95911f0bc", "commit_rev":
"6e25d168dddbb7dad13a39fc0b3e930c705abd8b"}
Message was sent while issue was closed.
Description was changed from ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 ========== to ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 Review-Url: https://codereview.chromium.org/2549543002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 Review-Url: https://codereview.chromium.org/2549543002 ========== to ========== Clamp dialog bounds to be fully visible on the nearest display If a displayed modal dialog is partially or entirely off-screen, the user may be unable to interact with it or even be aware of its presence. This change ensures that the dialog is moved to the nearest display so that it is fully visible to the user. BUG=668734 Committed: https://crrev.com/51116b9a03e97df9962af7b22e0f3661dbd6efb3 Cr-Commit-Position: refs/heads/master@{#439127} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/51116b9a03e97df9962af7b22e0f3661dbd6efb3 Cr-Commit-Position: refs/heads/master@{#439127} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
