Attaching screenshot on 125%. 

Deleted: boxstroke.png 527 KB

	boxstroke.png 527 KB View Download

Sounds like Issue Issue 612270.  Could you test with Google Chrome 57 or later?

Video attached. Compared 56(left) and 58(right) on 150% display.

1. autofill strokes are missing on both 56 and 58. In some place both bottom and right strokes are missing (e.g. City in the video) and in other places bottom stroke is missing, both 56 and 58. 

2. <select> bottom stroke that was missing on 56 does show on 58.
Side note: on 56 there is one <select> popup that already has complete strokes (not missing any) i.e. Payment method on the video. 58 shows the same.  

Deleted: Popup-stroke-align-2017-01-26.mp4 3.4 MB

	Popup-stroke-align-2017-01-26.mp4 3.4 MB View Download

My first thought is that it is related to promoting the popup into a composited layer when we previously weren't, but it seems to happen on non-scrolling popups.
I can't reproduce on Linux with browser zoom in 56.0.2924.67 beta. Will try Windows.

Verified on Win10 with 125% zoom, M58 and M56.

Assigning to myself in the event I look at it today. Others are welcome to investigate whether this is a border painting issue or a layer sizing issue.

I can't reproduce with --force-device-scale-factor=1.25 on linux either.

The popup is not painted in Blink, it's painted via a View delegate. Stack trace (on Linux
at least):

#1 0x7faddb0f0889 autofill::AutofillPopupViewViews::OnPaint()
#2 0x7fadd5e069ba views::View::Paint()
#3 0x7fadd5e08752 views::View::PaintChildren()
#4 0x7fadd5e069e7 views::View::Paint()
#5 0x7fadd5c64fdf ui::Layer::PaintContentsToDisplayList()
#6 0x7fadd5c6515d ui::Layer::PaintContentsToDisplayList()
#7 0x7fadd632c485 cc::PictureLayer::Update()
#8 0x7fadd63ce139 cc::LayerTree::UpdateLayers()
#9 0x7fadd63d1c2d cc::LayerTreeHost::DoUpdateLayers()
#10 0x7fadd63d1736 cc::LayerTreeHost::UpdateLayers()
#11 0x7fadd640c6ba cc::SingleThreadProxy::DoBeginMainFrame()
#12 0x7fadd640cdef cc::SingleThreadProxy::BeginMainFrame()

My guess is that the code which determines the size of the view cc::Layer is rounding incorrectly.

Might be related to --enable-use-zoom-for-dsf. Stephen could you try to repro
without this flag? about:flags has a setting.

Also, the autofill widget is sized in this code:

https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/popup_view_common.cc?cl=GROK&rcl=1485491011&l=96

Happens even when zoom-for-dsf is turned off.

Based on code investigation from chrishtr@, I'm relabeling this as an autofill component.

The autofill sizing uses ints alone. I'm guessing they need more careful rounding. I'm getting a build up and running to see if I can fix it.

ericrk is uploading pictures, but it seems the size of the widget is correct,
just the positioning of the border stroke is wrong.

Here are the comparison shots from 1x, 1.25x and 1.75x on Windows. I've also included a 4x scale of each to make it easy to see the border differences.

Deleted: autofill_border.png 24.5 KB

	autofill_border.png 24.5 KB View Download

Another possibility is that the border is being overwritten by other content.
The border is currently painted before other content:

  canvas->DrawColor(GetNativeTheme()->GetSystemColor(
      ui::NativeTheme::kColorId_ResultsTableNormalBackground));
  OnPaintBorder(canvas);

  for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
    gfx::Rect line_rect = controller_->layout_model().GetRowBounds(i);

    if (controller_->GetSuggestionAt(i).frontend_id ==
        POPUP_ITEM_ID_SEPARATOR) {
      canvas->FillRect(
          line_rect,
          GetNativeTheme()->GetSystemColor(
              ui::NativeTheme::kColorId_ResultsTableNormalDimmedText));
    } else {
      DrawAutofillEntry(canvas, i, line_rect);
    }
  }
}

Changing it to paint after could check that theory.

I have a theory that FillRect as used here is not great with 125/150% displays (it's used in the code snippet above to draw a separator and in DrawAutofillEntry, where it draws a big white box with text in it).

Should we experiment with a fractional RectF?

Seems reasonable to try.

Could your team take ownership of the bug?

The primary goal is to be consistent across everything you draw. In Blink we do things like accumulate fractional rounding to avoid problems like this, and compute lengths based on containing elements etc. We still get it wrong in various cases, generally due to having some sub-pixel sized content and some pixel snapped content. I think you can avoid that because you are not dealing with images, the primary source of our problems.

Drawing the border last does not fix it. On to adjusting the size of the layer.

Just adding 1 to the layer size fixes the missing bottom border, so it is indeed a layer sizing issue.

Not necessarily, right? Could be the border is not painted at the right location.

Excellent point. A cursory look suggests that everything in the layer sizing is integral, so adjusting where things are drawn may well be simpler.

The bug can be fixed by changing browser\ui\autofill\autofill_popup_layout_model.cc:276 from
 return gfx::ToEnclosingRect(delegate_->element_bounds());
to
 return gfx::ToNearestRect(delegate_->element_bounds());

The size returned by Display::SetScaleAndBounds are floored points, so ideally we would use the same thing to convert the element size, but I don't immediately see how to do that and nothing seems to break if I change AutofillPopupLayoutModel::RoundedElementBounds() as above.

We have related ugliness in that the popup overdraws the focus ring on the element when the device has high DPI, but not otherwise, so the location of the popup is also not what we ideally want. The best fix is probably to have all the code agree on how big the popup will be, and someone with more knowledge of the system might want to do that.

I'll put up a patch. I suspect some unit tests might break.

ToFlooredRectDeprecated() will floor all points, but is usually wrong.

I don't know enough about how autofill draws to solve your problem, but it's suspicious that you'd want to change from the enclosing rect to the nearest rect.  I'd like to hear a full explanation of who paints border vs. content and how each is currently computing the integral coordinates they should paint.

Patch at https://codereview.chromium.org/2658123003/ but it does not fix the 175% case. My alternate fix does not fix the 125% case. So still working.

pkasting@, you're totally right. It would be better to identify exactly how the points are moving around under rounding and fix this properly, but I'm out of time before being away from my Windows 10 machine for a week. As I say on the review, I'm totally happy if my work just serves as a starting point for someone else to pick it up.

A bisect would still be helpful too.

Final thoughts for this week. A better fix is problably to pass un-rounded element_bounds into PopupViewCommon::CalculatePopupBounds and round up or down depending on whether the popup is going above or below, or other considerations. CalculatePopupYAndHeight takes mins and maxes and subtracts, which is a sure-fire way to reveal problems from rounding the wrong way.

Making the bug untriaged for the Autofill team. If you're not going to fix this before next Friday, assign it back to me.

We sure appreciate you looking into this! We will try to find someone to look at this very shortly.

Hey Math and team, wanted to check in -- any estimate on a time for fix?

csashi@ is looking at moving this code to views (instead of explicit drawing) to fix accessibility issues. This change should also fix these rendering issues.

The target is M58.

It's unclear if moving to views code will address this particular issue of bounds:  schenney@'s investigation remains valid, and has yet to find an owner on Autofill side.

I'll try to find time to poke around with this when traveling tomorrow. It's just a slow grind looking at rounding behavior.

On a related note, how do you test this code?

Thanks! There are a few test forms at https://rsolomakhin.github.io/autofill/, or http://zkoch.github.io/https-things/datalist.html for the datalist version

Thanks, Stephen! Checking in to see if you were able to poke around, or have a timeframe for fix (beyond the move to views).

I'll probably get to it tomorrow, or maybe even later today.

All evidence right now points to a Skia bug or compositor bug that manifests when an SkPicture is replayed at scale due to high DPI.

The border is painted by view.cc, with a Skia recording canvas. The values below are at 100% DPI, but the rough sizes and relationship is retained under higher DPI settings so it seems that the DPI adjustment is elsewhere in the code.

These are the clip rect, transform and painting cull rect:
View::Paint for 0000000038C83D08 clip 0,0 173x122
View::Paint for 0000000038C83D08 transform [ +1.0000 +0.0000 +0.0000 +0.0000  
  +0.0000 +1.0000 +0.0000 +0.0000  
  +0.0000 +0.0000 +1.0000 +0.0000  
  +0.0000 +0.0000 +0.0000 +1.0000 ]
View::Paint for 0000000038C83D08 cullRect 173x122

The canvas "base layer size" matches the cull rect, as one would expect:
[INFO:view.cc(1554)] base layer size: 173 122

And the border size is:
[INFO:border.cc(45)] SolidSidedBorder::Paint on 0000000038C83D08
[INFO:border.cc(46)]   Top 0 0 173 1
[INFO:border.cc(47)]   Left 0 1 1 120
[INFO:border.cc(48)]   Bottom 0 121 173 1
[INFO:border.cc(49)]   Right 172 1 1 120

Unless I've lost my ability to reason about pixel locations, that border does indeed draw around the edge of the underlying canvas, and the clip rect and cull rect do not clip it out. I verified that the clip does not make any difference.

So the issue arises when the display list is rasterized.

I'll continue to own it until I can isolate the breaking change and/or code location.

Still need a bisect.

I do not have time to investigate further. I'm un-triaging so that the compositing team can decide how to proceed.

Eric kindly agreed to take a look now.

Did a bisect, it looks like this issue was introduced with crrev.com/2263453002.

I'll need to dig a bit more to understand how everything interacts.

It looks like a case where we were previously broken, and a change that was put in to address one visual artifact has introduced another.

The root issue is that we start with a window we need to fill in actual device pixels. We then convert this into DIPs for the paint code. We then scale this back to device pixels for the final rasterization. I don't think there's a way to round which guarantees that DPs > DIPs > DPs round-trips when dealing with non-integer scale factors.

We used to use a flooring operation, which could result in us underfilling the Aura window (leaving a black border). We switched to a ceil operation, which has the opposite problem, our contents may be clipped.

There may be a way to adjust the transformation from DIPs > DPs in CC during rasterization, such that we always round back to the right value - looking into this now.

After looking at a number of ways to fix this, decided to go with a patch to the autofill popup drawing code. This produces the best results of the methods I tried (the border is always flush with the popup contents), and has the least risk of regressing something else. https://codereview.chromium.org/2694183005/

Thanks so much @ericrk! When this is landed, could you let me know what chrome version it's in?

Unfortunately, this turned out to be fairly tricky to fix, due to the way Aura converts from DIPs > Pixels and Pixels > DIPs.

To give some background, when rendering in DIPs, we're left with the option to either clip contents or underfill the widget. We currently clip on Windows (leading to the missing border). We could move to underfilling, but this has its own issues. See the doc I put together here: https://goo.gl/c3ZB8W.

The easiest way to fix this would be to ensure that the DIP > Pixels and Pixels > DIP conversions are round-trippable and uniform everywhere they occur. This should let estade@'s recent cl (https://codereview.chromium.org/2694933002/) work correctly, leading to the right border.

Unfortunately, these conversions are fairly inconsistent right now. To give a few examples:

These operations take the enclosing rect when converting from Pixels > DIPs
- ScreenToDIPRectInWindow (both the generic and Windows versions) take the enclosing rect when converting either direction.
- WindowTreeHost::UpdateRootWindowSizeInPixels takes the enclosing rect when converting from Pixels > DIPs

These take enclosed rect:
- RootWindowTransformer::GetRootWindowBounds does a floor operation (simlar to enclosed rect) when converting from Pixels > DIPs
- ui::ConvertRectToDIP takes an floor (enclosed rect) when converting Pixels > DIPs
- ui::ConvertRectToPixels takes an enclosing rect when converting DIPs > Pixels

Because of the number of places these functions are used, it seems like a fairly risky change to adjust these. Even between platforms (CrOS vs Windows), inconsistencies lead to different behavior (we underfill the Autofill popup on CrOS and overfill/clip on Windows).

I tried to put together a fix that would work around these inconsistencies by adding additional border-filling logic ( https://codereview.chromium.org/2718533004). Unfortunately, because of the differences between CrOS and Windows, I wasn't able to get a consistent experience on each.

Unfortunately the Aura code isn't something I'm super familiar with, and I don't have the bandwidth to continue pushing on this at the moment. Can someone with more expertise in Aura / UI code take this over?

So, basically, if we make sure the functions you list are consistent about pairing enclosed vs. enclosing when going back and forth, this bug should disappear?

Assuming yes, and morphing.

It seems like, if we want to promote "clip" instead of "underfill" as the way to handle inexactness, we should take the enclosed rect for DIPs -> px and the enclosing rect for px -> DIPs.  +CC bsep/robliao/sky to sanity-check that proposal.

> So, basically, if we make sure the functions you list are consistent about pairing enclosed vs. enclosing when going back and forth, this bug should disappear?

Yup, making the behavior uniform among the functions mentioned above fixes the issue for me locally.

Ping on this bug - bsep/robliao/sky do you have feedback? This bug is blocking
a new feature so we really need an owner for it.

I'm not sure there is a right answer here (other than to use pixels everywhere, which is out of scope). I suspect attempting to change any of the central functions (e.g. ScreenToDIPRectInWindow) is going to be problematic and cause side effects in at least some environment.

Have you tried undoing the scale and drawing in pixels at the place that is doing the border?

Yes.  That's not sufficient to fix the issue.

I agree that we'll likely have side effects, but that's sort of the design intent: we want side effects like clipping for now.  I think by being consistent about "enclosed rect when going to px, enclosing rect when going to DIPs" we'll tend to get the side effects we want, instead of the set of side effects we have today, which is just kind of arbitrary.

I'm worried that "enclosed rect when going to px" is not always the right thing. When scheduling a paint don't we want to go to enclosing rect?

I agree it is not always the right thing. When a pixel is partially included in a rect, sometimes you want that pixel to be in the final result and sometimes you don't.

When considering occlusion for instance, enclosed rect is correct because a partially occluded pixel is not considered occluded.
When considering damage, enclosing rect is correct because a partially damaged pixel is considered damaged.

What problem occurred when undoing the scale? How did you calculate the bounds to draw to?

See comment 49, specifically https://codereview.chromium.org/2694933002/ .

It is OK to say that we don't always want enclosed vs. enclosing, but that's not an argument for doing nothing, that's an argument that we should have no global default, and every call should individually be audited and forced to do the right thing.  I don't think that's tractable.  In the absence of that, my claim is that at least changing so the global defaults perform the inverse operations of each other when you round-trip is better than the current seemingly-random system.  The only other sane choice, as I see it, is to still perform the inverse operations, but flip the defaults from what I said (do enclosing rect when going DIPs -> px and enclosed rect otherwise).

Unless I'm missing something I can see two possible solutions to this:

1. functions that attempt to draw borders to the edges of windows need to draw in pixels using the pixel size of the window. For linux/windows you can get this size by way of widget->GetNativeWindow()->GetHost()->GetBoundsInPixels().

2. Don't clip, instead use the enclosing rect. To avoid black bars at the edges force the window to be transparent and fill those areas transparent.

(1) is painful in that it requires every place we have code like this to deal with the issue. That could be made easier by having an explicit api that deals. I'm not sure how many places we have like auto-fill.

(2) only makes sense for popup style windows (such as autofill) and likely has performance ramifications.

Neither of these options are particular compelling and further motivate why I feel using DIPs in views and aura is wrong. Views need to be sized, positioned and drawn in pixels to avoid these problems.

As to what conversion functions should do. The problem with functions that sounds authoritative, e.g. ConvertSizeToPixel() is that what is right depends upon the context. We should rename all functions to indicate the direction they are rounding.

I totally agree that the right long-term fix is "use px everywhere".  This bug is about finding an implementable short-term fix, since DIPs-to-px is unlikely to happen in the near future.

For (1), not only is this painful, it also only addresses the issue of "I happen to know I want to draw a border at the edge of the window", not the more general problem of e.g. getting a different window size (in DIPs) than you requested (in DIPs) when creating a window, or other potential clipping/underpainting problems.

(2) got discussed and, as you mention, has both scope and perf concerns.

So I agree that I don't really find either of these compelling.

That's precisely why I'm looking to make the conversion functions do something "as sane as possible" (acknowledging that no option is completely sane) instead.

I'm OK with renaming the conversion functions to make their behavior clearer as part of this process.

IMO (1) is the right short term solution for the specific problem of the autofill popup. Trying to change functions used in a ton of places is likely to have lots of unintended side effects that will take a while to completely sort out.

What I'm trying to say is that this doesn't even solve the autofill popup alone.  It doesn't solve the problem I mentioned where we ask for a window of one DIP size and get back a different DIP size because we round-trip through functions that both convert to the enclosing rect.

Other people already changed (last year) some of the functions used broadly in lots of places.  I'm trying to fix the broad fallout from that change, visible in multiple distinct ways in the autofill case alone but also likely affecting lots of other stuff, in an equally broad way.  Yes, it is likely to have side effects; no, I do not believe any lesser bandaid is appropriate instead.

I'm curious to see the problem in action. How can I reproduce it?

Do the steps in comment 0 work?

I had to force 1.35 to see issues. I have no doubt there are issues at other scale factors, but 1.35 made things most evident.

The interesting cases are when the rounding differences are small in one direction (and big in the other). For example, how should you round 6 DIPS at a scale of 1.35? In floating point that's 8.1 pixels. If we want round up to 9 pixels and we want to preserve the mapping then we have to end up at 6 DIPs. This means when drawing we only impact 10% of the pixels along the edge. In other words we mostly end up with a black line no matter what the painting code does (assuming the window is not transparent, which generally we don't do).

So then, always floor. Then you have to consider a big difference in the other extreme. 5 DIPs is 6.75 pixels, rounding down is 6 pixels. If you round down then you end up cutting off a major portion of the last pixel, and single line pixel borders are going to look very slight to non-existent.

Maybe rounding works best for < 2 scales. Once you get to > 2 scales you have the possibility of pixels you either can't draw to at all, or are cut off. I don't have a good answer for this, other than forcing windows sizes not to allow for it, but that may have subtle effects where maximized/fullscreen window sizes needs to be dictated by physical hardware, and to do otherwise may have weird consequences (such as not hiding the shelf on windows).

Here's diffs for rounding that made the 1.35 case better for me:

diff --git a/ui/display/win/screen_win.cc b/ui/display/win/screen_win.cc
index 636eb90d..01b9295 100644
--- a/ui/display/win/screen_win.cc
+++ b/ui/display/win/screen_win.cc
@@ -275,14 +275,15 @@ gfx::Rect ScreenWin::DIPToScreenRect(HWND hwnd, const gfx::Rect& dip_bounds) {
       : GetScreenWinDisplayVia(
             &ScreenWin::GetScreenWinDisplayNearestDIPRect, dip_bounds);
   float scale_factor = screen_win_display.display().device_scale_factor();
-  gfx::Rect screen_rect = ScaleToEnclosingRect(dip_bounds, scale_factor);
+  const gfx::Size size_in_pixels =
+      ScaleToRoundedSize(dip_bounds.size(), scale_factor);
   const Display display = screen_win_display.display();
-  screen_rect.set_origin(ScalePointRelative(
+  const gfx::Point origin_in_pixels(ScalePointRelative(
       display.bounds().origin(),
       screen_win_display.pixel_bounds().origin(),
       scale_factor,
       dip_bounds.origin()));
-  return screen_rect;
+  return gfx::Rect(origin_in_pixels, size_in_pixels);
 }
 
 // static

diff --git a/ui/aura/window_tree_host.cc b/ui/aura/window_tree_host.cc
index e624728..02455ed 100644
--- a/ui/aura/window_tree_host.cc
+++ b/ui/aura/window_tree_host.cc
@@ -113,10 +113,13 @@ void WindowTreeHost::UpdateRootWindowSizeInPixels(
                    output_surface_padding_in_pixels_.top(),
                    host_size_in_pixels.width(), host_size_in_pixels.height());
   float scale_factor = ui::GetDeviceScaleFactor(window()->layer());
-  gfx::RectF new_bounds =
+  gfx::RectF new_bounds_f =
       gfx::ScaleRect(gfx::RectF(bounds), 1.0f / scale_factor);
-  window()->layer()->transform().TransformRect(&new_bounds);
-  window()->SetBounds(gfx::ToEnclosingRect(new_bounds));
+  window()->layer()->transform().TransformRect(&new_bounds_f);
+  const gfx::Rect new_bounds(
+      gfx::ToFlooredPoint(new_bounds_f.origin()),
+      gfx::ToRoundedSize(new_bounds_f.size()));
+  window()->SetBounds(new_bounds);
 }
 
 void WindowTreeHost::ConvertDIPToScreenInPixels(gfx::Point* point) const {

I still maintain that having conversion functions without the direction in them is confusing, because it sounds as though they are the *right* way to round. There is no right way to round, it depends upon what you want to do. We should rename the functions to indicate what they are doing and call sites should be evaluated for the best fit.

NOTE: the code in ScreenWin is used DesktopNativeWidgetAura::SetBounds and as it stands now (using ScaleToEnclosingRect) is very problematic. The reason for that is that ScaleToEnclosingRect() scales the upper-left and lower-right corners of the window and subtracts those to get a new size. This means the same DIP size can vary depending upon the location of the window. That's problematic. The size should be the same no matter where the window lies.

Yes, renaming so effects are explicit, and it makes it clear there is no "best answer", seems like pure win to me.  As noted previously, we should do that.  I'm not sure how to get to the world where we've evaluated every call site for best fit.

Using a rounded size means we'll be reversing course on the call that "if we have to have error, better to clip than underpaint".  Instead it's a way of saying "if we have to have error, minimize the magnitude of the error".  Making this change will, I think, reintroduce the issue that we sometimes have unfilled areas in windows, without fixing all the clipping cases.  It will just reduce the incidence of each.  I don't have a strong opinion about which set of tradeoffs is correct, but it seems like we should acknowledge that in principle.

Also, I'd assume if we update e.g. DIPToScreenRect() to round, we want to update ScreenToDIPRect() to round too.  That is, the principle I suggested earlier still applies: functions named as inverses should do the inverse operation, it's just that the inverse of rounding is rounding.

Nice summary!

I tend to think ScreenToDIPRect would need to be updated as well, but it's hard to say given I don't know who the callers are and their expectations. As you can see the from the patch, the code responsible for calculating the pixel to dip conversion in WindowTreeHost doesn't use ScreenToDIPRect, so a change there wasn't necessary.