1168134 - URL Bar should color the domain differently from the rest of the URL

Status: RESOLVED FIXED

(Firefox for iOS :: Theme & Visual Design, defect)

Description

[Darrin Henein [:darrin]]

As seen in the mockup here http://invis.io/FC25FA153 as well as Firefox on Android, we should color the url accordingly for better contrast and visual parsing.

Comment 2

[Stephan Leroux [:sleroux]]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/598

Set colors to be default dark grey/light grey.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8621642 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/598

Looks good! I'm not sure we want to mess with the scheme display, though, as mentioned in the PR (looks like Darrin's mockup also includes "https://").

Comment 4

[Stephan Leroux [:sleroux]]

So I updated the patch to include changes in the PR comments and refactored out the logic for coloring the URL into extension methods on NSURL and NSAttributedString so I could wrap some of the behavior in tests. To color the host without the subdomains, I'm currently using the last two components of the host, joining them, and coloring that in hopes of excluding any subdomains. Would this work as a workaround? If not, I can look into using the tld.dat list to create special handling and wrap that in some additional tests if needed.

Comment 5

[Wesley Johnston (:wesj)]

We don't want to break things like www.bbc.co.uk . I think thats why things got complex originally...

Comment 6

[Stephan Leroux [:sleroux]]

Would it be difficult to emulate what Android does? I can probably port over whatever checking logic we do in Java over to Swift.

Comment 7

[Wesley Johnston (:wesj)]

Android uses Gecko's code, so its basically the same as desktop.

Comment 8

[Stephan Leroux [:sleroux]]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/598

I managed to find http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsEffectiveTLDService.cpp#224 in Gecko which is the code thats responsible for determining the public suffix/base domain for a given hostname using the public suffix list found here: https://publicsuffix.org/. I implemented the algorithm in Swift as an extension to NSURL so we can match the domain highlighting logic we have on Android/Desktop. I also included some basic test cases to confirm the algorithm's logic.

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8623277 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/598

Couple of regressions:

We show https:// again in front of the URL. (But not http://) Not sure if that was a design decision but before this patch we did not?

If you open https://www.apple.com/ only apple.com is darker grey. Is that intentional?

Opening a new tab shows the URL of the previously active tab in the location bar instead of "Search or enter address" - Not sure if that is this bug or the recent refactor of this url bar?

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8623277 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/598

Code looks great. Nice to have that TLD code in the project, I think that will be useful for other things too. Just see my regression notes in the previous comment.

Are the performance test data (.xcbaseline) supposed to be committed? I don't know how that stuff works.

Comment 11

[Stephan Leroux [:sleroux]]

> We show https:// again in front of the URL. (But not http://) Not sure if that was a design decision but before this patch we did not?

Just spoke with Darrin and he said to go with this behavior as it matches what we do on desktop

> If you open https://www.apple.com/ only apple.com is darker grey. Is that intentional?

Yup - Firefox Desktop does the same thing.

> Opening a new tab shows the URL of the previously active tab in the location bar instead of "Search or enter address" - Not sure if that is this bug or the recent refactor of this url bar?

This is actually a regression thats on master at the moment.

Comment 12

[Stephan Leroux [:sleroux]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1175792 for URL regression bug.

Comment 13

[Stephan Leroux [:sleroux]]

Should I wait on merging this until we fix the URL regression on master?

Comment 14

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Nope lets merge it!

Comment 15

[Stephan Leroux [:sleroux]]

Merged