771380 - Large gap between reader mode icon and padlock icon

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[:Margaret Leibovic]

Attachment: screenshot

See screenshot.

Comment 1

[Sriram Ramasubramanian [:sriram]]

There are some issues with padding too.
I guess, the layout should my @dimen/browser_toolbar_height X @dimen/browser_toolbar_height to have it clickable, but still compress the padding -- when 2 or more are present there.

Comment 2

[Sriram Ramasubramanian [:sriram]]

@Ian,
Can't we just restrict to "one" icon on the right?

Comment 3

[:Margaret Leibovic]

This looks like a regression from bug 739364. I think it's also causing the popup to be anchored from the wrong spot.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> @Ian,
> Can't we just restrict to "one" icon on the right?

Restricting to one icon is a bit hard since one of the icons is the https lock icon, which should always be shown if active.

Bug 734877 also proposes allowing add-ons to add their own icons (page actions) to the urlbar. If more than one icon is shown, I suggest we use a dropdown arrow and popup to show the list of page actions. That would mean a max of 2 visible icons in the urlbar.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Could we do something like this:

(www.example.com LR)

Where L is the lock icon, and R is the reader icon. R would be clipped, only showing half of it. Tapping on R would slide out the various page action icons in the horizontal direction, covering the URL such as...

(www.example LRABC)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: screenshot #2 of bug

not sure if this is covered by this bug or not, but the reader icon also happens to cover the page title and overlap with it very often. see this screenshot for an example.

Comment 7

[:Margaret Leibovic]

The fix for this can probably be combined with a fix for bug 768349.

Comment 8

[Sriram Ramasubramanian [:sriram]]

@lucasr:

In case you've started working on this, there is some discussion going on moving the lock to be over / next to favicon. Please check the channel to get the updated method of solving this.

Comment 9

[Ian Barlow (:ibarlow)]

Attachment: Proposed design of lock overlay on favicon

Hi all, as Sriram mentioned there has been some discussion over email and IRC about how best to handle this problem. 

We could fine tune the space between the lock and reader icon, and make sure text doesn't overlap the icons, but doing so would introduce a different set of problems: the two icons would likely be too close together and we would end up with a lot of accidental taps on the wrong thing.

I propose we actually return to a similar interaction we had in XUL, where tapping the *favicon* displays site security info. We can still use blue and green locks for EV/SSL, but in this case we would treat them as small tiles that are offset on top of the favicon. 

A mockup is attached.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ian Barlow (:ibarlow) from comment #9) 
> I propose we actually return to a similar interaction we had in XUL, where
> tapping the *favicon* displays site security info. We can still use blue and
> green locks for EV/SSL, but in this case we would treat them as small tiles
> that are offset on top of the favicon. 

Can we use this as an opportunity to bring parity between Android and Desktop Firefox? I'm not sure what value the favicon brings in this context.

If we instead replace the favicon with a {globe, lock, EV/SSL lock} then we can:
1) fix this bug
2) remove the security bug that allows sites to use a lock as their favicon to trick their visitors
3) further "clean" our UI by removing many pixelated favicons from the location bar

Comment 11

[Ian Barlow (:ibarlow)]

The difference on Android is that the URL bar is the only place in the UI that we show the favicon, whereas desktop still has the favicon displayed in the tab, thereby freeing up space in the URL bar to only show security related UI.

As for pixelated favicons, I'm with you there in that they drive me crazy. But I would prefer looking at ways of optimizing the graphics better when they scale, possibly by not anti-aliasing them which is what makes them blurry now.

Comment 12

[Wesley Johnston (:wesj)]

(In reply to Ian Barlow (:ibarlow) from comment #9)
> I propose we actually return to a similar interaction we had in XUL, where
> tapping the *favicon* displays site security info. We can still use blue and
> green locks for EV/SSL, but in this case we would treat them as small tiles
> that are offset on top of the favicon. 
Should we show something in the no ev/ssl state as well then, to prevent spoofing?

Comment 13

[Ian Barlow (:ibarlow)]

I would prefer not to, since the whole idea behind offsetting the lock tile to extend beyond the edge of the favicon area makes a design that can't be spoofed.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ian Barlow (:ibarlow) from comment #13)
> I would prefer not to, since the whole idea behind offsetting the lock tile
> to extend beyond the edge of the favicon area makes a design that can't be
> spoofed.

I'm not sure how that prevents spoofing. I could either make my websites favicon a lock (which will be bigger and look *more* secure than the legitimate icons), or I could make my site's favicon have an offset lock in it.

When viewed over HTTP it will appear to users as though it is being viewed over HTTPS.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

I forgot to add earlier that the benefit I see for favicons in the tabs on Desktop is to know at a quicker glance which tab you want to click on when switching tabs. Switching tabs in Fennec is done much differently and uses screenshots of the webpage right now.

Comment 16

[Ian Barlow (:ibarlow)]

(In reply to Jared Wein [:jaws] from comment #15)
> Switching tabs in Fennec is done much differently and uses
> screenshots of the webpage right now.

True, but we use favicons in the awesomescreen, so the correlation between what you see there and what you see here is still useful.

Another additional security measure we could take, which Wes suggested in IRC, was possibly colouring the page title on secure sites as well.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

FWIW, we have the same inconsistency on desktop Firefox with results in the suggestions in the awesomebar having the favicon while the location bar doesn't.

I don't think the inconsistency is too much pain when compared to the other wins.

Comment 18

[Ian Barlow (:ibarlow)]

Jared, that's true, but the favicon is still used in the tab graphic on desktop. In other words, there is still *some place* for a visual identifier to be displayed in the primary UI of the desktop browser, other than the content area. This is something we want to preserve on mobile, as well, but we have less space in which to do so.

--

Here are the basic requirements as I see them:

1. Showing the favicon. On desktop we do this in the tab graphic. On mobile, we show it in the URL bar, given the absence of visible tabs.

2. Click to the left to the page title for site info. On desktop, we only show the lock or globe. On mobile, we need a way to communicate this while still fulfilling #1.

3. Create a clear security indicator that appears somewhere in the vicinity of the page title, but in such way that it cannot be spoofed by evil sites. On desktop, this is simply the lock or globe. On mobile, we need to explore more design options for how to combine this design with the favicon in a smart, safe way.

--

I am all for consistency across platforms, but in this case that consistency does not have to manifest itself in an identical UI on phone and desktop. Rather, we should be providing similar functions that will probably look different, since they are optimized for different kinds of hardware.

I will continue to work on some new security indicator designs (part 3), and share some more designs here soon.

Comment 19

[Sriram Ramasubramanian [:sriram]]

One large question:
Why do we want a favicon anyways?

Possible reasons:
1. Visual identifier for the tab
2. Security identifier for the website
3. Identifying them in AwesomeBar.

Possible counter-arguments:
1. "Visual identifer" is needed when we have more things of same kind and we need to identify something.
On desktop, all tabs are visible in the same time (in the tab-bar), and hence these favicons serve a purpose. On mobile, only one favicon is shown at a time, and hence this doesn't qualifier to be an identifier for selecting tabs -- as tabs-tray doesn't have it.
2. It can qualify as a "visual identifier" only if it is associated with thumbnails in tabs-tray.
3. Thumbnails serve the purpose of these identifiers in tabs-tray -- which removes the need for favicons.
4. It cannot be a "security identifier" -- as in, I know this favicon stands for BoA or Chase or something as phishing sites can spoof the favicons too. That's the reason for site-security locks to show the certificates. Hence the idea of treating them as "security identifier" doesn't hold right.
5. When I navigate to awesomebar, I see 15 entries with Mozilla favicon or 15 entries with google favicon. How can it by itself identify a single row uniquely? Or, how does the favicon on the tab I see helps me correlate with the list I see here? I anyways have to look through the entire row -- parse characters -- before selecting a row. Favicon is a nice image (often pixellated though) in this huge list of results.

Which all makes me feel -- let's kill favicons in URL bar.

Comment 20

[Aaron Train [:aaronmt]]

Ian/Madhava .. ?

Comment 21

[Ian Barlow (:ibarlow)]

Attachment: mockup of padlock placement

I've given this a lot of thought, and here is my assessment of the situation.

* I think we all agree that we want to use the limited space available in the title bar in as smart a way as we can, and not have any "dead" touch zones like we do in our current release version, where the favicon provides no additional information when touched. From a UX perspective, the most logical and useful-to-the-user solution would be to roll the favicon and identity menu touch area together into one thing.

* UX feels strongly about leaving the favicon in the title bar, as it provides space for website authors to display their own designed content, provides a quick identifier for a website, and frankly adds some personality to the title area which is nice. 

* On the other hand, Engineering feels strongly about concerns around spoofing with this solution, the theory that favicons are less useful in this UI than they are on desktop, and is generally more in favour of creating parity across platforms with the lock / globe icons in the URL bar.  

* I suggested that maybe there is another way where we can all be happy, and roll the favicon and lock indicators into the same touch area, in such a way that is clear to the user, can not be spoofed, and is still pleasing to the eye. 

--------

Well, I tried a number of options and they all totally sucked.

So where does this leave us? As I see it, from a UX perspective we are left with a couple of options:  show both a favicon, AND a lock somewhere else in the URL bar, which is basically where we are now, or remove the favicon and save some space. 

I think we should go with the latter: remove the favicon, and replace them with locks for EV / SSL. Let's keep the Globe away from the mobile UI, though. The only real purpose it serves on desktop is to provide a draggable element for people that want to drag a page into the bookmarks bar, which is a use case we don't need to support. We can better use that extra space here by moving the title to the left, which will buy us a few more characters.

--------

I'm not very happy about this move, nor is anyone in UX, but I will admit that we are also in a state where we are all starting to think about new features like Reader Mode, where Add-ons live, etc, and it would be wise to take this opportunity to start future proofing our UI as best we can, should we decide later that we want to add something else to our title bar. 

To that end, I even want to start exploring alternate interactions for switching to Reader Mode that might allow us to remove the icon from the URL bar. I will be filing a separate bug to track this.

Comment 22

[Lucas Rocha (:lucasr)]

Ian, could you please post the new asset for the lock icon?

Comment 23

[Ian Barlow (:ibarlow)]

Attachment: Updated lock icons

Note - I have also slightly tweaked the colour and shading on the other URL bar icons (Reader Mode, Stop, Go, Search), so please replace these assets along with the new locks. Thanks!

Comment 24

[Lucas Rocha (:lucasr)]

I'm already working on this one, sorry.

Comment 25

[Lucas Rocha (:lucasr)]

Ian, I implemented the suggested behaviour (from comment #21) but it doesn't feel good IMO. The urlbar lacks spatial stability (sometimes there is an icon on the left, sometimes on the right, sometimes it's just text). There's just too many moving elements in a rather small space. Furthermore, we need to a define where doorhangers will point to now that the favicon image is gone. Maybe one more design iteration to cover these issues?

Here's a (rough) test build so that you can try it yourself:

  https://dl.dropbox.com/u/1187037/urlbar.apk

Comment 26

[Ian Barlow (:ibarlow)]

Alright, I agree I'm seeing a few issues here, some that were also brought up last week as well as some new ones:

* The URL bar behaves erratically when a page is loading. The spinner pushes the text over when it appears, and then moves it back to the left edge when the page finishes loading. 
* When title text changes position, there is no transition from one location to another so it feels very janky.
* If a page has plugin content, the doorhanger either points to nothing, or the lock icon, both of which are misleading. 
* Also, you are still using the old urlbar icons in this build. Please make sure you replace them with the ones in comment 23. 

Having said all that, I think we are generally pointed in the right direction. I'll put together some suggested tweaks that cover these issues. Just be prepared for a solution that may be a little more complex to resolve than we initially anticipated, since it could affect the spinner, doorhangers, possibly the reader mode switcher, etc.

Comment 28

[Lucas Rocha (:lucasr)]

Attachment: Reorganize toolbar layout to better handle dynamic icons

Comment 29

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 652832 [details] [diff] [review]
Reorganize toolbar layout to better handle dynamic icons

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

This looks good to me.
Please add a contentDescription to mAwesomeBar, whenever title changes, as this would be needed for Accessibility.

Comment 30

[Lucas Rocha (:lucasr)]

Ian, it feels like the the lock icons should be smaller than it is right now. The new icons seem to have the same size than the current one. I guess you're preparing assets for it?

Here's a test build so that you can try it:
http://dl.dropbox.com/u/1187037/new-toolbar-layout.apk

I probably need specs for the placement of the new/smaller lock icon. Let me know of any layout tweaks you want to make.

Comment 31

[Marco Zehe (:MarcoZ)]

Yes, the contentDescription is important or the button won't be discoverable. Let me know if you need any help with that.

Comment 32

[Lucas Rocha (:lucasr)]

Attachment: Reorganize toolbar layout to better handle dynamic icons

Comment 33

[Lucas Rocha (:lucasr)]

Attachment: Use animation to show/hide lock icon in toolbar

Comment 34

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 654632 [details] [diff] [review]
Reorganize toolbar layout to better handle dynamic icons

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

This looks good to me.

Comment 35

[Lucas Rocha (:lucasr)]

Had to change a few major things in the patch to fix the site identity popup. Here's the new version for review. I also implemented the animation to show/hide the lock animation. Here's a test build:

http://dl.dropbox.com/u/1187037/ibarlow-goodness-final.apk

Comment 36

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 654634 [details] [diff] [review]
Use animation to show/hide lock icon in toolbar

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

>>> slideWidth += (siteSecParams.leftMargin + siteSecParams.rightMargin) * scale + 0.5f;
I would probably want to move the margin too into XML file and take it from there, instead of scale.
And please use mActivity to get the resources, and please refrain from using GeckoApp.mAppContext. :(

Animation Listener can be set globally, say at BrowserToolbar level. Now that we have all the animation as local variables,
you can do,
onAnimationStart(Animation animation) {
   if (animation.equals(mLockFadeIn)) { ... }
}

and so on. There by we don't need many listeners. (More like "body" capturing all events and passing to required elements).

I would like to see the changes before r+ ing them :(

Comment 37

[Sriram Ramasubramanian [:sriram]]

Overall the patch looks good to me.

Comment 38

[Lucas Rocha (:lucasr)]

Attachment: Use animation to show/hide lock icon in toolbar

Comment 39

[Lucas Rocha (:lucasr)]

I prefer to keep the margin handling in code (instead of creating dimens for the margins) because the margin values are defined in relation to the favicon's in the layout files. We better keep things defined in the layout files directly to avoid confusion.

Comment 40

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 654707 [details] [diff] [review]
Use animation to show/hide lock icon in toolbar

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

Awesome! This looks clean. :)

Comment 41

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52453ab440ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9059056119

Comment 42

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out because it appeared to cause this robocop failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb25861ffd9

testAboutPage | page title match - Expected /About (Fennec|Nightly|Aurora|Firefox|Firefox Beta)/, got ""
https://tbpl.mozilla.org/php/getParsedLog.php?id=14645542&tree=Mozilla-Inbound

Strangely, this occurred just once as an intermittent failure earlier today (bug 785039), but then became perma-orange after these patches landed.

Comment 43

[Sriram Ramasubramanian [:sriram]]

If this is looking at url bar, this will fail for sure. We need to change the tests that depend on Button having the "title".

Comment 44

[Marco Zehe (:MarcoZ)]

If I read the patch for this bug correctly, it looks like we'll end up with a similar problem as with the tabs, in that we get duplicate info with both the text view and the actual button. See bug 784748. So we may want to apply the technique there to the text view that holds the page title, too.

Eitan, am I reading this right?

Comment 45

[Marco Zehe (:MarcoZ)]

As I suspected, we get duplicated content. To fix that, after the line:


        mTitle = (TextView) mLayout.findViewById(R.id.title);

add:

        mTitle.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_NO);

Also, the awesome button only gets a contentDescription when there is an actual page title. It should *always* get one, e. g. also when the mTitle is "Search or enter an adress". In addition, the favicon, now being clickable, must always have a contentDescription, too.

Comment 46

[Lucas Rocha (:lucasr)]

Pushed with a small change to failing test, which seems to make try happy:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fde4b39fa708
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5db456160a

Comment 47

[Lucas Rocha (:lucasr)]

Comment on attachment 654632 [details] [diff] [review]
Reorganize toolbar layout to better handle dynamic icons

[Approval Request Comment]
User impact if declined: The toolbar layout is pretty much broken for dynamic icons right now. Reader icon shows up on top of awesomebar title on phones and have a long empty space from the lock icon. This patch sanitizes all interactions with the the browser toolbar (proper clickable areas everywhere, correct spacing between elements, etc) and fixes all issues with dynamic icons such as the reader one.
Testing completed (on m-c, etc.): Local testing, no regressions found. a11y team tested the patch and approved it.
Risk to taking this patch (and alternatives if risky): Any changes to browser toolbar involve some risk. Let's wait until Monday to confirm that everything is working fine. But I think we have to uplift this anyway because, as I said, the toolbar layout is broken for reader icon at the moment.
String or UUID changes made by this patch: None.

Comment 48

[Lucas Rocha (:lucasr)]

Comment on attachment 654707 [details] [diff] [review]
Use animation to show/hide lock icon in toolbar

[Approval Request Comment]
User impact if declined: The new toolbar layout shows the site identifty lock icon on the left side of the favicon. It's important to shows proper visual feedback when it shows up. This animation is very important to implement the intended design and interaction with the toolbar.
Testing completed (on m-c, etc.): Local tests shows no regressions, let's get this in m-c until Monday to confirm nothing obvious regressed.
Risk to taking this patch (and alternatives if risky): Changes to toolbar always involve some risk. The patch is not tiny but is not complex either.
String or UUID changes made by this patch: None.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fde4b39fa708
https://hg.mozilla.org/mozilla-central/rev/7e5db456160a

Comment 50

[Marco Zehe (:MarcoZ)]

I can verify that the accessibility of the landed patch is correct in the 2012-08-26 build of Nightly 17.0a1. Leaving the status on "Resolved" so Aaron or someone else can verify the visual and animation bits and set the status field accordingly. Accessibility is good!

Comment 51

[Lucas Rocha (:lucasr)]

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/80edc958e20d
https://hg.mozilla.org/releases/mozilla-beta/rev/caf449802692

Comment 52

[Cristian Nicolae (:xti)]

This issue is fixed on the latest Nightly.

--
Firefox 18.0a1 (2012-08-29)
Device: Galaxy Note
OS: Android 4.0.4