1078508 - Design and implement doorhanger for unidentified security state (new tablet)

Status: RESOLVED FIXED

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

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Similar to desktop so when the not-identified globe is clicked (bug 1071267), we display a "This website does not supply identity information." doorhanger, or similar.

Any thoughts on design and strings, Anthony?

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hmmm. I'll NI me to think about this more but for the time being, parity with Desktop does seem like a good place to start (in terms of string). 

I can't seem to recall what that looks like on Mobile ATM, have you got a screen shot handy perhaps?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Security doorhanger

Note that this image has some WIP patches and should be slightly different than the current build.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I'd like to give the top and bottom some more padding to free it up a bit (double what we have currently). But, for V1, let's aim for parity first and polish doorhangers as a part of a larger effort so we can include mobile devices too.

How does that sound?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sounds good to me.

Just to be clear: I'm taking no action on V1 here.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #4)
> Sounds good to me.
> 
> Just to be clear: I'm taking no action on V1 here.

Awesome. FYI https://bugzilla.mozilla.org/show_bug.cgi?id=1058818 is where the meta bug is filed to clean up doorhangers. :) I've also engaged in conversations with Desktop side so we can keep some consistency there as well (they're looking to possibly improve theirs).

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Spoke IRL: antlam okayed us to implement something similar to desktop.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Take a SiteIdentity as a parameter to SiteIdentityPopup.updateUi

Small cleanup: Params are easier to test and more predictable than using member variables.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Take a SiteIdentity as a parameter to SiteIdentityPopup.updateUi

Changed something I didn't mean to.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Change SiteIdentityPopup.updateUi to use a switch statement

Not strictly necessary, but seemed a little more readable and safe (i.e. IDE's missing case warnings).

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Take a SiteIdentity as a parameter to SiteIdentityPopup.updateUi

Rebase.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8521406 [details] [diff] [review]
Part 2: Change SiteIdentityPopup.updateUi to use a switch statement

No longer relevant because rebase.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add "unknown site identity" popup

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Unknown site identity

Anthony, what do you think?

...vroom, vroom.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

On comment 12: due to a bad hg qref error, I merged p1 w/ p2 - oops! Both are in the patch in comment 12 and still pretty readable. See comment 7 for motivation on taking the SiteIdentity parameter in updateIdentity.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add "unknown site identity" popup

Really, really bad with the qref these days.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add "unknown site identity" popup

On about: pages, desktop shows "This is a secure Firefox site" or similar - we
were showing "This site has an unindentified identity." or similar, which is
misleading and bad. Part 2 fixes this by not showing a doorhanger.

Really, really, really bad at qref, so this is included here.

Added a followup for implementing the "This is a secure Firefox site", as per
desktop. See bug 1099088.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8522944 [details] [diff] [review]
Add "unknown site identity" popup

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

Looks good. Just need the missing layout file to give r+.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +474,5 @@
>  with that structure, consider a translation which ignores the preceding domain and
>  just addresses the organization to follow, e.g. "This site is run by " -->
>  <!ENTITY identity_run_by "which is run by">
> +<!ENTITY identity_no_info "This website does not supply identity information.">
> +<!ENTITY identity_not_encrypted "Your connection to this website is not encrypted.">

I assume we're just using the same strings than desktop?

::: mobile/android/base/resources/layout/site_identity.xml
@@ +21,2 @@
>  
> +        <include layout="@layout/site_identity_unknown" />

Where's this layout? Forgot to include in the patch? I was going to suggest stubbing this bug it's probably not worth it.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +101,5 @@
> +
> +        mIdentity.setVisibility(View.VISIBLE);
> +        mContent.setPadding(0, 0, 0, 0);
> +
> +        final boolean isIdentityKnown = siteIdentity.getSecurityMode() != SecurityMode.UNKNOWN;

nit: enclose expression with parens.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add "unknown site identity" popup

-_-

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8522958 [details] [diff] [review]
Add "unknown site identity" popup

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

Yep.

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8522924 [details]
Unknown site identity

Looks likes it's a good place to start! :) I'm happy with this for V1.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add "unknown site identity" popup

Updated for nits.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #17)
> I assume we're just using the same strings than desktop?

Yep.

> Where's this layout? Forgot to include in the patch?

Added.

> nit: enclose expression with parens.

Done.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add "unknown site identity" popup

Apparently forgot some imports.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/3ddd0d4aa067

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ddd0d4aa067