New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 631422 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

[RTL] Omnibox alignment jumps when focusing/unfocusing omnibox

Project Member Reported by bauerb@chromium.org, Jul 26 2016

Issue description

Chrome version: 54.0.2796.0

Steps to reproduce:
1) Set an RTL device locale
2) Enable scrolling on the New Tab Page in Chrome, e.g. by enabling snippets in chrome://flags or turning the device to landscape
3) Open the New Tab Page
4) Focus the omnibox and unfocus it. Note that the text is right-aligned both in the search box on the NTP and in the omnibox.
5) Scroll the NTP so that the search box transforms into the omnibox at the top. Note that the omnibox now is left-aligned.

Screen recording is attached. Step 4 seems to be necessary to trigger this -- before you do step 4, scrolling the NTP will keep the alignment.
 
Cc: mgiuca@chromium.org
You didn't attach the recording.

Comment 2 by mgiuca@chromium.org, Jul 27 2016

Does this have anything to do with scrolling? Or is it just that the text alignment flips when focused / defocused? If so, this is Issue 616702.

Comment 3 by bauerb@chromium.org, Jul 27 2016

D'oh, here's the recording.

Yes, it could be that the omnibox simply flips its alignment when it gets unfocused, although here it's not depending on the URL, yet there is some state that gets messed up. Maybe that'll be fixed with issue 616702 anyway though.
2016_07_26.mp4
5.2 MB View Download

Comment 4 by bauerb@chromium.org, Jul 27 2016

Cc: -mgiuca@chromium.org
Labels: -Type-Bug -Pri-3 M-53 Pri-1 Type-Bug-Regression
Owner: mgiuca@chromium.org
Status: Assigned (was: Available)
Summary: [RTL] Omnibox alignment jumps when focusing/unfocusing omnibox (was: [RTL] Omnibox alignment jumps when scrolling on NTP)
So, looks like this is actually a regression caused by r400359 :-(

Matt, assigning to you in the hope that you can fix this as part of issue 616702.
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 27 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by mgiuca@chromium.org, Jul 28 2016

Status: Started (was: Assigned)
OK thanks for reporting, I've got a fix:
https://codereview.chromium.org/2182183008

But now that I look, I see that I may have messed up the concept of mUrlDirection so maybe I'll take a closer look at that before sending out that fix.

Comment 7 by fi...@chromium.org, Jul 28 2016

Labels: zine-triaged
Labels: -M-54 -MovedFrom-53 ReleaseBlock-Beta M-53
This is a regression in M53, right? Can this be fixed before the next M53 Beta?
Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Nope, it can't - we're already a week late in promoting, had planned to release today, and this would push us into next week.  The workflow to get into this state requires multiple steps, and the text appears to still be RTL aligned, just on the wrong side of the screen.

Thus marking this RB-Stable for M53.
Good call, thanks Alex for re-assigning this as a Stable blocker!
Sorry, this fell off my radar. I'll try to land the above fix in the next few days (likely early next week).
Awesome, thanks Matt!
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 9 2016

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

commit 1d266aa00775a9e9307c7d342fb6354ba7707012
Author: mgiuca <mgiuca@chromium.org>
Date: Tue Aug 09 00:15:55 2016

Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

Also fixes incorrect text direction when restoring a closed tab.

BUG= 631422 
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.

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

[modify] https://crrev.com/1d266aa00775a9e9307c7d342fb6354ba7707012/chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java

Labels: Merge-Request-53
Status: Fixed (was: Started)
Requesting a merge for M53 (fix for regression).

Comment 15 by dimu@chromium.org, Aug 9 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 9 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7f029f303598ce9e0e143859325c48d1043a21b

commit e7f029f303598ce9e0e143859325c48d1043a21b
Author: Matt Giuca <mgiuca@chromium.org>
Date: Tue Aug 09 03:15:53 2016

Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

Also fixes incorrect text direction when restoring a closed tab.

BUG= 631422 
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.

Review-Url: https://codereview.chromium.org/2182183008
Cr-Commit-Position: refs/heads/master@{#410510}
(cherry picked from commit 1d266aa00775a9e9307c7d342fb6354ba7707012)

Review URL: https://codereview.chromium.org/2225283002 .

Cr-Commit-Position: refs/branch-heads/2785@{#540}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/e7f029f303598ce9e0e143859325c48d1043a21b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java

Status: Verified (was: Fixed)
Works as per expected behavior, Issue tested on Nexus 7 / 54.0.2825.0 and 53.0.2785.57 

Sign in to add a comment