Instant Extended: Reduce clipping in omnibox

Instant Extended: Reduce clipping in omnibox

The larger omnibox font sometimes causes clipping.

This CL increases the line height by 1 pixel to reduce clipping.

Screenshots:
  - before: http://i.imgur.com/waS4sIq.png
  - after: http://i.imgur.com/polxbYZ.png

BUG=237909
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202186

[Scott Hess - ex-Googler, 2013-05-22 19:56:37]

I would prefer you figure out how the Cocoa drawing model works, here, rather
than randomly tweaking things until something works.  I doubt that default
NSTextField instances have a bunch of one-off tweaks for stuff like this.  So
figure out why we need them and fix it, rather than adding a bunch of one-off
tweaks.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:224: return
kBaselineAdjustInstantExtended;
No, this shouldn't be here.  Your example images clearly show that the effective
baseline didn't actually move.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:160: // clipping.
No, it's getting the line height from the font.  Is the font wrong?  Is the
wrong metric being used?  Are we shifting something incorrectly elsewhere?

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/style...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/style...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:38: return
NSIntegralRect(NSInsetRect(cellFrame, 0, [self baselineAdjust]));
Does this imply that cellFrame has fractional bits?  Because otherwise it's just
going to give the same rect as the previous integral value for baselineAdjust
would have given.

[sail, 2013-05-22 20:23:55]

There are two things that cause clipping:
  #1 - The small drawing rect we use.
  #2 - The small line height we set by setMaximumLineHeight:.
If we remove both of those then no clipping occurs.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:224: return
kBaselineAdjustInstantExtended;
On 2013/05/22 19:56:37, shess wrote:
> No, this shouldn't be here.  Your example images clearly show that the
effective
> baseline didn't actually move.

-baselineAdjust isn't used to adjust the baseline. It's used to control the size
of the drawing rect. I can rename it if you like.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:160: // clipping.
On 2013/05/22 19:56:37, shess wrote:
> No, it's getting the line height from the font.  Is the font wrong?  Is the
> wrong metric being used?  Are we shifting something incorrectly elsewhere?

The line height doesn't account for accents and stuff. By setting a fixed
min/max line height the text will get clipped regardless of how big the drawing
rect is.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/style...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/style...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:38: return
NSIntegralRect(NSInsetRect(cellFrame, 0, [self baselineAdjust]));
On 2013/05/22 19:56:37, shess wrote:
> Does this imply that cellFrame has fractional bits?  Because otherwise it's
just
> going to give the same rect as the previous integral value for baselineAdjust
> would have given.

Yes, this will be fractional since it's being inset by 2.5.

[Scott Hess - ex-Googler, 2013-05-22 21:02:36]

It seems like we probably want to have tweaks for:
 - how many pixels from bottom of field to selection rect.
 - how many pixels from bottom of selection rect to baseline.
 - how many pixels from top of field to selection rect.

I am not positive that the current gaps between the inner edge of the field
border and the selection rect are right (2px above, 3px below, which focussed). 
The cert bubble certainly aims for symmetry.  OTOH, can't really move the
old-style baseline, I think, so maybe that would leave too many pixels below? 
Looking at various text fields I can find around OSX, I have no idea if that's a
faux pas or not.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:224: return
kBaselineAdjustInstantExtended;
On 2013/05/22 20:23:55, sail wrote:
> On 2013/05/22 19:56:37, shess wrote:
> > No, this shouldn't be here.  Your example images clearly show that the
> effective
> > baseline didn't actually move.
> 
> -baselineAdjust isn't used to adjust the baseline. It's used to control the
size
> of the drawing rect. I can rename it if you like.

That seems like a good idea, then.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:160: // clipping.
On 2013/05/22 20:23:55, sail wrote:
> On 2013/05/22 19:56:37, shess wrote:
> > No, it's getting the line height from the font.  Is the font wrong?  Is the
> > wrong metric being used?  Are we shifting something incorrectly elsewhere?
> 
> The line height doesn't account for accents and stuff. By setting a fixed
> min/max line height the text will get clipped regardless of how big the
drawing
> rect is.

Yeah, but fields are doing this stuff somehow.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:447: // otherwise the
baseline may shift "downwards".
This seems like the relevant issue.  Sure hope I didn't write it, because this
makes little to no sense.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/style...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/style...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:38: return
NSIntegralRect(NSInsetRect(cellFrame, 0, [self baselineAdjust]));
On 2013/05/22 20:23:55, sail wrote:
> On 2013/05/22 19:56:37, shess wrote:
> > Does this imply that cellFrame has fractional bits?  Because otherwise it's
> just
> > going to give the same rect as the previous integral value for
baselineAdjust
> > would have given.
> 
> Yes, this will be fractional since it's being inset by 2.5.

The NSIntegralRect() means that this will be pushed out to be integral, and the
baseline doesn't go to a half-pixel, so ... this seems very mis-leading.

If we need to have separate hard-coded adjustments for height and offset, fine,
but having everything driven by a "baseline" adjustment which adjusts different
things differently seems poor.

[sail, 2013-05-22 21:10:35]

It's not clear from your comments what you'd like me to do next. I can do the
following if you like:
  - keep the line_height_ change as is
  - revert the baselineAdjust change
  - delete the existing baselineAdjust method and replace it with new top and
bottom offset methods.
Does this seem reasonable?

> I am not positive that the current gaps between the inner edge of the field
> border and the selection rect are right (2px above, 3px below, which
> focussed). 

Yea, alcor@ asked for the new look which is 2px above and 2px below.

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:160: // clipping.
On 2013/05/22 21:02:36, shess wrote:
> On 2013/05/22 20:23:55, sail wrote:
> > On 2013/05/22 19:56:37, shess wrote:
> > > No, it's getting the line height from the font.  Is the font wrong?  Is
the
> > > wrong metric being used?  Are we shifting something incorrectly elsewhere?
> > 
> > The line height doesn't account for accents and stuff. By setting a fixed
> > min/max line height the text will get clipped regardless of how big the
> drawing
> > rect is.
> 
> Yeah, but fields are doing this stuff somehow.

I'm not sure what this comment means.

[Scott Hess - ex-Googler, 2013-05-22 21:21:05]

On 2013/05/22 21:10:35, sail wrote:
> It's not clear from your comments what you'd like me to do next. I can do the
> following if you like:
>   - keep the line_height_ change as is
>   - revert the baselineAdjust change
>   - delete the existing baselineAdjust method and replace it with new top and
> bottom offset methods.
> Does this seem reasonable?

Can you do it with just top and bottom?  The comment where the min/max heights
are set makes me wonder if there aren't cases where the baseline moves
independently of the height.

> > I am not positive that the current gaps between the inner edge of the field
> > border and the selection rect are right (2px above, 3px below, which
> > focussed). 
> 
> Yea, alcor@ asked for the new look which is 2px above and 2px below.

I think if we want a specific top and bottom gap, it's reasonable to have those
values specified, rather than derived.

>
https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
> File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):
> 
>
https://codereview.chromium.org/15553008/diff/1/chrome/browser/ui/cocoa/omnib...
> chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:160: // clipping.
> On 2013/05/22 21:02:36, shess wrote:
> > On 2013/05/22 20:23:55, sail wrote:
> > > On 2013/05/22 19:56:37, shess wrote:
> > > > No, it's getting the line height from the font.  Is the font wrong?  Is
> the
> > > > wrong metric being used?  Are we shifting something incorrectly
elsewhere?
> > > 
> > > The line height doesn't account for accents and stuff. By setting a fixed
> > > min/max line height the text will get clipped regardless of how big the
> > drawing
> > > rect is.
> > 
> > Yeah, but fields are doing this stuff somehow.
> 
> I'm not sure what this comment means.

Regular OSX text fields manage to size themselves to fit ascenders and
descenders without clipping.  What are we doing wrong to make this text field
not work right?

[sail, 2013-05-22 21:23:21]

> Regular OSX text fields manage to size themselves to fit ascenders and
> descenders without clipping.  What are we doing wrong to make this text field
> not work right?

This is the #1 and #2 issue I mentioned above (custom drawingRectForBounds: and
custom setMaximumLineHeight:).

Both those things are needed unfortunately.

[sail, 2013-05-22 21:53:49]

Replaced -baselineAdjust with -topTextFrameOffset and -bottomTextFrameOffset.

Please take another look.

[Scott Hess - ex-Googler, 2013-05-22 23:39:39]

Bear with me on this.  When we're in there faking out Cocoa like this code does,
penalties will be paid.

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:162: line_height_ += 1;
I'm still questioning this guy.  I feel like someone is conflating clipping with
drawing, somewhere?  Or am I missing something?

Hmm.  Using this font in TextEdit, the accents rise above the selection box one
pixel.  Also happens with Lucida Grande.

How do -[NSFont ascender], -[NSFont descender], -[NSFont xHeight] and -[NSFont
capHeight] compare on this?  AFAICT the top pixel is a total of 16 pixels above
the baseline, which match the font size, so maybe [NSFont
ascender]+[NSLayoutManager defaultBaselineOffsetForFont:font] would work.  Or
maybe not.

Is line_height_ literally the selection-rect height?  Since we actually want
that to be a specific value, another possibility would be to pull it from some
new method on AutocompleteTextField.  That would mean some potential for the
font and size to get out of sync, but that's why we're even looking at this CL,
so we haven't gained anything.  I'm assuming that that field has already been
placed at its final size at this point?

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/s...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/s...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:173: return return textFrame;
typo typo

[sail, 2013-05-23 19:46:32]

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:162: line_height_ += 1;
On 2013/05/22 23:39:39, shess wrote:
> I'm still questioning this guy.  I feel like someone is conflating clipping
with
> drawing, somewhere?  Or am I missing something?
> 
> Hmm.  Using this font in TextEdit, the accents rise above the selection box
one
> pixel.  Also happens with Lucida Grande.
> 
> How do -[NSFont ascender], -[NSFont descender], -[NSFont xHeight] and -[NSFont
> capHeight] compare on this?  AFAICT the top pixel is a total of 16 pixels
above
> the baseline, which match the font size, so maybe [NSFont
> ascender]+[NSLayoutManager defaultBaselineOffsetForFont:font] would work.  Or
> maybe not.
> 
> Is line_height_ literally the selection-rect height?  Since we actually want
> that to be a specific value, another possibility would be to pull it from some
> new method on AutocompleteTextField.  That would mean some potential for the
> font and size to get out of sync, but that's why we're even looking at this
CL,
> so we haven't gained anything.  I'm assuming that that field has already been
> placed at its final size at this point?

Replaced this with -[AutocompleteTextFieldCell lineHeight] and used fixed values
instead.

I took a look at the font metrics and I couldn't find anything helpful there.
Here are the font metrics for Helvetica Neue 16pt:
  line_height_: 18
  ascender: 15
  descender: -3
  xHeight: 8
  capHeight: 11
  defaultBaselineOffsetForFont: 15

for the system font 13pt the metrics are:
  line_height_: 16
  ascender: 12
  descender: -2
  xHeight: 6
  capHeight: 9
  defaultBaselineOffsetForFont: 13

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/s...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://codereview.chromium.org/15553008/diff/11001/chrome/browser/ui/cocoa/s...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:173: return return textFrame;
On 2013/05/22 23:39:39, shess wrote:
> typo typo

Done.

[Scott Hess - ex-Googler, 2013-05-23 20:08:02]

Have I mentioned that the OSX text subsystem is awesome?

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h
(right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:31: // Line
height used for text in this field.
s/field/cell/, unless you decide to move this to the field.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:244: return
16;
Per offline discussion, this is kinda fine, except I think it would work better
if it could somehow just fall out of the height-topborder-bottomborder
calculation.  If we're lucky, both values will be right rather than needing a
bunch of adjustment.

Too much more of this and I'll start suggesting someone should write a new file
to contain the random omnibox constants in one place.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:442: [paragraph_style
setMinimumLineHeight:[[field_ cell] lineHeight]];
Definitely memoize this, since the compiler cannot.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:18: -
(NSRect)textFrameForFrameInternal:(NSRect)cellFrame;
I would just put this method above the two clients, and then drop the
@interface.  Rest of the code ain't sorted...

[sail, 2013-05-23 20:45:44]

Addressed review comments.

I also verified that with my change non-instant extended UI is exactly the same
as it used to be.

I also verified that with instant extended the only visible change is that the
selection height is 1 pixel higher.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h
(right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:31: // Line
height used for text in this field.
On 2013/05/23 20:08:02, shess wrote:
> s/field/cell/, unless you decide to move this to the field.

Done.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:244: return
16;
On 2013/05/23 20:08:02, shess wrote:
> Per offline discussion, this is kinda fine, except I think it would work
better
> if it could somehow just fall out of the height-topborder-bottomborder
> calculation.  If we're lucky, both values will be right rather than needing a
> bunch of adjustment.
> 
> Too much more of this and I'll start suggesting someone should write a new
file
> to contain the random omnibox constants in one place.

Unfortunately I couldn't find anyway to derive the line height from the frame
calculations.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:442: [paragraph_style
setMinimumLineHeight:[[field_ cell] lineHeight]];
On 2013/05/23 20:08:02, shess wrote:
> Definitely memoize this, since the compiler cannot.

Done.

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/styled_text_field_cell.mm (right):

https://chromiumcodereview.appspot.com/15553008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/cocoa/styled_text_field_cell.mm:18: -
(NSRect)textFrameForFrameInternal:(NSRect)cellFrame;
On 2013/05/23 20:08:02, shess wrote:
> I would just put this method above the two clients, and then drop the
> @interface.  Rest of the code ain't sorted...

Done.

[Scott Hess - ex-Googler, 2013-05-24 17:03:31]

lgtm.

[sail, 2013-05-24 17:06:08]

On 2013/05/24 17:03:31, shess wrote:
> lgtm.

Yay. I knew I could wear down your principles.

[commit-bot: I haz the power, 2013-05-24 17:06:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/15553008/26001

[commit-bot: I haz the power, 2013-05-24 21:52:49]

Change committed as 202186