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)
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)
31.22 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years 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•7 years 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•7 years ago
|
Whiteboard: #sbv4-m9
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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)
Comment 10•7 years ago
|
||
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.
Comment 11•7 years ago
|
||
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•7 years 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+
Comment 13•7 years ago
|
||
(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...
Comment 14•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2546f74b4561b7e4d2683f71ac062905d6095c0a
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 17•7 years 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•7 years 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•7 years ago
|
Whiteboard: #sbv4-m9
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/07b2b1b55448
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Version: unspecified → 56 Branch
Comment 20•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1a8880e29288
Updated•7 years ago
|
Flags: qe-verify+
Comment 22•7 years ago
|
||
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.
Description
•