New issue
Advanced search Search tips

Issue 721927 link

Starred by 3 users

Issue metadata

Status: Archived
Owner:
Closed: Sep 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 711342



Sign in to add a comment

Title y position does not match

Project Member Reported by jasonkliu@chromium.org, May 12 2017

Issue description

See the bottom half of go/mlrmb.  This concerns the alignment of the secondary bubble (seen below in the two screenshots) with the 'Bookmark added' bubble.

60.0.3088.44: https://drive.google.com/open?id=0B3dPCXKQYa2dQUoyZ1VIVzZyaG8 
Correct: The title text is not shifted to the right. 
Broken: The title text is slightly below the center, which causes the 'y' in Sync to be slightly cut off (by 1px or so).

59.0.3071.47: https://drive.google.com/open?id=0B3dPCXKQYa2dNWZrOGlCXzFIWEk
Correct: The title text vertical (y) position may be correct, though it's hard to tell because the text is on two lines.
Broken: The title text is shifted to the right, which causes two lines of content.  This is fixed in M60.
 
Looks like 1px is clipped off the y at the bottom.  No other "tall" letters in the messaging. 
Problem exists in 60.0.3080.6 (as well as one of the latest versions of 59).  I think it may be related to the spacing change.

Comment 3 by mrefaat@google.com, May 15 2017

i think there is ongoing effort from harmony to update the title spacing, so i'm not sure if we should do anything about that. 
Is there a Harmony owner that we should assign this to?  Thanks.
Summary: Title y position does not match (was: The "y" is cut off. )
Cc: pkasting@chromium.org
Can this bug be updated so it clearly states things like what's "correct" and "broken" without having to read other threads for reference?  Otherwise I don't know offhand what I'm helping with.  (I went and dug into the UI review thread, but it'd be good to not make every reader do that.)

As to why this looks wrong, I'd have to look at the code that implemented it to find out what it's doing.  There are many ways to do things in views, and most of them are subtly incorrect, so while we're trying to make the system easier to use over time, for now the answer is likely "make sure a Harmony eng codereviews all native UI that anyone adds to the product".
Description: Show this description
Updated description to make it clearer; thanks for the heads-up.

Here is the view used for the bubble: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
Huh, this code is strange.  It creates its content as a child that is added to a different parent bubble (e.g. the bookmark bubble) instead of being a standalone dialog, which would be much easier to do in a standard way.  Do you know why that is?

It looks to me like this code isn't even responsible for the bubble title, but rather that's being drawn by higher-level code, meaning I'd expect that any change here in M60 would also be visible on a variety of other bubbles in M60.
> Huh, this code is strange.  It creates its content as a child that is added to
> a different parent bubble (e.g. the bookmark bubble) instead of being a
> standalone dialog, which would be much easier to do in a standard way.  Do you
> know why that is?
mrefaat can answer this in more detail

> It looks to me like this code isn't even responsible for the bubble title, but
> rather that's being drawn by higher-level code, meaning I'd expect that any
> change here in M60 would also be visible on a variety of other bubbles in M60.
Yes, changes to the existing bubbles can break our implementation. For example,
see https://docs.google.com/document/d/14hT69R4o3A3C0Chs4hW-CkIVYTBO6caLLsW33klErL0/edit
The alignment seen above in #0 was filed as  crbug.com/710212 , which had root cause  crbug.com/711012 .
I think the ios promo should be its own bubble. We have more changes coming for the bookmarks bubble:  Issue 726187  and  Issue 651652 .
Status: Archived (was: Assigned)
Archiving old bugs that have only received trivial updates for some time.

If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!

Sign in to add a comment