bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Proposal: Scrollable URL in URL bar

VERIFIED FIXED in Firefox 57

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: sebastian, Assigned: JanH)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 57+, firefox57 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Reporter)

Description

2 years ago
Created attachment 8751297 [details]
proposal-concept.png

Problems:
* (1) The current release version hides the origin for long URLs: Vulnerable to spoofing (bug 1236431)
* (2) Users want to see the full URL or as much as possible (bug 1268753)

Solution in Nightly/Aurora/Beta:
* We show only the origin (or the owner of the EV certificate)
-> Problem 1 solved.
-> Problem 2 worsened: Even less parts of the URL are visible.

Proposal:
(See attached image for a visual explanation):
* We show the full URL again
* We make the URL scrollable (for the app AND the user)
* Initially we scroll the URL to reveal the origin (if needed)
* The user can scroll the URL to reveal whatever other parts are important
-> Problem 1 solved
-> Problem 2 solved and situation improved over current release version


I hacked a prototype and that's what I created the screenshots with. Nevertheless it's tricky to get this right and this needs more work.
I think this is a good start!

My main questions would be:

1) to confirm, does this change remove what we're currently doing for the "all good" sites? i.e. will we show "Mozilla Foundation (US)" or the full URL like you've shown there?

2) how should we determine if we "need" to center the origin? perhaps only if it's completely or more than 50% on screen? 

3) for sites with longer origins, we'll be hiding the "https://" or "http://" section of the URL for the sake of the origin. But if we have this scrolling gesture, we might not need to center align it?
(Reporter)

Comment 2

2 years ago
(In reply to Anthony Lam (:antlam) from comment #1)
> 1) to confirm, does this change remove what we're currently doing for the
> "all good" sites? i.e. will we show "Mozilla Foundation (US)" or the full
> URL like you've shown there?

This is a very good question. I omitted this from the proposal because we can solve this later (it's an improvement not related to the spoofing bug and we haven't shipped it yet). But right now I see two options:

A) We drop it. The same information is in the doorhanger when clicking the lock. So it's not completely lost.

B) We prefix the URL with the cert owner (basically what desktop does; but we need a 'mobile design') instead of replacing the URL completely. In this case we probably wouldn't scroll the URL initially and show the owner + the beginning of the URL.


(In reply to Anthony Lam (:antlam) from comment #1)
> 2) how should we determine if we "need" to center the origin? perhaps only
> if it's completely or more than 50% on screen? 

I actually do not center the origin but align it right (The subdomain is a little bit more important than the path). Additionally I move it a little to the left if there's more text after it. Just so that it's not completely in the part we fade out. If it looks centered in the screenshots than this is just a lucky coincidence (screen size <-> domain length).

For the decision, here's what I do:

* Measure size of text up to including base domain (W)
* If W is larger than the available space (A):
  * Scroll x=W-A

This is simplified. There are additional calculations to incorporate padding and the fading left/right.


> 3) for sites with longer origins, we'll be hiding the "https://" or
> "http://" section of the URL for the sake of the origin. But if we have this
> scrolling gesture, we might not need to center align it?

I don't fully understand what you are saying. Right now (release) we strip "http" and only show "https" and other protocols afaik.
(Reporter)

Updated

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> I actually do not center the origin but align it right (The subdomain is a
> little bit more important than the path). Additionally I move it a little to
> the left if there's more text after it. Just so that it's not completely in
> the part we fade out.

Nice. This is similar to what I originally proposed in bug 1236431, but with adding the URL back. Good thinking on the slight move to the left: that signals the user there is more to be seen.

Updated

2 years ago
Blocks: 1284046
(Reporter)

Comment 4

2 years ago
Anthony has been working on an update for the URL bar. We should consider adding such a functionality into the new version.
Priority: -- → P2
Duplicate of this bug: 863845
(Assignee)

Updated

11 months ago
Assignee: nobody → jh+bugzilla
(Assignee)

Comment 6

11 months ago
(In reply to Sebastian Kaspari (:sebastian) from comment #0)
> Nevertheless it's tricky to get this right and this needs more work.

I might run into the same problems myself anyway, but can you still remember what exactly the tricky bits were?
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 7

11 months ago
I think mostly it was about handling touch events because this view supports "clicks", "long presses" and "drags" in this proposal.
Flags: needinfo?(s.kaspari)
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1393504
(Assignee)

Comment 9

11 months ago
(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> I think mostly it was about handling touch events because this view supports
> "clicks", "long presses" and "drags" in this proposal.

That was less of a problem than expected, and the text positioning algorithm shouldn't be too complicated, either, but I've discovered another problem: To get better scroll kinetics (and to be honest because I initially didn't realise the TextView could be made scrollable itself), I used a HorizontalScrollView, which means that for fading over length text we now use the default Android "fadingEdge" implementation instead of our custom FadedMultiColorTextView.

As mentioned in bug 1338231 comment 6, fadingEdge still has a noticeable performance penalty when looking at GPU profiles, so I guess I should also try whether I can successfully apply the Faded*TextView approach there as well.
(Reporter)

Comment 10

11 months ago
Also see bug 1379552 about adding the fade back.
(Assignee)

Comment 11

11 months ago
So far, this is working fine now, but I've been doing the fading from onDrawForeground so I can add the fading before the scrollbars get drawn (although for our case here this won't matter because I don't intend on enabling any scrollbars for the URL bar).

However I've just realised that onDrawForeground was only introduced with Android M (API 23), so I still need to provide an alternative onDraw path for previous Android versions.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
mozreview-review
Comment on attachment 8903298 [details]
Bug 1271998 - Part 0 - Clean up imports.

https://reviewboard.mozilla.org/r/173328/#review180156
Attachment #8903298 - Flags: review+
(Assignee)

Comment 18

11 months ago
mozreview-review
Comment on attachment 8903301 [details]
Bug 1271998 - Part 3 - Scroll the URL to focus the origin for overlength URLs.

https://reviewboard.mozilla.org/r/173334/#review180174

::: mobile/android/app/src/photon/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:155
(Diff revision 1)
>          mTitle = (ThemedTextView) findViewById(R.id.url_bar_title);
>          mTitlePadding = mTitle.getPaddingRight();
> +        mTitle.addTextChangedListener(new TextChangeListener());
> +        mTitle.addOnLayoutChangeListener(new OnLayoutChangeListener() {
> +            @Override
> +            public void onLayoutChange(View v, int left, int top, int right, int bottom, int oldLeft, int oldTop, int oldRight, int oldBottom) {

One thing I've overlooked here: I actually need to watch both the ScrollView *and* the TextView for size changes.

Comment 19

11 months ago
Created attachment 8903483 [details]
Scrollable URL bar with LWT

With your patches, we just found an UI issue when LWT is applied. Check the screenshot you would see the fading edge on left separates the URL address and site identity into 2 parts. 

Just a thought that if we can change the background of URL address into a single color(no transparency), it not only can fix this issue, but also makes to URL text more clear.

Let ask for designer's opinion.
(Assignee)

Comment 20

11 months ago
Good point. I had looked at LWT, but not closely enough to notice this - back to the drawing board on that one I guess...
(Assignee)

Comment 21

11 months ago
Both things should be possible - we can keep the semi-transparent background and I'll fix the fading to work with that, or we can switch to a solid background as proposed above and adapt the code for that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

11 months ago
For now I've fixed this to work with the semi-transparent background, since I think it doesn't actually look bad.

Comment 26

11 months ago
mozreview-review
Comment on attachment 8903299 [details]
Bug 1271998 - Part 1 - Provide a ScrollView with a more efficient fadingEdge implementation.

https://reviewboard.mozilla.org/r/173330/#review180876
Attachment #8903299 - Flags: review?(topwu.tw) → review+

Comment 27

11 months ago
mozreview-review
Comment on attachment 8903300 [details]
Bug 1271998 - Part 2 - Make our URL bar scrollable.

https://reviewboard.mozilla.org/r/173332/#review180878
Attachment #8903300 - Flags: review?(topwu.tw) → review+

Comment 28

11 months ago
mozreview-review
Comment on attachment 8903301 [details]
Bug 1271998 - Part 3 - Scroll the URL to focus the origin for overlength URLs.

https://reviewboard.mozilla.org/r/173334/#review180882

Part 3 looks like a new bahavior, should we file another bug and let more people to join discussing?

Comment 29

11 months ago
mozreview-review
Comment on attachment 8903302 [details]
Bug 1271998 - Part 4 - Use a touch delegate to increase the clickable area of the URL bar.

https://reviewboard.mozilla.org/r/173962/#review180886
Attachment #8903302 - Flags: review?(topwu.tw) → review+

Comment 30

11 months ago
mozreview-review-reply
Comment on attachment 8903301 [details]
Bug 1271998 - Part 3 - Scroll the URL to focus the origin for overlength URLs.

https://reviewboard.mozilla.org/r/173334/#review180882

Oops this is also part of the proposal, I should give a review ASAP.

Comment 31

11 months ago
mozreview-review
Comment on attachment 8903301 [details]
Bug 1271998 - Part 3 - Scroll the URL to focus the origin for overlength URLs.

https://reviewboard.mozilla.org/r/173334/#review180906

::: mobile/android/app/src/photon/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:448
(Diff revision 2)
> +            return;
> +        }
> +
> +        int domainEnd = text.getSpanEnd(mDomainColorSpan);
> +        if (domainEnd == -1) {
> +            // Maybe we're in private mode?

Would it make the semantic more clear by using `Tabs.getInstance().getSelectedTab().isPrivate()`?
Attachment #8903301 - Flags: review?(topwu.tw) → review+
(Assignee)

Comment 32

11 months ago
mozreview-review-reply
Comment on attachment 8903301 [details]
Bug 1271998 - Part 3 - Scroll the URL to focus the origin for overlength URLs.

https://reviewboard.mozilla.org/r/173334/#review180906

> Would it make the semantic more clear by using `Tabs.getInstance().getSelectedTab().isPrivate()`?

In keeping with the spirit of the comment at the top ("It should always be updated through a single entry point (updateFromTab) and should never track any tab events or gecko messages on its own to keep it as dumb as possible.) probably not, but I could query the [the private state of the TextView](https://dxr.mozilla.org/mozilla-central/rev/37824bf5c5b08afa7e689fceb935b8f457ebd9eb/mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedTextView.java#112) to determine which span type to query.

Updated

10 months ago
Duplicate of this bug: 1379552
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 39

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a54f17a9022c :(

We're already have a noticeable amount of crashes in testSessionOOMSave (bug 1396324) and it seems that these patches make it considerably worse. I'm going to test whether turning the fading off helps here - if yes, I'm considering splitting that off into a separate bug and landing the scrollable URL bar without the fading enabled for now.
(Assignee)

Updated

10 months ago
Attachment #8903299 - Flags: review?(walkingice0204)
Attachment #8903300 - Flags: review?(walkingice0204)
Attachment #8903302 - Flags: review?(walkingice0204)
(Assignee)

Updated

10 months ago
Attachment #8903301 - Flags: review?(walkingice0204)
(Assignee)

Comment 41

10 months ago
With the workaround for testing (don't load the Activity Stream home panel during testSessionOOMSave) from bug 1396324 things look more promising:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e775468f32cb42f593fa57e8af0e7db65c4c3ae

Julian, do you still want to take a look at this, or can I go ahead?
Flags: needinfo?(walkingice0204)
(Assignee)

Updated

10 months ago
Depends on: 1396324
Yes you can go ahead! I had discussed with Jing-wei, and I think it would be great to have this.
Flags: needinfo?(walkingice0204)

Comment 43

10 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/d509047f7935
Part 0 - Clean up imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/fe02dfd16dc3
Part 1 - Provide a ScrollView with a more efficient fadingEdge implementation. r=jwu
https://hg.mozilla.org/integration/autoland/rev/c8aa9b29b278
Part 2 - Make our URL bar scrollable. r=jwu
https://hg.mozilla.org/integration/autoland/rev/953adb3e5e83
Part 3 - Scroll the URL to focus the origin for overlength URLs. r=jwu
https://hg.mozilla.org/integration/autoland/rev/ecae1c71816b
Part 4 - Use a touch delegate to increase the clickable area of the URL bar. r=jwu
(Assignee)

Comment 45

10 months ago
Release Note Request (optional, but appreciated)
[Why is this notable]: We now also initially scroll the URL such as to focus the base domain, users can then scroll left/right to see more if they want.
[Affects Firefox for Android]: Only.
[Suggested wording]: Long URLs in the URL bar are now scrollable.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Depends on: 1399148
Verified in 57.0b3. The URL bar is scrollable and works as expected.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified

Comment 47

9 months ago
This was added to 57 beta release notes.
relnote-firefox: ? → 57+
You need to log in before you can comment on or make changes to this bug.