Closed Bug 857165 Opened 7 years ago Closed 7 years ago

Highlight domain name when we show urls

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Whiteboard: ui-hackathon)

Attachments

(3 files, 4 obsolete files)

We should highlight the domain when we're showing urls, especially in the titlebar. The ability to show some rich text would help fight spoofing. And its parity with other platforms.

We should also probably refuse to show page titles that look like urls (for some definition of "look like").
If we do this we should also use different colors for characters from different unicode character sets, so we can also protect against IDN homograph attacks [1].

[1] http://en.wikipedia.org/wiki/IDN_homograph_attack
Attached patch Patch v1 (obsolete) — Splinter Review
This finds the domain the same way desktop does. It also strips out http:// as well as www., m., or mobile. on the front or urls (they'll still appear in the awesomescreen and will be copied). Some of that code is stolen from my patch in bug 858340. Applies on top of the patch in bug 778216.

Need to look at the idiograph stuff separately.
Attachment #735021 - Flags: review?(mark.finkle)
Attached image Screenshot normal tab
Attached image Screenshot private tab
OS: Linux → Android
Hardware: x86 → All
Attached patch Patch v2 (obsolete) — Splinter Review
Updated for the new show url's patch. I'll hold off on reviews until we've made a decision there.
Attachment #735021 - Attachment is obsolete: true
Attachment #735021 - Flags: review?(mark.finkle)
This is looks great, Wes! :-)
I just wanted to make you aware of bug 729495, where Brian did some work on removing 'http://' from URLs a while ago. There's some additional stuff over there which I'm not sure your also taking care of with your patch here, like:
- removing any trailing '/'
- showing the shortend URL also in awesomescreen entries

As a suggestion: Wouldn't it make sense to keep any URL shortening work out of this bug here (since its not really in scope given the bug summary anyway) and put that into bug 729495?
The patch there probably bit-rotted a bit but it should be mostly there...

Either way, I'm not sure about the removal of 'www.', 'm.' and 'mobile.' as I think that especially the mobile parts can be valuable information. Also Dektop Firefox does not remove 'www.' from URLs.
Comment on attachment 735583 [details] [diff] [review]
Patch v2

Review of attachment 735583 [details] [diff] [review]:
-----------------------------------------------------------------

The UI looks pretty cool btw :-)

::: mobile/android/base/BrowserToolbar.java
@@ +1216,5 @@
> +                SpannableStringBuilder builder = new SpannableStringBuilder(title);
> +                final ForegroundColorSpan gc = new ForegroundColorSpan(Color.rgb(166, 166, 166));
> +                final ForegroundColorSpan bc = new ForegroundColorSpan(tab.isPrivate() ? Color.rgb(255, 255, 255) :
> +                                                                                     Color.rgb(0, 0, 0));
> +                final ForegroundColorSpan gc2 = new ForegroundColorSpan(Color.rgb(166, 166, 166));

You would probably be better if you defined this color in colors.xml instead of hardcoding the values here.
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #735583 - Attachment is obsolete: true
Attachment #737570 - Flags: review?(mark.finkle)
Attached patch Patch v2 (obsolete) — Splinter Review
Unbitrotted. Moved to colors. Cleaned up setTitle implementation a bit based on a talk with mfinkle.

> I just wanted to make you aware of bug 729495, where Brian did some work on
> removing 'http://' from URLs a while ago. There's some additional stuff over
> there which I'm not sure your also taking care of with your patch here, like:
> - removing any trailing '/'
> - showing the shortend URL also in awesomescreen entries

Yeah. Removing the trailing slash would be nice. Fixing the tests and landing that would be great.

> As a suggestion: Wouldn't it make sense to keep any URL shortening work out
> of this bug here (since its not really in scope given the bug summary
> Either way, I'm not sure about the removal of 'www.', 'm.' and 'mobile.' as
> I think that especially the mobile parts can be valuable information. Also
> Dektop Firefox does not remove 'www.' from URLs.

Makes sense, but I don't really mind it being here. Basically what I noticed (and you will too) is that for a lot of URL's, even on my S3 which has a pretty huge high-res screen, the domain still winds up dangerously close to the right hand edge of the urlbar. Since you can't scroll there (bug 863845 which is non-trivial to fix), I'm worried the only really important thing we want to show people (the domain) will be offscreen. So I've been trying to remove junk that I think users are likely not interested in. Tapping to show the awesomescreen will give you the full, scrollable url.

As a secondary fix here, it might be nice to force the text to scroll so that the domain is always visible.
Assignee: nobody → wjohnston
Attachment #737570 - Attachment is obsolete: true
Attachment #737570 - Flags: review?(mark.finkle)
Attachment #742097 - Flags: review?(mark.finkle)
Attached patch Patch v2.1Splinter Review
Forgot to qref.
Attachment #742097 - Attachment is obsolete: true
Attachment #742097 - Flags: review?(mark.finkle)
Whiteboard: ui-hackathon
Comment on attachment 742105 [details] [diff] [review]
Patch v2.1

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+        mUrlColor = new ForegroundColorSpan(res.getColor(R.color.url_bar_urltext));
>+        mDomainColor = new ForegroundColorSpan(res.getColor(R.color.url_bar_domaintext_private));
>+        mPrivateDomainColor = new ForegroundColorSpan(res.getColor(R.color.url_bar_domaintext));
>+

nit: remove the blank line

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     this.shouldShowPluginDoorhanger = true;
>     this.clickToPlayPluginsActivated = false;
>+    // Borrowed from desktop Firefox: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#174

nit: add blank line before this comment
Attachment #742105 - Flags: review+
Comment on attachment 742105 [details] [diff] [review]
Patch v2.1

(In reply to Wesley Johnston (:wesj) from comment #10)
> Created attachment 742105 [details] [diff] [review]
> Patch v2.1
> 
> Forgot to qref.

Oops
Attachment #742105 - Flags: review+
This was unlikely the problem, but rebuilt, tested, and back in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9887910dba
https://hg.mozilla.org/mozilla-central/rev/fa9887910dba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 868342
Talked with Erin offline, the notes are fine as is and have already been up for several days so we'll leave this out.
relnote-firefox: ? → ---
You need to log in before you can comment on or make changes to this bug.