Adding RB label as this is a recent regression

This is not a web-based UI, so it can't be r400190.

Perhaps r400184, which touches chrome UI?

It appears from the image that the bookmarks bar isn't visible. The changes in r400184 *only* affect an empty bookmarks bar. This looks like a positioning issue for the omnibox dropdown, not bookmark bar related.

Unable to reproduce the issue on Windows 7 using latest M-53(53.0.2785.0).Observed that traces of text is not seen between omnibox and suggestion list.

Please find attached screenshot.

vvishwakarma@Could you please check the issue on latest M-53 and update the thread with your observations.

Deleted: 622678.png 12.0 KB

	622678.png 12.0 KB View Download

With response to comment #5: Above issue is reproducible on Chrome version: 54.0.2787.0 on Windows 8 and 10. Issue is not reproducible on Windows 7. Please refer attached screenshot.

Deleted: traces_actual.jpg 27.9 KB

	traces_actual.jpg 27.9 KB View Download

Unable to reproduce the issue on Windows 8 using latest M-53(53.0.2785.8) and latest M-54(54.0.2791.0).Observed that traces of text is not seen between omnibox and suggestion list.

Please find attached screenshot.

vvishwakarma@Could you please check the issue on latest M-53, 54 and update the thread with your observations.

Deleted: 622678_win8.png 8.9 KB

	622678_win8.png 8.9 KB View Download

With response to comment #7: The above issue is reproducible on latest M-54 (54.0.2794.0) on Windows 8 and 10. Issue is reproducible when browser language is changed to Hindi. It is not reproducible for English language. Please refer attached screencast.

Deleted: win8_behaviour.mp4 508 KB

	win8_behaviour.mp4 508 KB View Download

Deleted: win10_behaviour.mp4 360 KB

	win10_behaviour.mp4 360 KB View Download

@kylixrd: Gentle Ping!

Just to update, issue is still observed on Win 10 for chrome version 54.0.2803.0. As mentioned above issue is seen when browser language is changed to Hindi.

@kylixrd: Request you to please take a look into it, issue is marked with a stable blocker and M53 is going to be pushed to Beta soon.

Is there someone else that can take this? I doubt I'm the right person for this.

From the Good and Bad build changelog below:
---------------------------------------------------------
https://chromium.googlesource.com/chromium/src/+log/53.0.2767.0..53.0.2770.0?pretty=fuller&n=10000

Plausible offending CL: https://codereview.chromium.org/2064323005 ? @pkasting: Hey, would you mind checking the above issue and see if the above issue is anyway related to your change ?

Feel free to route the above issue to concern dev, if that is not the case.

I appreciate your help.

Thank you!

Based on the blame range, pretty sure this is indeed r400184.  It may have allowed the instructions text on the empty bookmark bar to shift upward enough in Hindi that the semitransparent divider drawn above the omnibox dropdown would overlay the top pixel.  The "expected result" image in comment 0 is misleading because it was taken not from just before the CL in question landed, but from before MD top chrome was turned on, and in that case the divider above the omnibox looked different.

We should first verify that Hindi text is being positioned where we want it, i.e. the text is not 1 px "too high" without the omnibox dropdown open.  If that's OK, then we either need to shift the dropdown up (probably not desirable), make the divider opaque, or draw in such a way that the text isn't seen (e.g. erase the text when the dropdown is open).

I think Evan did the transparent divider, CCing him.

@estade: As per above comment, request you to please take a look into the issue.

Thanks.!

I'm happy letting kylixrd take a crack at it :)

Ok, so what is the appropriate fix for this? I'm also still confused as to how my change caused this... especially since the text was actually moved *down*. If anything it should have been more visible prior to my change.

I did my best to describe the possible fixes in comment 13.  It depends how Hindi text is actually being positioned.  I would first take a look at that before and after your change and just see how it looks.

one other possibility not mentioned in #13 would be to make the bbar taller when it's showing Hindi text. I don't want to special-case Hindi, and I don't know how it's currently sized, but ideally larger text should make the whole bar taller (what happens when you adjust your system font size?). This may not be the easiest way to fix this problem as I expect it would require a lot of care not to break stuff (themes?).

The Hindi characters in the font are taller. If I grow the bookmark bar height by 2px(DIP?), the effect is no longer seen. I could look into shifting the text down in the view, but then it wouldn't be vertically centered.

They're taller than before your change?  Was that an intentional effect?

I grew the bookmark bar and the font height didn't change. It did shift the text down enough (remaining vertically centered) that it was no longer visible when the omnibox drop-down was displayed. Western characters/fonts are not as tall and never exhibit this behavior.

For Hindi, GetPreferredSize() returns 20 for the height. For English, it returns 16.

How is the font size for the bookmark bar being chosen?  In the omnibox in pre-MD we derive a font size based on the available height.  Seems like you may need to do something similar here so as to pick a smaller font for Hindi.

For the BookmarkBarInstructionsView, the label uses GetDefaultFontList(), which culls the font out of the resource bundle. No other calculations are being performed. Maybe the resource bundle is the problem?

The resource bundle doesn't know what constraints are on the font.  It's reasonable to expect we may want a smaller font in a place like the bookmark bar than we would be using in general.

I would look into FontList::DeriveWithHeightUpperBound() to clamp the height to something that will fit in the bar without having to enlarge the bar.  You could also try just enlarging the bookmark bar, but beware of how this might affect custom font alignment between the toolbar and new tab background images.

I was able to force the Hindi text to be near the same size as the English text for the BookmarkBarInstructionsView... however as you can see all the various Hindi text is larger (tab text, infobar, apps button). For the labels controls which make up the BookmarkBarInstructionsView, there is no specific code to set the size of the font. If anything, the fonts should be larger since the label height is larger than the preferred text height. 

For the internal RenderText and FontList objects, they use 0 for the size. It appears that those objects then a default font size, which is larger for Hindi than English.

Deleted: AdjustedHindiTextSize.png 78.8 KB

	AdjustedHindiTextSize.png 78.8 KB View Download

Note also, that none of the changes made here (https://codereview.chromium.org/2065653002) affected the Apps Page button. Its text still peeks out above the omnibox dropdown. As estade@ said, this was caused by the turning on MD.

M53 Stable launch is coming soon.Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix asap so it gets chance to bake in beta before stable promotion. Thank you.

As i see it we have a few options; 

1. Leave things as-is. 
2. Force the font size to a specific value on the bookmark bar.
3. Grow the height of the bookmark bar. 
4. Shift the location of the omnibox drop-down.

2 would be weird since all Hindi text is larger. 3 or 4 seem to be the best, low-impact, solutions. 

Opinions? Comments?

Unless there are objections, I don't think this should be marked as a release blocker.

4 might look weird if you're on a mostly-white page (e.g. the NTP) because it might look like the toolbar bottom was wiggling upward a tiny bit.

Check with msw about this as he's the one who last went around on how we should draw Hindi text.

The omnibox is deliberately positioned to slightly overlap the location bar by 2px. Changing that to 3px will eliminate this issue. Any objections to this?

Deleted: OmniboxDropDownHindi.png 77.5 KB

	OmniboxDropDownHindi.png 77.5 KB View Download

The gap inside the dropdown is higher in your screenshot than the gap above it, which looks unbalanced.

I'd think we'd want to overlap by 1 px instead of 2 (so we can just replace the toolbar bottom separator with the one in the dropdown?) but Evan and Sebastien have more context on the design goals here.

I'm confused on the naming here . 
The omnibox (dropdown) overlaps the toolbar by 2px, not the omnibox. So that means that if we want to cover Hindi, we would need to overlap the toolbar by +1px up, right ?

We would need the dropdown to overlap the toolbar by 3 px, or we would need to make the bookmark bar at least 1 px taller in at least the Hindi case, or we would need to shrink the Hindi font size or ignore the bug.

My comment 33 was saying that, Hindi aside, the fact that we overlap by 2 px instead of 1 today seems wrong, so if anything I'd rather go down to 1 than up to 3.

I like the current spacing, can we shrink the font size without affecting the UI or killing accessibility ? Hindi characters have way higher ascenders

See comment 26 for what happens if we clamp the Hindi text size inside the bookmark bar instructions view and make no other changes.

I think matching the content separator to the omnibox border (what you are suggesting in 36) is what we do on mac. I'm not opposed to that on Windows. However it increases the issue at hand, now showing 1 more pixel of ascenders underneath the omnibox.

In the end, I'd be more inclined to do nothing or increase the attached bookmark bar only for Hindi but I do not like special casing and I know we are very conservative when it comes to adding pixel. 

If overlapping by 1 px instead of 2 doesn't show anything for non-Hindi, my ideal solution for this would probably be:

(1) Change the overlap from 2 to 1
(2) Expand the bookmark bar by however much the font size exceeds the expected size

Doing (2) will handle not only Hindi but any other case of strange font metrics, a11y issues from users with huge font sizes, etc.

I actually like that very much. 

Unable to repro this issue on Windows 7 & Windows 10 [Pro] for Google Chrome Canary Version - 54.0.2817.0 

Screen-recording is attached.

@vvishwakarma: Could you please perform the steps mentioned beneath and let us know your observations.

1. Update your Canary - 54.0.2817.0 
2. Re-test the same on a clean profile [chrome://settings -> Add Person]

Thank you.

Deleted: 622678.mp4 519 KB

	622678.mp4 519 KB View Download

pkasting@, sgabriel@, I've got a CL almost ready that does #2 from comment #40. Should be ready for review today or tomorrow.

mimmagadda@, be sure you have the bookmark bar visible & attached and set the language to Hindi (in chrome://settings).

If we have a CL to grow the bookmark bar based on text size, that's exciting from an a11y point of view. I'm still worried that it could make some themes look bad, so it would be good to test with a few.

estade@, Yes, it will need serious scrutiny. I'm not 100% confident in it's efficacy in all cases.

@kylixrd: Friendly Ping! We are nearing M53 stable release, we need to get this issue fixed before the same.

Note: If this issue is not a blocker bug the feel free to remove the blocker label.

I really appreciate your help.

Thank you!

I'd be inclined to remove the RBS label as it's a minor graphical glitch only present in certain locales (or just one locale?). I'm particularly worried about merging this patch back to m53 since we'd have little chance to catch regressions.

I'll leave that decision to others, though.

I agree, this is not RBS

Still able to repro this issue on Chrome Canary Version - 54.0.2835.0

Please don't post "still broken" comments when the bug hasn't been marked fixed or had a patch posted, and thus is expected to still be broken.

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

commit 510b6ef721d3b695d5b11dac659d09e5cbc13a5f
Author: kylixrd <kylixrd@chromium.org>
Date: Wed Sep 07 23:20:18 2016

Sometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed.

BUG= 622678 

Review-Url: https://codereview.chromium.org/2208973003
Cr-Commit-Position: refs/heads/master@{#417113}

[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/bookmarks/bookmark_bar_constants.h
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_constants.h
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/views/bookmarks/bookmark_bar_view.h
[modify] https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f/chrome/browser/ui/views/frame/browser_view.cc