tedchoc/mgiuca:

I've been testing on Android with your test URL: https://goo.gl/gfEaLi

On Chrome Stable, I get what you see in Screenshot #1.

This seems to be because Android has a scrollToTLD method that tries to get to the end of the TLD.

I'm guessing this is to prevent google.com.foo.bar.malicious.com from looking like just google.com on narrow-width devices.

If I make scrollToTLD a no-op that always returns false, I get what you see in Screenshot #2.

Since scrollToTLD seems like it will be part of any Android solution, I have a few questions that probably tedchoc@ can answer:

 1. Is my guess as to why scrollToTLD exists correct? I can't see the whole history because this code was imported from clank (doesn't have full blame history).

 2. Is scrollToTLD still relevant? (Looks like it's pre-2015) We don't seem to implement anything similar on Desktop and phones have gotten a lot bigger.

 3. I'm guessing the scrollToTLD implementation as-is doesn't work correctly with RTL URLs, and the correct fix (if we need to keep scrollToTLD) is to make it work with RTL.

 4. mgiuca, Your screenshot in the master bug doesn't seem to have scrolled to the TLD for some reason... any idea why?

Thanks,

Tommy

Deleted: Screenshot_2017-05-16-16-53-47.png 70.4 KB

	Screenshot_2017-05-16-16-53-47.png 70.4 KB View Download

Deleted: Screenshot_2017-05-16-16-53-51.png 60.2 KB

	Screenshot_2017-05-16-16-53-51.png 60.2 KB View Download

Put on your mining hat, we're going digging.

scrollToTLD was originally added in (jan 2012):
https://chrome-internal-review.googlesource.com/#/c/10832/

FWIW, I changed it recently to be called for https sites as well:
https://codereview.chromium.org/2672513002

Not at all surprised that scrollToTLD doesn't play nicely with RTL as Android in general doesn't play all that nicely.  All it does is sets the selection to the end of the TLD and crosses it's fingers that Android brings the point into View.  To me, it looks like it is actually doing that, but it isn't doing it in the way you'd expect where it would attempt to put the selection at the end of the visible region thus showing all of the domain.

scrollToTLD definitely could use enhancement as setSelection isn't really what we want.  We want to give a selection range and say "ensure the end of the selection is visible but bias showing what is before it".  I don't think such a thing exists on Android and in my attempts at fixing this in the past, I have run into a bunch of issues with timing.

I believe I even have an Android studio app where I started trying to make even the LTR case better, but I gave up as it was "hard".

But yes, it is still needed, and yes, it very much should be improved.

#2: "mgiuca, Your screenshot in the master bug doesn't seem to have scrolled to the TLD for some reason... any idea why?"

Which screenshot? The original reporter's screenshot in Comment 1 (NOT mine)? Or one of the many small clips I posted? (NONE of those capture the default behaviour, they're all explanations of experiments I did.)

It's not exactly clear what "scroll to TLD" means. If it works the way it does on Desktop, scrolling "to" the TLD is incorrect behaviour, as explained by  https://crbug.com/656417#c8 . On Desktop, "scroll to TLD" means "make it so the back edge of the first character is in view" (i.e., the dotted red line in that screenshot). If the domain is RTL, that can mean we scroll until the RIGHT edge of the domain is aligned to the LEFT edge of the text field, meaning the entire domain is off the left side of the screen and totally invisible.

Do you know if Android elides the Omnibox text when it's not focused? That's what Desktop does. I'm just now discovering that my fix on Desktop is all wrong and in fact there's a one-line fix: simply set the scroll offset to 0. On Desktop, when the text field is deselected, the text is elided to fit the width of the text field. The advantage of eliding is that it naturally works with RTL, because unlike scrolling/clipping, it doesn't chop off the LEFT or RIGHT side of the text, but it chops of the END of the text. So the fix is to just elide, and then make sure there is 0 scroll offset.

For example, if you have (ASCII-hack-RTL) "ABC.COM/0123abcd", which normally renders as "0123/MOC.CBAabcd" (I know, this is a disaster), it will elide to "ABC.COM/012…" which renders as "…012/MOC.CBA". That means if you elide to the size of the text field, the domain will always be visible on screen as it is the FIRST part of the text (regardless of whether it's on the left, right or somewhere in the middle).

FWIW, I don't have access to the master bug, so I'll just assume what the images look like.

scrollToTLD in android attempts to move the selection to the end of the TLD (the back edge of the last character of the TLD is to be made visible)

Android does no eliding on the omnibox (unless the URL is >1000/4000 characters for performance reasons and then it in the middle of the full text).  As we rely on Android for our text layout, finding the exact pixel offset of certain characters can be annoying (I can only imagine the pain BiDi would introduce).

mgiuca: Hey, I was referring to your Android screenshot that didn't seem to scroll to the TLD at all: https://bugs.chromium.org/p/chromium/issues/detail?id=656417#c24

Both: Hmm, it seems that Android and Views have different logic as to which parts of the URL should be shown in the Omnibox... That's kind of discomfiting.

I will be looking in the code more and discussing with jdonnelly, but do you guys have any idea if iOS implements a similar scrollToTLD logic? It seems unfortunate that these fixes may be entirely platform-specific and will require a lot of work?

I'm starting to get the feeling that mgiuca's "display all URLs in memory order" proposal is looking better and better. I saw pkasting@ wrote "Just Do It" in a previous bug.

Do we have plans to Just Do It? And do you guys need any help making the memory-order proposal happen?

tedchoc: I cced you on the master bug so you can see.

tedchoc: When you were working on this, did you have any issues using setSelection(0)?

For me, setSelection seems to work as expected unless either argument is 0, then it appears to no-op.

My intuition says the correct call is probably setSelection(0, urlComponents.first.length()) but it doesn't work. setSelection(1, urlComponents.first.length()) seem to work as expected though...

Hmm...I don't recall seeing anything crazy with setSelection(0), but I wouldn't be surprised at all if Android optimizes in that state where the selection potentially isn't changing from what it internally thinks it should be.

Some links into the android code:

How selection is set:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/text/Selection.java#72

Where TextView starts to hear about it:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/widget/TextView.java#10392

All the logic about how it looks at the span:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/widget/TextView.java#8266

So setting selection to setSelection(0/1...first.length()), Android attempts to set the end of the selection visible but biases showing what is before it?  If so, that does indeed sound like a great option.

To recap a chat I had this morning with Tommy:

> Do we have plans to Just Do It? And do you guys need any help making the memory-order proposal happen?

Yes, but a) I am not full time on this so it may take awhile, b) it will likely be behind a flag until we get some wider agreement, and c) that will not fully solve this bug.

Consider a URL (ASCII hack, uppercase letters represent RTL script):

Memory order:
ABC.DEF.GHI/012/345/678/jkl/mno/pqr

Display order (under current scheme):
012/345/678/IHG.FED.CBA/jkl/mno/pqr

Not sure what Android does but the Views implementation (given an extremely narrow Omnibox) will show only "/jkl/mno/pqr" because it aligns the left edge with the 'A'.

If we change to the proposed all-components-ltr format, it will display like this:
CBA.FED.IHG/012/345/678/jkl/mno/pqr

Now the algorithm that aligns to the left edge of the 'A' will show ".FED.IHG/012/345/678/jkl/mno/pqr", which is an improvement, but it still clips off the least-significant domain label. So I think we should still fix this independent of Issue 351639.

tedchoc/mgiuca:

I did a bit more experimentation. It seems we want to do:

        bringPointIntoView(0);
        bringPointIntoView(urlComponents.first.length());

setSelection(x, y) seems to behave equivalently to only bringPointIntoView(y), which seems to be consistent with what onPreDraw says.

So those two consecutive bringPointIntoView calls would work -- EXCEPT that we also have a call within UrlBar.java to ApiCompatibilityUtils.setTextDirection(this, TEXT_DIRECTION_LTR);

For some reason, and I haven't figured it out, if either we have that TextDirection to LTR flag on (or if the string starts with an LRM), cursor position 0 is always interpreted as all the way to the left.

When I comment out the ApiCompatibilityUtils.setTextDirection(this, TEXT_DIRECTION_LTR), those two calls work perfectly, and we always display the most significant part of the URL, in both LTR and RTL domains...

But we can't just do that, since that would violate the IETF guidelines on URL display.

At this point I'm posting my findings to get some feedback from you rather than continuing to bang my head against this wall. :)

Reading more into the body of bringPointIntoView and Layout.getPrimaryHorizontal, cursor position 0 in LTR mode should indeed be all the way to the left... since that's where a new LTR character would be inserted. It seems correct, but unfortunate for our particular use case.

We really want to know the point of character 0 rather than offset 0.

Is it this implementation:
https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/android-25/android/widget/TextView.java?q=bringPointIntoView&l=7461
(Don't even know how to begin reading that...)

> For some reason, and I haven't figured it out, if either we have that
> TextDirection to LTR flag on (or if the string starts with an LRM), cursor
> position 0 is always interpreted as all the way to the left.

That just seems wrong. Just because a string starts with an LRM or has its top-level text direction set to LTR doesn't mean its first character starts on the left.

Example (using ASCII hack):

Memory:
"ABCDefgh"

Display (with auto paragraph direction):
"efghDCBA"

Display (with LRM inserted prefix, or top-level text direction LTR):
"DCBAefgh"

[NOTE: THE BIDI UTILITY IS DOWN OMG so I am doing this by hand. May be wrong.]

In both cases, the first character 'A' is not aligned to the left. This only changes the *top-level* direction, not all of the text direction.

> cursor position 0 in LTR mode should indeed be all the way to the left...
> since that's where a new LTR character would be inserted

Oh I think I understand: it's saying what's the position of the LTR character, not the 'A'? Indeed... that is on the left side but not particularly helpful. Is there a way to skip over any bidi mark characters and ask it to select the first normal char (in this case, the position of character 1)?

mgiuca/tedchoc:

Yes, it's that implementation. Inside, it calls Layout.getPrimaryHorizontal which is defined as "This is the location where a new character would be inserted in the paragraph's primary direction."

So I think for your example (with spaces added): 

Display (with LRM inserted prefix, or top-level text direction LTR):
           D C B A e f g h

Column     3 2 1 0 4 5 6 7
Offset    0 3 2 1 4 5 6 7 8

I think this is because the calculation assumes you're inserting an LTR character... since the top level text direction is LTR. That's my best theory of what's going on.

Anyways, in my CL, i just ended up using offset 1 instead of 0. I think it's a simple enough hack that at least improves the status quo.

Screenshots attached of 1. Your example URL, and 2. A really long LTR URL (correctly scrolled to the end).

Note in screenshot 1, you see the edge of the next character (which is the LTR extent of the path that appears after the RTL domain and RTL path). Like I said it ain't perfect, but I don't think we'll reach perfection without implementing the in-memory-order URL display thing...

Deleted: Screenshot_2017-05-19-10-45-09.png 58.6 KB

	Screenshot_2017-05-19-10-45-09.png 58.6 KB View Download

Deleted: Screenshot_2017-05-19-10-45-27.png 76.7 KB

	Screenshot_2017-05-19-10-45-27.png 76.7 KB View Download

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

commit d5a340817ee90fb66e5e23316311987e8f92e1ee
Author: tommycli <tommycli@chromium.org>
Date: Fri May 19 19:12:04 2017

Android UrlBar: Fix scrolling to most-significant part of RTL domains.

Fix isn't perfect. See the comment and bug, but it's decent.

BUG= 723100 

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

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

Yay! Fixed?

Yes! It should be fixed!

(It can't be completely fixed / perfect until we do the in-memory-layout URL thing, but in my testing this fixes the most egregious part where the actual origin is not visible).

Though actually mgiuca, would you be able to double-check my fix works in Canary 3106.0 and later? -- since you are the original reporter.

I did some playing in Chrome Canary (60.0.3108.3). There is one case that doesn't work properly:

1. Navigate to http://xn--nebl.xn--9dbq2a/0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
(i.e., the one with a very long sequence of digits on the left of the domain, and a very long sequence of Latin letters on the right of the domain).
2. Select the Omnibox (keyboard appears).
3. Press the down arrow to close the keyboard.

The Omnibox scrolls all the way to the left, and the domain is not visible.

This bug doesn't manifest upon navigation, so it isn't a huge deal, but is still one of the originally reported cases. (I don't really consider that a security risk, since it involves user interaction with the Omnibox so won't be seen just as you navigate around.)

I tested this with the system language in English and Hebrew, it's the same for both.

I don't think you need to go to any particularly complex URL to see this manifest itself.  If you just do a really long google search and do 2 & 3 above, you'll see that it is scrolled to the right after defocusing.

If you just scrub the omnibox a bit to the left and right, you'll see it correct itself.  I think this is the problems I was having with bringPointIntoView in that it requires the text to be laid out before it works and it can be delayed in these weird ways.

Ah that is unfortunate. How do you guys want to proceed on this.

My opinion is that if it's not a big deal, we should push on the in-memory-layout URL display and perfect the display logic after we do that.

#22 is that a regression after r473269 or was it always the case?

It is a regression.  I think a pretty visible one too :-/.

Eek.. re-opening bug.

I'll take a look at this and see if I can preserve the fix without introducing that regression.

If you guys prefer, we can also revert the previous CL, but I'd recommend letting me see what I can do first.

Tommy

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

commit 5f32dc08811c84362be404b8ce7a1d5b2100dd80
Author: tommycli <tommycli@chromium.org>
Date: Fri May 26 00:16:05 2017

Android UrlBar: Fix regression introduced by previous scrolling fix.

Fixes regression introduced by:
https://codereview.chromium.org/2892203002

Still preserves the fix.

BUG= 723100 
TEST=MANUAL

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

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

Fixed?

Fixed!

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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