Refresh: tabs are detached at 125% scaling |
||||||||||||||||
Issue description
,
May 31 2018
,
Jun 1 2018
Load-balancing this to bsep@ too.
,
Jun 12 2018
After pkasting@ committed https://chromium-review.googlesource.com/1083781, this doesn't appear to be an issue. This is at forced device scale factor of 1.25. The gap no longer appears.
,
Jun 13 2018
Delegating
,
Jun 19 2018
It happens at 150%, 175%, 250%, and 350% scaling. It doesn't happen at any scale when the window is maximized. At 150, at least, the tab strip and the toolbar are individually scaled correctly - i.e. the line is NOT there because one of the child elements is being drawn one pixel too small. I also tried enabling the bookmarks bar, that didn't affect anything. So from what I understand of this views layout stuff so far it seems likely that the toolbar or tab strip are advertising the wrong minimum or preferred height.
,
Jun 21 2018
OK, I have a better understanding of what's happening now. The tab strip bounds are being scaled relative to the top of the window, which includes the 1-pixel border. That's 9 dips, so when scaling to any of the common non-integral scale factors, it ends up being at a fractional pixel offset from the top of the window. At some of those offsets, the toolbar and the tab strip (or maybe the tab strip and the title bar that contains the tab strip) round differently. I believe that it should not be including the 1-pixel border when it's scaling, i.e. it should be treating the y offset as 8 dips + 1 pixel instead of 9 dips. Then, with any scaling that's a multiple of .25, it will always land on integral pixel coordinates and this class of bugs shouldn't happen. Alternatively (or additionally), I could just fiddle with the rounding to make sure that the tab strip and the toolbar always agree on how to handle non-integral pixel coordinates for their shared boundary. This way it would do something sensible at every scale factor. But I'm not really sure if we mean to support arbitrary scaling factors, so that might not be that relevant.
,
Jun 21 2018
#9: We don't need to support arbitrary scaling factors, but it might be hard to actually get the y-offset to be 8 dips + 1 pixel. Right now the engine doesn't support laying out between dip boundaries. So you'll probably need to get the tapstrip and toolbar to agree.
,
Jun 21 2018
Also, to disagree a bit with bsep, while arbitrary scale factors are uncommon, they are in fact possible, and we do want to support them, so even if you could make something "8 DIP + 1 px" (which I don't think can be done today), I would avoid that. The route of getting these two things to agree on where their border is sounds right to me.
,
Jun 21 2018
At minimum, we need to make sure this works with 125% and 150%.
,
Jun 22 2018
Dug more - it's not really possible to directly make the tabstrip and toolbar agree on rounding, because the tabstrip and toolbar drawing code aren't actually doing any meaningful rounding at all. They draw in local pixel space, and in the common cases work wholly with integral pixel coordinates. Those draw operations are then transformed into global pixel space in View::Paint and subsequently clipped. (In this case, that local->global transformation is a translation by 9 dips, which is not an integer number of pixels in many of the common scaling cases.) That transformation handles non-integral pixel coordinates by blending, whereas the clipping rounds to the nearest integer. Some examples: In the 125 case (see 125_zoomed.png), the tab draws in the (0, 45) range of y-coordinates in its local pixel space. Everything falls on integer pixel coordinates and it's all great. Those draw instructions get transformed to the (11.25, 56.25) range of y-coordinates, and are subsequently clipped to (11, 56). As a result you can see that the top of the tab is blended/fuzzy/not a sharp line, and the curvature at the bottom of the tab is *slightly* wrong. In the 150 case (see 150_zoomed.png), the tab draws in the (0, 54) range of y-coordinates in its local pixel space. Again, everything falls on integer pixels coordinates and it's all great. Those draw instructions get transformed to the (13.5, 67.5) range of y-coordinates, and are subsequently clipped to (14, 68). As a result, the top of the tab loses half a pixel, so the curvature is slightly wrong, and there is a half-pixel gap below the tab, which is pretty starkly visible. So this is really a pretty systemic root cause. Best options I can see without touching View::Paint are: - shrink the above-the-tabs area by one pixel to 8 or grow it by three to 12. Doesn't fix arbitrary scaling factors and has design implications. - do some hacks to draw the tab one slightly bigger on the bottom while still laying out the toolbar as if we weren't doing that. Doesn't fix the top of the tab and it's pretty gross/hacky, but it should prevent detached tabs at every scale. I can sketch out #2 to see how it looks, but I do feel like a deeper fix would be a good idea when we don't have deadline pressure.
,
Jun 22 2018
+CC malaykeshav@: is there something we can do to get the tabstrip/toolbar scaling and transform adjusted so we don't clip off fractions of pixels as described above, per your past "scalable views" work?
,
Jun 22 2018
How does enabling pixel canvas from chrome://flags effect this? Also you may need to enable the flag for #enable-use-zoom-for-dsf.
,
Jun 22 2018
We had this issue in Chrome OS before Pixel Canvas was enabled. If enabling the flag in #15 does indeed solve it, I can work towards enabling it by default on Windows. (Its been on my plate for a while Issue 765723)
,
Jun 22 2018
I think this is issue 628039 ?
,
Jun 22 2018
Issue 628039 has been merged into this issue.
,
Jun 22 2018
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69912f0c3379febf6e729ee45d7d011aee79f8bb commit 69912f0c3379febf6e729ee45d7d011aee79f8bb Author: Taylor Bergquist <tbergquist@chromium.org> Date: Fri Jun 29 22:33:52 2018 Move toolbar background painting into toolbar view Make ToolbarView draw the toolbar background instead of BrowserNonClientFrameView. This way the painting can be done in local space and the draw order obeys what is implied by the hierarchy. Doing this to support a bandaid mitigating crbug.com/848106 for refresh. Bug: 848106 Change-Id: Icb99b6ec1058418acbdb5be23117715cf66f50a5 Reviewed-on: https://chromium-review.googlesource.com/1119365 Commit-Queue: Taylor Bergquist <tbergquist@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#571682} [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/browser_non_client_frame_view.h [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/glass_browser_frame_view.cc [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/toolbar/toolbar_view.cc [modify] https://crrev.com/69912f0c3379febf6e729ee45d7d011aee79f8bb/chrome/browser/ui/views/toolbar/toolbar_view.h
,
Jul 2
Unable to reproduce the issue on win-10 and Mac 10.13.3 using chrome build without fix as per comment #6 and #8. Observed that at about 150% scaling, there is no gap seen in tabs. Attaching screenshots for reference. tbergquist@ - Could you please check the attached screenshots and please help us in confirming the fix. Thanks...!!
,
Jul 2
Sorry for the misunderstanding - the above CL in #20 isn't a fix for the issue, it's just a refactor that I pulled out of my fix-in-progress. That is still in review. I can still repro locally (on the tip of master). One note is that it looks in your repro like the window is maximized - this issue only occurs on windows in a restored state.
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c18d19908d8817f4057df5e50afc966d92a25701 commit c18d19908d8817f4057df5e50afc966d92a25701 Author: Taylor Bergquist <tbergquist@chromium.org> Date: Wed Jul 11 22:40:34 2018 Bandaid detached tabs at 150% scale At some dpi scales (150% for example), the tabs are drawn detached from the toolbar, with a stark one-pixel gap between the tabs and the toolbar. This CL draws one extra dip's worth of tab below the bottom of the tab, which is occluded by the toolbar at integral scales and fills the gap on non-integral scales. Bug: 848106 Change-Id: Ib465e8886c49613916bf3f3faff5fc43f36dfb18 Reviewed-on: https://chromium-review.googlesource.com/1121386 Commit-Queue: Taylor Bergquist <tbergquist@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#574384} [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/layout_constants.cc [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/layout_constants.h [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/views/frame/browser_non_client_frame_view_unittest.cc [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/views/frame/browser_view_layout.cc [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/views/frame/browser_view_unittest.cc [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/views/tabs/new_tab_button.cc [modify] https://crrev.com/c18d19908d8817f4057df5e50afc966d92a25701/chrome/browser/ui/views/tabs/tab.cc |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by robliao@chromium.org
, May 31 2018