Closed Bug 1271998 Opened 9 years ago Closed 7 years ago

Proposal: Scrollable URL in URL bar

Categories

(Firefox for Android Graveyard :: General, defect, P2)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(relnote-firefox 57+, firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
relnote-firefox --- 57+
firefox57 --- verified

People

(Reporter: sebastian, Assigned: JanH)

References

Details

Attachments

(7 files)

proposal-concept.png
(deleted), image/png
Details
Bug 1271998 - Part 0 - Clean up imports.
(deleted), text/x-review-board-request
Details
Bug 1271998 - Part 1 - Provide a ScrollView with a more efficient fadingEdge implementation.
(deleted), text/x-review-board-request
jwu
: review+
Details
Bug 1271998 - Part 2 - Make our URL bar scrollable.
(deleted), text/x-review-board-request
jwu
: review+
Details
Bug 1271998 - Part 3 - Scroll the URL to focus the origin for overlength URLs.
(deleted), text/x-review-board-request
jwu
: review+
Details
Bug 1271998 - Part 4 - Use a touch delegate to increase the clickable area of the URL bar.
(deleted), text/x-review-board-request
jwu
: review+
Details
Scrollable URL bar with LWT
(deleted), image/png
Details
Attached image proposal-concept.png (deleted) —
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?
(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.
(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.
Anthony has been working on an update for the URL bar. We should consider adding such a functionality into the new version.
Priority: -- → P2
Assignee: nobody → jh+bugzilla
(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)
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)
(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.
Also see bug 1379552 about adding the fade back.
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 on attachment 8903298 [details] Bug 1271998 - Part 0 - Clean up imports. https://reviewboard.mozilla.org/r/173328/#review180156
Attachment #8903298 - Flags: 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.
Attached image Scrollable URL bar with LWT (deleted) —
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.
Good point. I had looked at LWT, but not closely enough to notice this - back to the drawing board on that one I guess...
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.
For now I've fixed this to work with the semi-transparent background, since I think it doesn't actually look bad.
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 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 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 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 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 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+
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.
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.
Attachment #8903299 - Flags: review?(walkingice0204)
Attachment #8903300 - Flags: review?(walkingice0204)
Attachment #8903302 - Flags: review?(walkingice0204)
Attachment #8903301 - Flags: review?(walkingice0204)
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)
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)
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
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: