1250671 - URL bar: Consider showing full URL on tablets

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

On tablets we have much more space to show the (full) URL. Spoofing as described in bug 1236431 might not be an issue here.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

+1

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

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

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #0)
> On tablets we have much more space to show the (full) URL. Spoofing as
> described in bug 1236431 might not be an issue here.

This is more questionable for 7" tablets in portrait mode – should we consider limiting by orientation and/or screen size?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

https://reviewboard.mozilla.org/r/36219/#review33879

lgtm pending UX review.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #3)
> (In reply to Sebastian Kaspari (:sebastian) from comment #0)
> > On tablets we have much more space to show the (full) URL. Spoofing as
> > described in bug 1236431 might not be an issue here.
> 
> This is more questionable for 7" tablets in portrait mode – should we
> consider limiting by orientation and/or screen size?

In SF we decided to show the full URL on all tablets and orientations. However it will always be possible to move the base domain off the screen with a long subdomain - if we show the full URL. The question is whether this looks suspicious to the user or if this could be used to trick the user into thinking the browser is connected to a different domain.
We have two - sometimes contradicting - goals here: Showing the user the location of the page and/or showing the user /who/ the browser is connected to. On phones we started to rank the "who" higher now. On tablets we try to do both but this can be hard.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeah, I think we should still continue to show the URL in the URL bar here since we have the tabs on top as well. 

I don't think there's much from the work we did on the mobile side that affects tablets immediately. So, as we discussed, we can probably continue as we are on Tablets unless we hear more feedback otherwise.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

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

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

I'll re-request review because I changed the code: The previous patch did not restore the two-color URL on tablets. There's no reason to drop this functionality.

Additionally I started to reuse the ForegroundColorSpan for the certificate case (Your suggestion from bug 1249594) after seeing that we keep all the other spans too.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

https://reviewboard.mozilla.org/r/36219/#review35203

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:146
(Diff revision 2)
> +        mUrlColor = new ForegroundColorSpan(ColorUtils.getColor(context, R.color.url_bar_urltext));
>          mBlockedColor = new ForegroundColorSpan(ColorUtils.getColor(context, R.color.url_bar_blockedtext));
> +        mDomainColor = new ForegroundColorSpan(ColorUtils.getColor(context, R.color.url_bar_domaintext));
> +        mPrivateDomainColor = new ForegroundColorSpan(ColorUtils.getColor(context, R.color.url_bar_domaintext_private));
> +        mCertificateOwnerColor = new ForegroundColorSpan(ColorUtils.getColor(context, R.color.affirmative_green));

nit: rename -> `*Span`. Ending the name with `*Color` makes me think these are ints!

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:309
(Diff revision 2)
> +    private void updateTitleFromFullURL(String url, String baseDomain,boolean isPrivate) {

nit: a name like, `updateAndColorTitleTitleFromFullURL` might be more descriptive.

nit: space after ,

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:320
(Diff revision 2)
> +        index, index + baseDomain.length(), Spannable.SPAN_INCLUSIVE_INCLUSIVE);

nit: rb might be screwing with me, but this should be indented.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

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

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/386a3f51df04cea0ebe8971167a94d7920d8082e
Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

Approval Request Comment

[Feature/regressing bug #]: Displaying of base domain introduced in bug 1236431 (Firefox for Android 47).

[User impact if declined]: We decided to only show the base domain on mobile phones. On tablets we have more space and want to continue showing the full URL.

[Describe test coverage new/current, TreeHerder]: Local testing on a Nexus 9.

[Risks and why]: Low - This is more or less the same code we have been using before the patch in bug 1236431 landed.

[String/UUID change made/needed]: -

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/386a3f51df04

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8722755 [details]
MozReview Request: Bug 1250671 - Show full URL in URL bar on tablets. r=mcomella

Seems like a good idea, taking it.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ecdc38c5ea39

Comment 16

[Flaviu Cos]

Verified as fixed in builds:
- 48.0a1 2016-03-10;
- 47.0a2 2016-03-11;
Device: Asus ZenPad 8 (Android 5.0.2).

Comment 17

[Daniel]

The following either A: breaks the way the EVSSL thing works or B: disables the showing of the name instead of the url in the urlbar.

'''security.ssl.enable_ocsp_stapling''' = '''false''' 
'''security.OCSP.enabled''' = '''0'''

And so FF doesn't grey out parts of the url & blocks the showing of the http:// part I also DISABLED both of these:
'''browser.urlbar.trimURLs'''
'''browser.urlbar.formatting.enabled'''
 
As you can see in my screenshot it no longer shows the name, it now shows the full URL.