Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151903002:
Exclude caption button bounds from non-client frame hit testing (Closed)
|
Created:
4 years, 5 months ago by kylix_rd Modified:
4 years, 5 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, Bret Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis ensures that the non-client frame hit testing doesn't extend down into the caption button (minimize, maximize, close) area, thus giving the user the impression that the frame is resizable at that point.
BUG=548619
Committed: https://crrev.com/5db27ec315b8568a001a10148fb751867211e48e
Cr-Commit-Position: refs/heads/master@{#406651}
Patch Set 1 #Patch Set 2 : Slightly better code. Don't scale the small inset #
Total comments: 16
Patch Set 3 : Comment cleanup and minor code changes #Patch Set 4 : moved dwmapi.h above utility and use system search path #
Total comments: 8
Patch Set 5 : Better comments explaining the code, its purpose and its limitations #
Total comments: 1
Patch Set 6 : Even more clarity for the comments #Messages
Total messages: 26 (14 generated)
Description was changed from ========== Exclude caption button bounds from non-client frame hit testing BUG= ========== to ========== This ensures that the non-client frame hit testing doesn't extend down into the caption button (minimize, maximize, close) area, thus giving the user the impression that the frame is resizable at that point. BUG=548619 ==========
kylixrd@chromium.org changed reviewers: + pkasting@chromium.org
Both sky@ and pkasting@ are OOO, so this CL will sit for a while.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:24: #include "dwmapi.h" This should be in <> and moved up above the <utility> #include. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:76: // sizable frame "peeks" through. This will ensure that the behavior is Nit: sizable frame -> resize border I would omit the second sentence and do any necessary explanation thereof at the call site where this is used. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:255: // of the window. This code will ensure that the mouse isn't set to a size Nit: of -> corner of https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:260: const HWND hwnd = views::HWNDForWidget(frame()); Nit: I'd probably just inline this into the next statement https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:261: Nit: No blank here https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:267: // windows. Nit: This comment really belongs on the Inset() call. We should probably give more detail about how this works, e.g. does Windows normally include the resize border portion within the caption button bounds, and is it 1 px regardless of scale factor? The latter question also relates to my next comment. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:270: buttons.Inset(0, kCaptionButtonTopInset, 0, 0); Is it correct to inset after scaling? It's hard for me to work out the effects of different choices on different scale factors. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:272: return (HTNOWHERE); Should this be HTCAPTION? Nit: No parens
https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:24: #include "dwmapi.h" On 2016/07/18 19:12:15, Peter Kasting (slow) wrote: > This should be in <> and moved up above the <utility> #include. Done. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:76: // sizable frame "peeks" through. This will ensure that the behavior is On 2016/07/18 19:12:16, Peter Kasting (slow) wrote: > Nit: sizable frame -> resize border > > I would omit the second sentence and do any necessary explanation thereof at the > call site where this is used. Done. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:255: // of the window. This code will ensure that the mouse isn't set to a size On 2016/07/18 19:12:15, Peter Kasting (slow) wrote: > Nit: of -> corner of Done. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:260: const HWND hwnd = views::HWNDForWidget(frame()); On 2016/07/18 19:12:15, Peter Kasting (slow) wrote: > Nit: I'd probably just inline this into the next statement Done. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:261: On 2016/07/18 19:12:16, Peter Kasting (slow) wrote: > Nit: No blank here Done. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:267: // windows. On 2016/07/18 19:12:15, Peter Kasting (slow) wrote: > Nit: This comment really belongs on the Inset() call. We should probably give > more detail about how this works, e.g. does Windows normally include the resize > border portion within the caption button bounds, and is it 1 px regardless of > scale factor? The latter question also relates to my next comment. Done. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:270: buttons.Inset(0, kCaptionButtonTopInset, 0, 0); On 2016/07/18 19:12:16, Peter Kasting (slow) wrote: > Is it correct to inset after scaling? It's hard for me to work out the effects > of different choices on different scale factors. From my (admittedly empirical) investigation, it does indeed seem to be a constant 1px regardless of overall DSF. On my Surface Book set to 300% scaling, the area above the buttons which hit-test as a sizable region seems to still be 1px. https://codereview.chromium.org/2151903002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:272: return (HTNOWHERE); On 2016/07/18 19:12:16, Peter Kasting (slow) wrote: > Should this be HTCAPTION? > > Nit: No parens I want to ensure that subsequent processing lands at DefWindowProc which will do the OS-level hit-testing. Making this HTCAPTION may cause odd follow-on effects, such as disabling the caption buttons and make clicking on them move the window rather than their intended function.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:254: // corner of the window. This code will ensure that the mouse isn't set to a Tiny nit: will ensure that -> ensures https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:269: // inset after it is adjusted for the device scale. Wait, isn't this sort of backwards? If the region is always 1 px, then it seems like we'd need to inset before converting to DIP to match that. That would potentially have the problem that the 1 px region might get "lost" due to the lossy conversion to DIP... which suggests that we'd benefit from having the current position and the desired rect be a PointF and a RectF, respectively... ugh. I hate that views works in DIP everywhere instead of px. If all of the above is correct, then perhaps for now the code here is correct, but we should have a comment like "The resize region around the caption buttons is always 1 px. Since views works in DIP, the closest we can get to this while still ensuring there's always a resizable region is to inset after scaling. This may still display a resize cursor erroneously when the mouse isn't actually in the outermost pixel, which seems better than not displaying a resize cursor at all." Or is that not right either?
https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:254: // corner of the window. This code will ensure that the mouse isn't set to a On 2016/07/19 02:48:34, Peter Kasting (slow) wrote: > Tiny nit: will ensure that -> ensures Is this a stylistic subjective preference or just trying to reducing verbiage? I do tend to eschew brevity for more clarity. https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:269: // inset after it is adjusted for the device scale. On 2016/07/19 02:48:34, Peter Kasting (slow) wrote: > Wait, isn't this sort of backwards? If the region is always 1 px, then it seems > like we'd need to inset before converting to DIP to match that. That would > potentially have the problem that the 1 px region might get "lost" due to the > lossy conversion to DIP... which suggests that we'd benefit from having the > current position and the desired rect be a PointF and a RectF, respectively... > ugh. I hate that views works in DIP everywhere instead of px. > > If all of the above is correct, then perhaps for now the code here is correct, > but we should have a comment like "The resize region around the caption buttons > is always 1 px. Since views works in DIP, the closest we can get to this while > still ensuring there's always a resizable region is to inset after scaling. > This may still display a resize cursor erroneously when the mouse isn't actually > in the outermost pixel, which seems better than not displaying a resize cursor > at all." > > Or is that not right either? I'd thought about all of that. Yes, I would rather be working in pixels, but the point being passed in is in DIPs. What if we scale the point back to pixels instead for this test? Would that be more or less accurate? <shrug>
https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:254: // corner of the window. This code will ensure that the mouse isn't set to a On 2016/07/19 14:11:14, kylix_rd wrote: > On 2016/07/19 02:48:34, Peter Kasting (slow) wrote: > > Tiny nit: will ensure that -> ensures > > Is this a stylistic subjective preference or just trying to reducing verbiage? I > do tend to eschew brevity for more clarity. I think "will ensure" is a bit unclear because it implies the code will do something later rather than now. So I was mainly just aiming to reduce potential for misinterpretation. https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:269: // inset after it is adjusted for the device scale. On 2016/07/19 14:11:14, kylix_rd wrote: > On 2016/07/19 02:48:34, Peter Kasting (slow) wrote: > > Wait, isn't this sort of backwards? If the region is always 1 px, then it > seems > > like we'd need to inset before converting to DIP to match that. That would > > potentially have the problem that the 1 px region might get "lost" due to the > > lossy conversion to DIP... which suggests that we'd benefit from having the > > current position and the desired rect be a PointF and a RectF, respectively... > > ugh. I hate that views works in DIP everywhere instead of px. > > > > If all of the above is correct, then perhaps for now the code here is correct, > > but we should have a comment like "The resize region around the caption > buttons > > is always 1 px. Since views works in DIP, the closest we can get to this > while > > still ensuring there's always a resizable region is to inset after scaling. > > This may still display a resize cursor erroneously when the mouse isn't > actually > > in the outermost pixel, which seems better than not displaying a resize cursor > > at all." > > > > Or is that not right either? > > I'd thought about all of that. Yes, I would rather be working in pixels, but the > point being passed in is in DIPs. What if we scale the point back to pixels > instead for this test? Would that be more or less accurate? <shrug> I think the issue is that since the mouse coord passed in is not a PointF, we can't know whether it's actually on the outermost 1 px or not. To fix this, we'd need to pass in a PointF (or, even more wildly, pass in an integer in px instead of DIP). That sort of fix goes back to the whole "views does everything in integer DIPs and it shouldn't" issue with high DPI, which isn't addressable in the scope of this patch. So probably the best thing for the code is to do it the way you've done it, but explain in the comment why this is incorrect but the best we can do without larger views API changes.
https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:254: // corner of the window. This code will ensure that the mouse isn't set to a On 2016/07/19 22:44:52, Peter Kasting wrote: > On 2016/07/19 14:11:14, kylix_rd wrote: > > On 2016/07/19 02:48:34, Peter Kasting (slow) wrote: > > > Tiny nit: will ensure that -> ensures > > > > Is this a stylistic subjective preference or just trying to reducing verbiage? > I > > do tend to eschew brevity for more clarity. > > I think "will ensure" is a bit unclear because it implies the code will do > something later rather than now. So I was mainly just aiming to reduce > potential for misinterpretation. Fair enough. I'll defer to your judgement on this. https://codereview.chromium.org/2151903002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:269: // inset after it is adjusted for the device scale. On 2016/07/19 22:44:52, Peter Kasting wrote: > On 2016/07/19 14:11:14, kylix_rd wrote: > > On 2016/07/19 02:48:34, Peter Kasting (slow) wrote: > > > Wait, isn't this sort of backwards? If the region is always 1 px, then it > > seems > > > like we'd need to inset before converting to DIP to match that. That would > > > potentially have the problem that the 1 px region might get "lost" due to > the > > > lossy conversion to DIP... which suggests that we'd benefit from having the > > > current position and the desired rect be a PointF and a RectF, > respectively... > > > ugh. I hate that views works in DIP everywhere instead of px. > > > > > > If all of the above is correct, then perhaps for now the code here is > correct, > > > but we should have a comment like "The resize region around the caption > > buttons > > > is always 1 px. Since views works in DIP, the closest we can get to this > > while > > > still ensuring there's always a resizable region is to inset after scaling. > > > This may still display a resize cursor erroneously when the mouse isn't > > actually > > > in the outermost pixel, which seems better than not displaying a resize > cursor > > > at all." > > > > > > Or is that not right either? > > > > I'd thought about all of that. Yes, I would rather be working in pixels, but > the > > point being passed in is in DIPs. What if we scale the point back to pixels > > instead for this test? Would that be more or less accurate? <shrug> > > I think the issue is that since the mouse coord passed in is not a PointF, we > can't know whether it's actually on the outermost 1 px or not. To fix this, > we'd need to pass in a PointF (or, even more wildly, pass in an integer in px > instead of DIP). That sort of fix goes back to the whole "views does everything > in integer DIPs and it shouldn't" issue with high DPI, which isn't addressable > in the scope of this patch. So probably the best thing for the code is to do it > the way you've done it, but explain in the comment why this is incorrect but the > best we can do without larger views API changes. I think we're in agreement here. This is the best approach given the API we're working with.
LGTM https://codereview.chromium.org/2151903002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2151903002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:272: // best we can do. Nit: This still reads pretty confusingly to me. What do you think of this?: The sizing region at the window edge is 1 px regardless of scale factor. But if we inset by 1 before converting to DIPs, the precision loss might eliminate this region entirely. The best we can do is to inset after conversion, which guarantees we'll show the resize cursor when resizing is possible, at the cost of also showing it over the portion of the DIP that isn't the outermost pixel.
I took most of your comment as is. I did break up the slightly run-on sentence.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2151903002/#ps100001 (title: "Even more clarity for the comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This ensures that the non-client frame hit testing doesn't extend down into the caption button (minimize, maximize, close) area, thus giving the user the impression that the frame is resizable at that point. BUG=548619 ========== to ========== This ensures that the non-client frame hit testing doesn't extend down into the caption button (minimize, maximize, close) area, thus giving the user the impression that the frame is resizable at that point. BUG=548619 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== This ensures that the non-client frame hit testing doesn't extend down into the caption button (minimize, maximize, close) area, thus giving the user the impression that the frame is resizable at that point. BUG=548619 ========== to ========== This ensures that the non-client frame hit testing doesn't extend down into the caption button (minimize, maximize, close) area, thus giving the user the impression that the frame is resizable at that point. BUG=548619 Committed: https://crrev.com/5db27ec315b8568a001a10148fb751867211e48e Cr-Commit-Position: refs/heads/master@{#406651} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5db27ec315b8568a001a10148fb751867211e48e Cr-Commit-Position: refs/heads/master@{#406651} |
