Skip to content
This repository has been archived by the owner. It is now read-only.

Made urlBarIconContainer clickable to display connection info. Fix #7888 #13147

Merged
merged 1 commit into from Feb 15, 2018

Conversation

@MargarytaChepiga
Copy link
Collaborator

commented Feb 15, 2018

Fixes #7888
Fixes #13011

Before:
before
After:
after

I also touched some css, changed few things that had a TODO. In addition to changing a little bit of styling.
Also, I am not sure if I need to update and/or add new tests for this fix. Please let me know if so :)

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  1. Navigate to wikipedia.org
  2. Put mouse in URL bar and try clicking around the lock icon
  3. Secure connection modal should come up (even near the edges)
  4. Verify you can still drag and drop the lock icon to bookmarks toolbar
  5. Navigate to github.com
  6. Put mouse in URL bar and try clicking around the lock icon
  7. Verify secure modal is shown when clicking everything left of where you type (ex: the EV info bar and the lock icon, including the edges)
  8. Visit https://www.wsj.com/europe
  9. Page should be mixed (HTTP/HTTPS); the unlock icon should be gray

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header
@srirambv srirambv requested review from cezaraugusto, bsclifton and srirambv Feb 15, 2018
@srirambv srirambv modified the milestones: 0.21.x (Beta Channel), Triage Backlog Feb 15, 2018
@NejcZdovc NejcZdovc modified the milestones: 0.21.x (Beta Channel), 0.23.x (Nightly Channel) Feb 15, 2018
@bsclifton

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

@MargarytaChepiga I updated your post to include a test plan- can you take a peek and let me know what you think? 😄

Copy link
Member

left a comment

Changes work great 😄 👍 Definitely nice to have this fixed. Thanks! 🥇

@bsclifton bsclifton merged commit fbdf34d into brave:master Feb 15, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@codecov-io

This comment has been minimized.

Copy link

commented Feb 15, 2018

Codecov Report

Merging #13147 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13147      +/-   ##
==========================================
- Coverage    56.3%   56.26%   -0.04%     
==========================================
  Files         278      278              
  Lines       27585    27584       -1     
  Branches     4497     4497              
==========================================
- Hits        15531    15521      -10     
- Misses      12054    12063       +9
Flag Coverage Δ
#unittest 56.26% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
app/renderer/components/styles/global.js 100% <ø> (ø) ⬆️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 84.61% <0%> (-6.16%) ⬇️
js/stores/windowStore.js 27.69% <0%> (-0.3%) ⬇️
@srirambv

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2018

Works great ++.

@MargarytaChepiga would it be possible to add the separator after the lock icon for sites that do not show the EVC

image

The reason being as per test plan it says click around edges to see view certificate modal is shown but where EVC is not shown there is no edge on the right side so having the separator would make it clickable within that area

@bsclifton what do you think?

@MargarytaChepiga

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2018

@srirambv Good point actually. I will fix it

@MargarytaChepiga

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2018

@bsclifton Thank you so much! 😄 I think test plan looks great, and as already mentioned above I will fix the missing separator.

@bsclifton

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

@MargarytaChepiga there is one other issue, caught by @NejcZdovc
image

It seems the color used by the EV elements is bleeding through- causing the lock icon on mixes HTTP/HTTPS links to show as green

For your next PR, can you please take a look at this? 😄

Here's what it looks like currently (0.20.42):
screen shot 2018-02-15 at 11 02 26 pm

@bsclifton

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

@srirambv as called out in the issue you created (#13159), we should leave the separator off (at least for now)

I think we just need a fix for the color bleed through for mixed content and we'll be good 😄 I updated the test plan to capture this (thanks, @NejcZdovc!)

@@ -40,7 +40,7 @@
@settingItemSubtextFontSize: 0.95rem;
@audioColor: @highlightBlue;
@focusUrlbarOutline: rgba(55, 169, 253, 0.4);
@siteSecureColor: @buttonColor;
@siteSecureColor: green;

This comment has been minimized.

Copy link
@NejcZdovc

NejcZdovc Feb 16, 2018

Member

@bsclifton this line is problematic as well, because I think that we only want EV to be green

This comment has been minimized.

Copy link
@srirambv

srirambv Feb 16, 2018

Collaborator

Just replacing green with @siteEVColor fixes it

id='urlbar'
>
{urlbarIconContainer}
{
this.props.titleMode
? <div id='titleBar' data-test-id='titleBar'>
? <div id='titleBar' data-test-id='titleBar' >

This comment has been minimized.

Copy link
@bsclifton

bsclifton Feb 16, 2018

Member

this extra space at the end is also causing the linter to fail; you can manually run this with:
npm run lint

@srirambv

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2018

petemill added a commit to srirambv/browser-laptop that referenced this pull request Feb 16, 2018
@@ -513,13 +517,13 @@ class UrlBar extends React.Component {
urlbarForm: true,
[css(styles.urlbarForm_wide)]: this.props.isWideURLbarEnabled,
noBorderRadius: this.props.publisherButtonVisible
})}
})}

This comment has been minimized.

Copy link
@petemill

petemill Feb 16, 2018

Member

lint error here

@diracdeltas

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

I think we should revert the color changes for now, pending review by security team. There are some edge cases where I think the behavior is incorrect:
screen shot 2018-02-16 at 1 35 02 pm

BTW this should have been marked for sec review before merge. :(

@diracdeltas

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

Just catching up here, I see @srirambv already opened #13164. Please be sure to request a security review this time, thanks.

bsclifton added a commit to srirambv/browser-laptop that referenced this pull request Feb 16, 2018
@bsclifton

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

Removing milestone since this was reverted with #13168

@bsclifton bsclifton removed this from the 0.23.x (Nightly Channel) milestone Mar 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.