1524857 - URL alignment in location bar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Keul]

Attachment: URL incorrectly displayed.jpg

The location bar does not display the TLD first.

On Firefox for desktop, the domain is displayed in black and the rest in grey. Because domain name is more important than any other part of the URL.

On Firefox mobile, the domain name is partially masked while it could have been displayed in full before displaying subdomain or the rest of the URL.

Note than if the domain is not fully displayed, we should be certain that the fact that it's truncated is displayed.

What should be done:
1° display the URL from left to right
2° If URL overflow, do the overflow on the left, the .TLD/ should always be visible on the right
3° if the subdomains overflow, display them while keeping the TLD visible, try to display the rest of the URL too if there things after the /

Comment 1

[Jan Henning [:JanH]]

What you suggest is in fact what should normally happen, but it seems that currently we don't run domain highlighting when the page load ends up in an error page, and the URL justification code depends on the domain highlighting in order to right-justify the TLD.

Desktop apparently always does the eTLD+1 highlighting, so I guess that's what should actually happen on Android as well.

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1524857 - Part 0: Use short form where possible for defining properties. r?Gijs

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1524857 - Part 1: Separate base domain for doorhangers from base domain used for domain highlighting. r?Gijs

Currently, the Android front-end uses a tab's base domain both for permission
doorhangers, as well as for doing domain highlighting in the URL bar. The base
domain in turn is based on the document's nodePrincipal's URI.

As per bug 1325955, the nodePrincipal is the right choice for permission
prompts, but it causes some problems for domain highlighting instead: For error
pages for example, the nodePrincipal's URI will be some variety of
about:neterror, which means that the front-end won't be able to do any domain
highlighting based on that, since
a) we don't generate any baseDomain anyway because the URI's scheme isn't
HTTP(S)/FTP, and
b) even if we did, the nodePrincipal's baseDomain won't match the contents of
the URI displayed in the URL bar.

Therefore, we want to separate these two concerns, and generate two baseDomains:
One based on the nodePrincipal for use in permission doorhangers and the like,
and one based on the display URI, which going forward will power our domain
highlighting.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1524857 - Part 2: Use display URI's base domain for domain highlighting. r?snorp

That way, domain highlighting (and therefore the URL justification code that
right-justifies the TLD within the URL bar) can run even on error pages.

This also means that the workaround from bug 1479311 for blocking javascript:
URIs from being highlighted in ToolbarDisplayLayout is no longer required -
the base domain for domain highlighting is now being generated from the same
URI that actually ends up being displayed in the URL bar, and as such the
existing checks in browser.js for only generating a base domain for HTTP(S)/
FTP-URIs, but not any other schemes, finally work the way they are intended.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Jan, once this lands, we may want to uplift to beta, if you think it will not be too risky.

Comment 6

[Jan Henning [:JanH]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #5)

Jan, once this lands, we may want to uplift to beta, if you think it will not be too risky.

Should be possible.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/858b4a542bf4
Part 0: Use short form where possible for defining properties. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d32c9ce0d366
Part 1: Separate base domain for doorhangers from base domain used for domain highlighting. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f110ae482730
Part 2: Use display URI's base domain for domain highlighting. r=snorp

Comment 8

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/858b4a542bf4
https://hg.mozilla.org/mozilla-central/rev/d32c9ce0d366
https://hg.mozilla.org/mozilla-central/rev/f110ae482730

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 9041241 [details]
Bug 1524857 - Part 2: Use display URI's base domain for domain highlighting. r?snorp

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

URL bar

User impact if declined

Domain highlighting and right-aligning of the TLD won't work when the load ends up in an error page

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Existing code for generating a base domain has simply been extracted into a helper function, so we can now generate a base domain for URL bar domain highlighting that is based on the the URI that's actually being displayed in the URL bar. Everything else has remained unchanged.

String changes made/needed

none

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Jan, all 3 patches or just part 2?

Comment 11

[Jan Henning [:JanH]]

All three.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9041241 [details]
Bug 1524857 - Part 2: Use display URI's base domain for domain highlighting. r?snorp

Fix for url display in some edge cases, OK to uplift for beta 7.

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0cfbcd1d35c4
https://hg.mozilla.org/releases/mozilla-beta/rev/7c5414a3d4b0
https://hg.mozilla.org/releases/mozilla-beta/rev/8b10cbe1b887

Comment 14

[Stefan Deiac]

Hi, verified as fixed on the latest version of Beta 66.0b7 and Nightly 67.0a1 (2019-02-12).
Devices:

• OnePlus 5T (Android 9);
• Samsung Galaxy S8 (Android 8.0).

Due to that, I'll mark this issue as verified, thanks.