1290490 - Firefox shouldn't RTL the first item in the URL dropdown list when typing a URL

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Itiel]

User Agent: Mozilla/5.0 (Windows NT 10.0; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

1. Install Hebrew version of Firefox Developer Edition 49.0a2 (or possibly any RTL version of Firefox having the new design of the URL bar)
2. a. In the URL bar, type "www.google.com"
   b. In the URL bar, type "google." (with a dot after 'google')
3. Observe the first item in the dropdown list, the one having "Visit" after the URL and the second with "Search with Google"


Actual results:

Firefox autocompletes A to "/http://www.google.com", and autocompletes B to ".google" (both being RTL'd)
Notice the '/' and the dot's locations.


Expected results:

A and B should have been "http://www.google.com/" and "google.", respectively.
In short, they should be LTR.

Currently it works okay in 47.0.1 (partially, as the "Search with Google" string is incorrectly LTR'd, which is fixed in the new design of the URL bar).

bug 1267355 is somehow related.

Comment 1

[Itiel]

That's still an issue in latest builds.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1290490 - Firefox shouldn't RTL the first item in the URL dropdown list when typing a URL.

Review commit: https://reviewboard.mozilla.org/r/86220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86220/

Comment 3

[Drew Willcoxon :adw]

Thanks for filing.

This fixes www.google.com but not "google.".  It looks like the latter has not been recognized as a URL in the urlbar since Firefox 41, due to this changeset (bug 1107883): https://hg.mozilla.org/mozilla-central/rev/e67ddc64b010#l4.50

The problem is that fixupInfo.keywordAsSent is "google.", so _matchUnknownUrl returns early and does not add a visiturl match for it.  That seems like an intentional change made by that bug.

Marco reviewed that patch though, so maybe he can comment on that part.

Comment 4

[Itiel]

(In reply to Drew Willcoxon :adw from comment #3)
> This fixes www.google.com but not "google.".  It looks like the latter has
> not been recognized as a URL in the urlbar since Firefox 41

That.. actually makes sense. Now that I re-think about it, Firefox shouldn't assume that "google." is a start of a URL.
If the attached patch fixes "www.google.com/", I'm good with closing this bug as RESOLVED FIXED at this point.

Thank you!

Comment 5

[Drew Willcoxon :adw]

All right, thanks for your comment.  Is it possible for you to try the following build in an RTL language so you can verify that the patch works?

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-d6024e03410676c51536ffec8e8e35917be40774/

(Depending on how quickly you check this link after I posted it, it may not have been created yet, or the builds may not have finished yet.)

Comment 6

[Itiel]

(In reply to Drew Willcoxon :adw from comment #5)
> All right, thanks for your comment.  Is it possible for you to try the
> following build in an RTL language so you can verify that the patch works?
> 
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> d6024e03410676c51536ffec8e8e35917be40774/

It's a bit of a problem to verify whether a RTL issue is fixed or not on a forced-RTL build using the Force-RTL addon. I've seen bugs where the addon forced-RTL the UI anyway, when the issue wasn't actually fixed.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8801468 [details]
Bug 1290490 - Firefox shouldn't RTL the first item in the URL dropdown list when typing a URL.

https://reviewboard.mozilla.org/r/86220/#review85566

off-hand looks like it should work.

::: browser/base/content/browser.css:462
(Diff revision 1)
>    -moz-binding: url(chrome://browser/content/urlbarBindings.xml#urlbar);
>  }
>  
>  /* Always show URLs LTR. */
> -.ac-url-text:-moz-locale-dir(rtl) {
> +.ac-url-text:-moz-locale-dir(rtl),
> +.ac-title-text[isurl]:-moz-locale-dir(rtl) {

nit: isurl is quite strong and we won't ever be 100% sure, I'd name it lookslikeurl just in case.

::: toolkit/content/widgets/autocomplete.xml:1860
(Diff revision 1)
>              // the popup is open.
>              this._removeMaxWidths();
>            }
>  
>            let title = this.getAttribute("title");
> +          let isTitleUrl = false;

...and titleLooksLikeUrl

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8801468 [details]
Bug 1290490 - Firefox shouldn't RTL the first item in the URL dropdown list when typing a URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86220/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ce14ff1acd5
Firefox shouldn't RTL the first item in the URL dropdown list when typing a URL. r=mak

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3ce14ff1acd5