Advisory link on Safe Browsing interstitials should not be blue.

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
Safe Browsing
P1
normal
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: francois, Assigned: tnguyen)

Tracking

({regression})

56 Branch
mozilla57
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56+ verified, firefox57 verified)

Details

(Whiteboard: #sbv4-m9)

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

2 months ago
Created attachment 8895584 [details]
advisory-link-blue.png

The Safe Browsing advisory link has bad contrast with the red background.

We should follow the new design (https://d26dzxoao6i3hh.cloudfront.net/items/2K3B2P083j3K2Y0L060u/dangerous_phishing_win_expanded.png?v=f292c721) and make that link white just like the "Ignore this warning" link.

[Tracking Requested - why for this release]: This is a regression introduced in bug 1366384 where we updated the Safe Browsing error pages.
Track 56+ as regression.
tracking-firefox56: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
looks bad contrast, thanks for reporting.
Hi Johann, could you please review, assume we don't have any additional design on link hover, visited?
(Reporter)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8895752 [details]
Bug 1388912 - Change text color of advisory link on SB warning page to white

https://reviewboard.mozilla.org/r/167078/#review172438

::: browser/themes/shared/blockedSite.css:67
(Diff revision 1)
>  #ignoreWarning {
>    margin-top: 1.2em;
>    text-align: end;
>  }
> +
> +#advisory_provider {

nit: should probably be `advisoryProvider` to match the style of the other classes

::: mobile/android/themes/core/netError.css:186
(Diff revision 1)
>    margin-inline-start: -20px;
>    font-size: smaller;
>    border-radius: 0;
>  }
>  
> +#advisory_provider {

ditto
(Reporter)

Updated

2 months ago
Whiteboard: #sbv4-m9
Hm, I'm not really sure how to act on this. This needs active and hover states.

The UX spec just seems incomplete to me. Does it include a hover state? Normally the hover state for links is underline.

The fact that the "Ignore this warning" "button" doesn't include those makes it only marginally better.

Bram, what do you think about this?
Flags: needinfo?(bram)
What if we extend the white area for the whole text? The background would remain red but we would have a white card in the center of the page where the typography is be set in black and links and primary buttons are set in blue (secondary buttons will be set in gray). I tweaked the page with the inspector, this is how it could look https://screenshots.firefox.com/V9nAYWGKqEJmYXV5/null, I used the photon colors, borders radius, shadows and the photon buttons we are currently working on :). What do you folks think?
Flags: needinfo?(jhofmann)

Comment 7

2 months ago
(In reply to Amin Al Hazwani [:amin] (Firefox UX) from comment #6)
> What if we extend the white area for the whole text? The background would
> remain red but we would have a white card in the center of the page where
> the typography is be set in black and links and primary buttons are set in
> blue (secondary buttons will be set in gray). I tweaked the page with the
> inspector, this is how it could look
> https://screenshots.firefox.com/V9nAYWGKqEJmYXV5/null, I used the photon
> colors, borders radius, shadows and the photon buttons we are currently
> working on :). What do you folks think?


I like it ( ;) ). In this way, we can the default links (including all the states for hover, focus, active, etc., etc.) and buttons. 

Suggestion: I'd personally align the secondary button and the link to the left. The current alignment creates a weird white space now between the two buttons.

Comment 8

2 months ago
(In reply to Johann Hofmann [:johannh] from comment #5)
> Hm, I'm not really sure how to act on this. This needs active and hover
> states.
> 
> The UX spec just seems incomplete to me. Does it include a hover state?
> Normally the hover state for links is underline.
> 
> The fact that the "Ignore this warning" "button" doesn't include those makes
> it only marginally better.

To underline or not underline the hover state: it would be best to follow what Photon have specified for the rest of the in-content pages. I would say that it’s optional.

My only advice on comment #6 would be to make the "Ignore this warning" link grey - deemphasise it so that users know that it's insecure and not preferrable to "Get me out of here!"
Flags: needinfo?(bram)
(Assignee)

Comment 9

2 months ago
Comment on attachment 8895752 [details]
Bug 1388912 - Change text color of advisory link on SB warning page to white

Cancel review for now, until we have a ui decision. Do we need a sign off for the UI change?
Attachment #8895752 - Flags: review?(jhofmann)
During our Photon Design System weekly meeting with Shorlander we talked about the warning pages. We decided to ditch the card layout proposed above and keep the full red background. Regarding links and buttons we are proposing to align them with the new Photon guidelines. When it's not possible to use blue links we could fallback to white text and underline. When it's not possible to use blue buttons (for primary actions) and grey buttons (for secondary actions) we could fallback to darker shades of the background color. I created a quick pen https://codepen.io/aminalhazwani/pen/LjzeRQ that features the Photon colors http://design.firefox.com/photon/visual/color.html.
Ok, I'm reading this as "we're okay with this patch and do not propose any improvements at the moment". I just wanted to make sure we're aware that this doesn't have hover state and looks different than normal links.

Thanks for the help! :)
Flags: needinfo?(jhofmann)

Comment 12

2 months ago
mozreview-review
Comment on attachment 8895752 [details]
Bug 1388912 - Change text color of advisory link on SB warning page to white

https://reviewboard.mozilla.org/r/167078/#review172654

This needs a hover state (and ideally active and visited, too).

The UX spec just seems incomplete to me. Does it include a hover state? Normally the hover state for links is underline.

::: browser/themes/shared/blockedSite.css:67
(Diff revision 1)
>  #ignoreWarning {
>    margin-top: 1.2em;
>    text-align: end;
>  }
> +
> +#advisory_provider {

I didn't note this in my review of the original implementation, since it's consistent with the other ids in the translation file:

https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd#18

I would advise keeping the current id to avoid touching the translation key. :)
Attachment #8895752 - Flags: review+
(In reply to Johann Hofmann [:johannh] from comment #12)
> Comment on attachment 8895752 [details]
> Bug 1388912 - Change text color of advisory link on SB warning page to white
> 
> https://reviewboard.mozilla.org/r/167078/#review172654
> 
> This needs a hover state (and ideally active and visited, too).
> 
> The UX spec just seems incomplete to me. Does it include a hover state?
> Normally the hover state for links is underline.
> 
> ::: browser/themes/shared/blockedSite.css:67
> (Diff revision 1)
> >  #ignoreWarning {
> >    margin-top: 1.2em;
> >    text-align: end;
> >  }
> > +
> > +#advisory_provider {
> 
> I didn't note this in my review of the original implementation, since it's
> consistent with the other ids in the translation file:
> 
> https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/safebrowsing/phishing-afterload-warning-message.dtd#18
> 
> I would advise keeping the current id to avoid touching the translation key.
> :)

Ugh, this was meant as a reply to comment 4, reviewboard...
(In reply to Johann Hofmann [:johannh] from comment #12)
> Comment on attachment 8895752 [details]
> Bug 1388912 - Change text color of advisory link on SB warning page to white
> 
> https://reviewboard.mozilla.org/r/167078/#review172654
> 
> This needs a hover state (and ideally active and visited, too).
> 
> The UX spec just seems incomplete to me. Does it include a hover state?
> Normally the hover state for links is underline.
> 

Also ignore this part, reviewboard "helpfully" saved my original comment...
(Assignee)

Comment 15

2 months ago
mozreview-review
Comment on attachment 8895752 [details]
Bug 1388912 - Change text color of advisory link on SB warning page to white

https://reviewboard.mozilla.org/r/167078/#review173842

::: browser/themes/shared/blockedSite.css:67
(Diff revision 1)
>  #ignoreWarning {
>    margin-top: 1.2em;
>    text-align: end;
>  }
> +
> +#advisory_provider {

Okay, keep advisory_provider as consistent of 
https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd#18
Thanks both of you
(Assignee)

Comment 16

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2546f74b4561b7e4d2683f71ac062905d6095c0a
(Assignee)

Updated

2 months ago
Keywords: checkin-needed
(Assignee)

Comment 17

2 months ago
Comment on attachment 8895752 [details]
Bug 1388912 - Change text color of advisory link on SB warning page to white

Approval Request Comment
[Feature/Bug causing the regression]: 1366384
[User impact if declined]: Advisory link has bad contrast with the red background
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: yes
1. Go to about:url-classifier to make sure google4 sb list is updated
2. Go to a page which is blocked by google. Such as http://testsafebrowsing.appspot.com/s/phishing.html. 
3. Check the style of "Google Safe Browsing" advisory link is white and underline
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only change style of the link
[String changes made/needed]: No
Attachment #8895752 - Flags: approval-mozilla-beta?

Comment 18

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07b2b1b55448
Change text color of advisory link on SB warning page to white r=johannh
Keywords: checkin-needed
(Assignee)

Updated

2 months ago
Whiteboard: #sbv4-m9

Comment 19

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07b2b1b55448
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
Version: unspecified → 56 Branch
Comment on attachment 8895752 [details]
Bug 1388912 - Change text color of advisory link on SB warning page to white

Fix a regression. Beta56+.
Attachment #8895752 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 21

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1a8880e29288
status-firefox56: affected → fixed
Flags: qe-verify+
Reproduced this issue on an affected Nightly build (2017-08-09), by following the STR from comment 17.

This is verified fixed on latest Nightly 57.0a1 (2017-08-24) and 56 Beta 5 (20170821193225) across platforms:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.