Closed Bug 480207 Opened 15 years ago Closed 15 years ago

Improve the UI of the safebrowsing blocked site overlay

Categories

(Camino Graveyard :: Security, defect)

Product:
Camino Graveyard
Component:
Security
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino2.0

People

(Reporter: alqahira, Assigned: alqahira)

References

(
URL
)

Details

(Whiteboard: l10n [camino-2.0])

Attachments

(6 files, 3 obsolete files)

WIP 1 screenshot
106.73 KB, image/png
Details
screenshot of WIP1, red-green colour blindness
65.38 KB, image/png
Details
WIP 1.1 screenshot (somewhat brighter red)
100.97 KB, image/png
Details
Better key handling
777 bytes, text/plain
Details
WIP 2
16.55 KB, patch
phiw2
: review+
stuart.morgan+bugzilla
: superreview+
Details | Diff | Splinter Review
WIP 2 screenshot
100.79 KB, image/png
Details 
Sean mentioned at this morning's meeting that he wanted to improve the UI of the blocked site overlay.  I've also been collecting some notes on improving the UI of the overlay on my Safebrowsing UI Improvements page in the wiki.

Note that some of this work will need to be done before b3 (anything that changes a string on the page, like the awful button names).
Flags: camino2.0b3?
Whiteboard: l10n
Target Milestone: --- → Camino2.0
* As part of cleaning up the Firefox-only jargon ("Web Forgery", "Attack Site"), we should improve the wording of the error messages, too.  In particular, Safari's phishing warning text reads better to me.

* One of my concerns is that with the SSL error pages, we provide a final warning not to add an exception, whereas here we don't seem to provide the same quality of warning (Safari pops up a dialogue for the malware page after you click the button).

* We need to make a decision on "Get me out of here" doing "Back" vs "Home" (with fallback to cbo/start/ if Home is blocked), or Close (which Safari does, but I don't think we want).

* I think if we keep using "Get me out of here" as a button name, we should add "!" (like Firefox 3.5); we might instead want to change to "Back" or "Home", though.

* As noted in comment 0, Sean wants to move the "action" part of the warning to a Safari-like model, with "Why" becoming a link and "Ignore" and "Get Out" as buttons, dialogue-style (at least 24px between them, since "Ignore" is dangerous).

Warning Warning Warning Warning Warning Warning Warning Warning Warning

Explanation Explanation Explanation Explanation Explanation Explanation

_Why is this site blocked?_

                         [Ignore Warning]       [[Get Me Out Of Here]]

I have a few more comments on http://wiki.caminobrowser.org/User:Sardisson/Safebrowsing_UI_Improvements#Overlay but I think I've covered most of them here.
Depends on: 479554
In bug 479554 comment 1 I posted a few thoughts on the colour combinations, and a suggestion for using grey (similar to Safari) as background.

screenshot: attachment (id=388905)
Attempted to post this last night but mid-aired with myself and didn't notice :P

(In reply to comment #1)
> * As part of cleaning up the Firefox-only jargon ("Web Forgery", "Attack
> Site"), we should improve the wording of the error messages, too.

Further, do we really care that the site Camino "has been blocked" by Camino "based on your security preferences"?  That obliquely tells users where to turn off safebrowsing, but it sounds stilted and like a bad nightmare string from NSS :P  (There are better ways and places to tell users how to disable the feature.)

Also, I have a partial patch zapping "Web Forgery" and "Attack Site" from the strings, and I think the page title looks *much* better without the "!".  I'm a little less convinced that the blocker bar looks better without it (and maybe the <H1> in the overlay), though they all currently share the same string.  What do others think about the "!"?
(Revisited the page, re-pasted the comment, and somehow *still* managed to remove the dep :/ )
Depends on: 479554
Attached patch string changes for discussion (obsolete) — — Splinter Review 
Here's the aforementioned patch with a bunch of string changes for discussion.

1) Removed the "!" from {Phishing|Malware}TitleText, which removes it from page <title>s, <H1>s, and from the blocker bar.  Pages look much better IMHO; less sure about the blocker bar.

2) New localization note about the "may/possible/etc." language (might also want to mention the prefs UI string here, too, since it can't have an l10n note in the nib).

3) Improved l10n note for the LongDescText strings, to stave off yellow-screen-of-death.  l10n will hit it, because I did ;)

4) ShortDescText no longer speaks of "your security preferences" and instead says sites are "blocked for your protection"

5) Improved (I hope) LongDescText, borrowing some phrase ideas from Safari.  I couldn't think of a way to state "<p>Your computer can be infected just by visiting this site, without any further action on your part.</p>" any other way without being less clear, so that is borrowed verbatim from Safari :(

6) No changes for "Get me out of here!", but as noted above, we probably want the button to say "Home" "Go Back to the Previous Page" or something else, depending on what it ends up doing.

Thoughts?
Why the added "such as banks"? Seems iffy to call out a specific example, since it might lead some people to think that if it's not something bank-like the warning doesn't apply.

The long description still has a reference to "attack sites".

I'm a bit worried that "without any further action on your part" will make people think that they have *already* been infected; they may not understand that what they just did is not "visiting this site", since that's what they were trying to do. Maybe "continuing to" instead of "visiting"? Somewhat awkward, but the best I can come up with at the moment.
(In reply to comment #6)
> Why the added "such as banks"? Seems iffy to call out a specific example, since
> it might lead some people to think that if it's not something bank-like the
> warning doesn't apply.

Safari uses that, and I thought it was a good way to help clarify what "trusted websites" are.  Do you think something like "including banks, online stores, and foo" would work, or should we just remove the clarification entirely? Frankly, though, they're going after your WoW login and web-based community logins, too, so it's not just "trusted websites" in the sense of sites you expect to "be secure" in the old lock/yellow bar sense of the word, but sites you might not think are "trusted sites" at all :|

> The long description still has a reference to "attack sites".

Fixed locally.

> I'm a bit worried that "without any further action on your part" will make
> people think that they have *already* been infected; they may not understand
> that what they just did is not "visiting this site", since that's what they
> were trying to do. Maybe "continuing to" instead of "visiting"? Somewhat
> awkward, but the best I can come up with at the moment.

How about "proceeding to"?  I think it reads a better than "continuing to".  Or do we want to reword it to reference the button name directly, "…just by clicking the “Ignore this warning” button, without…" (whatever we may call the button, when we make it look like an actual button).
(In reply to comment #7)
> Frankly, though, they're going after your WoW login and web-based community
> logins, too, so it's not just "trusted websites" in the sense of sites you
> expect to "be secure" in the old lock/yellow bar sense of the word, but sites
> you might not think are "trusted sites" at all :|

Yeah, that's basically my concern; I trust Facebook to at least some degree (with my Facebook password, certainly), but it's not bank-like. I'd say no examples is probably better.

> How about "proceeding to"?

Works for me.

> do we want to reword it to reference the button name directly

That was my first thought, but I couldn't come up with anything that didn't seem really awkward and verbose, so let's go with "proceeding to" absent an epiphany.
Attached patch WIP 1 (obsolete) — — Splinter Review 
Here's what I've hacked up so far ;)

1) String changes as discussed above
2) New string change for moving "Why Blocked" from a button to text, with 
   supporting constant changes (but still needs some code changes to pull 
   the URL from the pref)
3) Buttons more like you'd expect if the error page were a dialogue (except for
   the fact that return does not actually trigger the default button, even when
   focused; XUL-in-HTML is awesome?!).  There's probably more slight tweaks to 
   do to the CSS positioning (the buttons have some extra? padding), but it's 
   close.
4) Updated the comments to remove/replace the browser.js references.
5) Less sucky colors.  We should try to match the red better, and I'm not sure 
   if we can make a grey that will work (or a different shade of black).  I 
   don't really like philippe's color scheme in attachment 388905 [details], but 
   the color scheme issues he identifies in bug 479554 comment 1 are problems.

Screenshot to follow.
Attachment #390152 - Attachment is obsolete: true
Fwiw,
1. the red used in the icon:
    * top pixel: rgb(238,31,31) - #ee1f1f
    * pixel just above white bar: rgb(216,21,18) - #d81512
those might be a little too bright for a large background-colour, though.

2. the grey text is very hard to read on (current) the background-colour. I'd go for something closer to white.

(In reply to comment #9)
>  There's probably more slight tweaks to 
>    do to the CSS positioning (the buttons have some extra? padding), but it's 
>    close.

You mean the space between the 2 buttons ? Based on your screenshot, there is some 34px between them - 24px comes from your CSS, the rest is possibly white-space in the xhtml (buttons are inline replaced elements). But I'll need to patch and build to check that.
This is how WIP 1 would appear to a person with red-green colour blindness.
A useful tool to analyse colour contrasts
http://juicystudio.com/services/luminositycontrastratio.php

For this kind of pages I'd aim for Level AAA for small (regular) text.
Yeah, both of the above-the-white-bar reds are too bright :-(

I kind of like #ac2320 for the bg (which is from a few px below the white bar in the blocked image), but I can't seem to find a color for the text that rates AAA :( :grmbl: :grmbl:  (#DCDCDC passes with the #772222 bg in the WIP 1 patch, but I don't think it's very legible.)

Maybe we could do a double swap, so that the page bg is red, the message area is white or a lighter grey than attachment 388905 [details], and then black and (some highlight color) text?
(In reply to comment #15)

> http://www.ardisson.org/smokey/moz/sb-overlay/safebrowsing_overlay_wip_v1.1-swap-e8.png
> http://www.ardisson.org/smokey/moz/sb-overlay/safebrowsing_overlay_wip_v1.1-swap-f0.png

I prefer those. 
 
> (I had no good ideas for the shortDesc color.)
What about simply making it bolder ? Not many colours will fit well on the grey background. The similar message in Opera 10 & Safari uses black, afaik.

(In reply to comment #11)
> (In reply to comment #9)
 
> You mean the space between the 2 buttons ? Based on your screenshot, there is
> some 34px between them - 24px comes from your CSS, the rest is possibly
> white-space in the xhtml (buttons are inline replaced elements). But I'll need
> to patch and build to check that.

After a round of quizzing & DomI (and cursing Fx3 that refused to load the safe browser page at first):
1. chrome://global/skin/button.css sets a margin for button {margin:6px;}
          button + button {margin-left:0;}

2. as I suspected, there is some additional space (3~4px) due to the white-space node (the line-break in the source code) between in the 2 buttons.
          Removing the line-break between the 2 buttons removes the extra space.
Fx 3, after cursing, shows a similar 3~4 px spacing. Minefield, with html5 parser enabled, suppresses that white-space node, which puzzles me.

... or fudging on the button's margin-right (15px instead of 24px) (#ignoreWarningButton)
(In reply to comment #11)
> (In reply to comment #9)
> >  There's probably more slight tweaks to 
> >  do to the CSS positioning (the buttons have some extra? padding), but 
> >  it's close.
> 
> You mean the space between the 2 buttons ? Based on your screenshot, there is
> some 34px between them - 24px comes from your CSS, the rest is possibly
> white-space in the xhtml (buttons are inline replaced elements). But I'll need
> to patch and build to check that.

Oops, I forgot that part: I was mostly concerned with the right edge alignment (wrt the text above) and whether that looks OK.  Space between the buttons doesn't bother me so much; 24px is a minimum for dangerous buttons, but we should make sure whatever we do looks OK.  (Things further look a bit odd since the button is "Regular" size but the text inside and the text-to-edge padding is "Small" size :boggle:)(In reply to comment #16)

> 2. as I suspected, there is some additional space (3~4px) due to the
> white-space node (the line-break in the source code) between in the 2 buttons.
>           Removing the line-break between the 2 buttons removes the extra
> space.

:O
(In reply to comment #9)
> 3) Buttons more like you'd expect if the error page were a dialogue (except for
>    the fact that return does not actually trigger the default button, even when
>    focused; XUL-in-HTML is awesome?!).

I applied the same content tweaks to Firefox and, indeed, return does not trigger the default button.  I suppose we could write some JS to listen for return and trigger the default button; I don't know how much work that would be or how hairy it would make things.  It may be simpler to not focus (and not make default) the "Get me out of here" button (or whatever name we finally give it when we decide if it goes Back or Home).
From this morning's meeting:

* There was concern that the -swap versions weren't scary enough, but I eventually convinced everyone that readability of the text was (more) important
* We decided to proceed with safebrowsing_overlay_wip_v1-swap-e8.png [1] with the <h1> text the same color as the background
* We need to fix the button spacing, both wrt the right edge alignment with the <hr> and between the buttons (24px)
* hendy offered to look about writing js to make return actually trigger the default button
* No action yet on getting the "more info" URL read from the prefs and inserted into the localized string
* We didn't discuss what "get me out of here" should do or be named

[1] http://www.ardisson.org/smokey/moz/sb-overlay/safebrowsing_overlay_wip_v1-swap-e8.png

I'll make a new mockup (and patch) with the color/alignment changes.
Attached file Add key handling to blockedSite (obsolete) — 
Here are the javascript additions for handling an Enter/Return keypress. Changes the initPage() function, and adds 2 other functions. These make the focus code in WIP 1 unnecessary.

The event handler will call the button's click() function, which doesn't momentarily highlight the button. The whole thing happens quickly enough that I don't think it matters.

Not provided as a patch, as it's probably better that Smokey incorporate it into his patch instead.
Attached file Better key handling — 
We actually want the enter/return key to work even if the default button isn't focused, to emulate a real Cocoa default button.
Attachment #392851 - Attachment is obsolete: true
Since I was missing one and we need it this morning, I added an "original" (representing current cvs) to http://www.ardisson.org/smokey/moz/sb-overlay/
Attached patch WIP 2 — — Splinter Review 
Here's the patch that covers all of this morning's comments (colors, button spacing) and incorporates hendy's js code to make return work.

Assuming everyone is happy with that look (:cough: Sam :cough:), two things are outstanding:

* The "more info" URL should be read from the browser.safebrowsing.warning.infoURL pref and inserted into the MoreInformationText localized string (instead of hardcoding the URL in the string, as it is in this patch).

* We need to decide what "Get me out of here!" should do and what the button should be named.

I'll attach a screenshot of this UI momentarily, but you can also find all of my mockups so far at http://www.ardisson.org/smokey/moz/sb-overlay/

There is one final wrinkle: Sam thinks this is absolutely awful and has promised to make an awesome UI, which he'll deliver as a .psd for us to implement in HTML, CSS, and JS. :P
Assignee: nobody → alqahira
Attachment #392129 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 392879 [details] [diff] [review]
WIP 2

Given the current status of things, I think it's best we get the bulk of the changes in now (i.e., early this week), and just figure out "Get me out of here" and the URL-from-pref-into-localized-string as followup patches.  That should be much less disruptive than a radical UI change later on before b4.

philippe, can you look over the changes to blockedSite.xhtml and netError.css?
Attachment #392879 - Flags: review?(phiw)
Attachment #392879 - Flags: review?(phiw) → review+
Comment on attachment 392879 [details] [diff] [review]
WIP 2

The page looks good now.
r=me for the .css and .xhtml changes
Comment on attachment 392879 [details] [diff] [review]
WIP 2

Thanks, philippe!

The Obj-C code changes are trivial, so going straight to sr for the rest.
Attachment #392879 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #392879 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
Comment on attachment 392879 [details] [diff] [review]
WIP 2

sr=smorgan
Landed on CAMINO_2_0_BRANCH and cvs trunk.  

Closing this because the bulk of the work is done, this bug is quite long already, and I probably won't be the one to fix the rest, given they need code changes.

Filed bug 509468 on the "Get me out of here" button and bug 509469 on the "MoreInformationText" string's URL.  Both still have l10n implications.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: camino2.0b4? → camino2.0b4+
Resolution: --- → FIXED
Whiteboard: l10n → l10n [camino-2.0]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: