Assigning to orinj@

(I filed the bug in my stable channel chrome, but the screenshot is from today's canary -- ignore the user agent string in comment 0)

I think this is not a macOS-only issue. (Please see  issue 851041  C#4.)

Definitely not Mac-only -- this happens on Linux and, I guess, everywhere.  The linked bug shows a potentially helpful extra screenshot with the ink drop rendering outside the shield-shaped mask.  So it might be a masking issue in ink drop logic for highlights -- or it might be other things rendering over the highlight circle (which looks like it should be larger, judging by that bottom round edge).  I started looking at ink_drop_* in ui/views/animation but ideas about the cause are welcome, the ink drop is just my early suspect.

+pbos@ who can probably help here :)

It looks like perhaps the highlight is getting clipped to the new tab button bounds, and they're too small for the whole highlight.

The first thing to ask is whether the highlight is the right size.  Perhaps the highlight is a larger radius than specced, because I thought the spec had the highlight radius contained within the existing bounds.

The second thing to ask is whether the bounds are too small.  Perhaps the bounds need to be enlarged (and the padding around the button maybe shrunk?), if the button hit area (which ought ideally to correspond roughly to the highlight) is not big enough.

If neither of those is the problem, then the button probably needs to paint to a layer so it can paint outside its own bounds.  This is technically easy, but definitely don't do it without ensuring that the highlight and bounds are both correct first, since we wouldn't want to mask errors in those.

I added a bit of debug output in RoundRectInkDropMask::OnPaintLayer (what draws the highlight) to see that:

corner_radius_ = 14, dsf = 1, masking_bound = 0.000000,0.000000 28.000000x28.000000, layer size: 28x28

So it's a circle, drawn using this round rectangle, filling the space of the layer.  And if I push the rectangle right/down by 0.25 and shrink by 0.5 then it draws a nice little unclipped circle around the + icon.  So maybe this whole layer is clipped, not just the round rect highlight itself.

One interesting thing I'm noticing is that changing flags in this function doesn't seem to have all intended effects.  For example, setColor to red -> still gray.

I'll try to zoom out and see what owns this layer and how it's being clipped, maybe try turning off that clipping to see if I can get it to render in full -- the ripple doesn't get clipped, only highlight.

Making progress, I think: RoundedRectangleLayerDelegate::OnPaintLayer does the actual draw, apparently (even in red when I tell it to).  And here, GetPaintedBounds() returns only 24x24 not 28x28 ... so I guess once I figure out the discrepancy it may be correctable.

Found the cause of the discrepancy: in ink_drop_host_view.h...

static constexpr int kDefaultInkDropSize = 24;

CreateDefaultInkDropHighlight has a default parameter for size using this.  I suspect somewhere in the call chain, the full size is being omitted, taking the default of 24x24.  Sure enough, if I force 28x28 then I get an almost-perfect circle -- and if I force 28x32 (the true size according to debug output below) then I get the full circle unclipped.

InkDropHostView::CreateInkDropHighlight - GetContentsBounds=0,0 28x32 GetVisibleBounds=0,0 28x32

I will seek out the call using default size and see if I can find sensible context to pass through instead.

Right, so the proper fix appears to be adding the second parameter in the call below, and my question for pkasting@ is: will this have unintended consequences elsewhere?  Seems like a pretty deep level to change that could affect many derivatives - but at least in this case, it clearly corrects behavior.

std::unique_ptr<InkDropHighlight> InkDropHostView::CreateInkDropHighlight()
    const {
  return CreateDefaultInkDropHighlight(
      gfx::RectF(GetMirroredRect(GetContentsBounds())).CenterPoint(), GetContentsBounds().size());
}

For now I'd suggest you override CreateInkDropHighlight in NewTabButton similar to these, that way this'll only affect NTB:

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/new_tab_button.cc?type=cs&sq=package:chromium&g=0&l=212

You can also check if we're on Refresh and return the base implementation otherwise. That way you don't need to verify that this works well in other MD modes.

That definitely sounds safer, thanks!  But if the fix is correct, and it seems to be more correct than using a constant default, then it can prevent similar issues from cropping up elsewhere in the future.  Any thoughts on the correctness of the change as is?  Personally I prefer to risk high for right, but if anyone's in doubt then I'll take the safer route for the CL. :)

Hmmmmm.

I like the idea of fixing this "the right way".  But it's not obvious to me what "the right way" is.  Is the highlight only bigger than 24x24 in the first place because NewTabButton::CreateInkDropRipple() has been overridden (and uses the visible bounds' size)?  If so, then either the rule is "people who override that should also override CreateInkDropHighlight()" (and we should fix this spot + maybe audit/fix elsewhere), or we should adjust the API so that people can't do the wrong thing here (e.g. one API provides the size/shape for both ripple and highlight, or in some other way the methods are linked).

I think doing it the safe way for now is good, we should reconsider large parts of the inkdrop API later as it's used by overriding way too many methods and really easy to get inconsistent parameters by overriding only X out of Y methods.

I like the idea of defining the inkdrop shape separately as it can also be used for the focus ring and we could also generate the inkdrop mask from it, hence hoping to have some sort of consistency.

Agreed, and as a Noogler I'm probably in deeper waters than I know, so I will happily narrow the fix as suggested for now.  I'll just drop a comment where I see an opportunity to use context over a parameter default, in case something like this pattern exists elsewhere or in the future.  API tweaks for consistency are even better though, someday! :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/651b61fdd2ae4c4973a110364faa3443c8b2951c

commit 651b61fdd2ae4c4973a110364faa3443c8b2951c
Author: Orin Jaworski <orinj@chromium.org>
Date: Tue Jun 19 17:52:51 2018

Use content bounds size for ink drop highlight

The new tab button ink drop needed a larger size than the default
and overriding to pass bounds size corrected a rendering artifact
on the highlight.

Bug:  853700 
Change-Id: Iff98f43049df3fa0c7142c3213707f423b6b1c14
Reviewed-on: https://chromium-review.googlesource.com/1105487
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568520}
[modify] https://crrev.com/651b61fdd2ae4c4973a110364faa3443c8b2951c/chrome/browser/ui/views/tabs/new_tab_button.cc
[modify] https://crrev.com/651b61fdd2ae4c4973a110364faa3443c8b2951c/chrome/browser/ui/views/tabs/new_tab_button.h

This looks fixed.