Closed Bug 1388912 Opened 7 years ago Closed 7 years ago

Advisory link on Safe Browsing interstitials should not be blue.

Categories

(Toolkit :: Safe Browsing, defect, P1)

56 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + verified
firefox57 --- verified

People

(Reporter: francois, Assigned: tnguyen)

References

Details

(Keywords: regression, Whiteboard: #sbv4-m9)

Attachments

(2 files)

Attached image 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.
looks bad contrast, thanks for reporting.
Hi Johann, could you please review, assume we don't have any additional design on link hover, visited?
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
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)
(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.
(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)
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 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...
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
Keywords: checkin-needed
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?
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
Whiteboard: #sbv4-m9
https://hg.mozilla.org/mozilla-central/rev/07b2b1b55448
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.