Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Connection info click area is on the urlbarIcon, should be on urlbarIconContainer #7888

Closed
srirambv opened this issue Mar 25, 2017 · 18 comments · Fixed by #13147 or #13164
Closed

Connection info click area is on the urlbarIcon, should be on urlbarIconContainer #7888

srirambv opened this issue Mar 25, 2017 · 18 comments · Fixed by #13147 or #13164
Assignees
Labels
accessibility bug/good-first-bug design A design change, especially one which needs input from the design team. feature/URLbar parity Features which should be supported in Brave since they're supported in other major browsers. polish Nice to have — usually related to front-end/visual tasks. QA/test-plan-specified release-notes/include
Milestone

Comments

@srirambv
Copy link
Collaborator

srirambv commented Mar 25, 2017

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

Original issue description

  • Did you search for similar issues before submitting this one?
    Yes

  • Describe the issue you encountered:
    The click area is pretty small limited only to the urlbaricon, even a little off causes the click to be not captured. The click should be on the urlbarIconContainer which increases the hit area

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    All

  • Brave Version (revision SHA):
    Brave 0.14.0
    rev ffa6f8f

  • Steps to reproduce:

    1. Open any http/https site
    2. Click on the padlock icon the connection info is shown
    3. Move cursor sightly off the padlock but visually the pointer still looks on the padlock, click not recognized and connection info is not shown

    All other browser has the large click area to show the connection info window/modal

  • Actual result:
    Connection info click area is on the urlbarIcon, should be on urlbarIconContainer

  • Expected result:
    Connection info not shown if the click is off by a marginal amount

  • Will the steps above reproduce in a fresh profile? If not what other info can be added?
    Yes

  • Is this an issue in the currently released version?
    Yes

  • Can this issue be consistently reproduced?
    Yes

  • Extra QA steps:
    1.
    2.
    3.

  • Screenshot if needed:
    What it is now
    image
    What it should be
    image

  • Any related issues: Make entire bookmarkButtonContainer clickable to add/edit a bookmark #6704
    cc: @bradleyrichter @bsclifton

@srirambv srirambv added design A design change, especially one which needs input from the design team. feature/URLbar parity Features which should be supported in Brave since they're supported in other major browsers. polish Nice to have — usually related to front-end/visual tasks. QA/test-plan-specified labels Mar 25, 2017
@srirambv srirambv added this to the 0.14.1 milestone Mar 25, 2017
@luixxiul
Copy link
Contributor

The current clickable are is the same as Chrome fyi

@luixxiul luixxiul removed this from the 0.14.1 milestone Mar 25, 2017
@srirambv
Copy link
Collaborator Author

No its not the same. Its completely different from Chrome
7888

@srirambv srirambv added this to the 0.14.1 milestone Mar 25, 2017
@luixxiul
Copy link
Contributor

luixxiul commented Mar 25, 2017

Sorry I was on Ubuntu.

icon

@luixxiul
Copy link
Contributor

Still I believe that the clickable area is not only the lock icon but also the whole "GitHub, Inc. [US]" rectangle on your GIF animation.

icon

@srirambv
Copy link
Collaborator Author

Yes. Since we don't have any text next to the padlock it makes sense to have the entire urlbarIconContainer clickable. Users might be used to click on the text next to padlock and would expect the similar behavior. Just makes it easy to view.

@bsclifton
Copy link
Member

this is a super important one to fix- do you think you could check this out @luixxiul?

Moving to 0.14.3 for now

@bsclifton bsclifton modified the milestones: 0.14.3, 0.14.2 Apr 5, 2017
@luixxiul
Copy link
Contributor

luixxiul commented Apr 5, 2017

Fine, I will add another container inside urlbarIconContainer to wrap it.

I don't think it is a good idea to make all of urlbarIconContainer clickable, unless we add the border like bookmark button and add fund button. This should be another task (if we will display some info like Chrome does).

@luixxiul luixxiul self-assigned this Apr 5, 2017
@bsclifton
Copy link
Member

@luixxiul I think that's a great point- @bradleyrichter would we be interested in adding a border?

@bradleyrichter
Copy link
Contributor

a border? I like Chrome's "border". Invisible. ; )

@alexwykoff alexwykoff assigned bsclifton and unassigned luixxiul Apr 18, 2017
@bbondy bbondy removed this from the 0.15.2 milestone May 2, 2017
@bsclifton bsclifton removed their assignment May 2, 2017
@MargarytaChepiga
Copy link
Contributor

@srirambv Working on it!

@bsclifton bsclifton self-assigned this Feb 7, 2018
@bsclifton
Copy link
Member

Awesome! thanks, @MargarytaChepiga 😄 I assigned the issue to myself to reserve it for you- let us know if you need any help 😄

@MargarytaChepiga
Copy link
Contributor

@bsclifton Great! Thank you!

@MargarytaChepiga
Copy link
Contributor

So I've been working on the issue for a few days now, and I haven't done much progress. I've been looking at urlbar.js and urlBaricon.js files to find and understand how the urlbarIcon get's clickable, but it is still unclear to me. I would really appreciate any help/tips on how I could tackle this issue. Thanks!

@cndouglas
Copy link

@MargarytaChepiga
The onClick function in urlBarIcon.js contains the code that shows the connection info.

onClick () {
if (isSourceAboutUrl(this.props.location) || this.isSearch) {
return
}
windowActions.setSiteInfoVisible(true)
}

The onClick function is bound to the component here:

this.onClick = this.onClick.bind(this)

And the onClick function is set to the onClick attribute here:

The onClick function in urlBarIcon.js needs to be moved over to urlBar.js. Then, the onClick attribute of the urlbarIconContainer (on both lines 500 and 506) can be set to the new function.

render () {
const urlbarIconContainer = this.props.evCert
? (<div className='urlbarIconContainer'>
<UrlBarIcon
titleMode={this.props.titleMode}
/>
{this.showEvCert}
</div>)
: (<div className='urlbarIconContainer'>
<UrlBarIcon
titleMode={this.props.titleMode}
/>
</div>)

@MargarytaChepiga
Copy link
Contributor

@LiunkaeThat helps a lot! Thank you so much!

MargarytaChepiga added a commit to MargarytaChepiga/browser-laptop that referenced this issue Feb 15, 2018
@NejcZdovc NejcZdovc modified the milestones: Triage Backlog, 0.23.x (Nightly Channel) Feb 15, 2018
bsclifton added a commit that referenced this issue Feb 15, 2018
Made urlBarIconContainer clickable to display connection info. Fix #7888
@bsclifton bsclifton reopened this Feb 16, 2018
@bsclifton bsclifton modified the milestones: 0.23.x (Nightly Channel), 0.24.x Feb 26, 2018
ryanml pushed a commit to ryanml/browser-laptop that referenced this issue Feb 27, 2018
ryanml pushed a commit to ryanml/browser-laptop that referenced this issue Feb 27, 2018
@bsclifton bsclifton modified the milestones: Completed work, 0.24.x (Nightly Channel) Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility bug/good-first-bug design A design change, especially one which needs input from the design team. feature/URLbar parity Features which should be supported in Brave since they're supported in other major browsers. polish Nice to have — usually related to front-end/visual tasks. QA/test-plan-specified release-notes/include
Projects
None yet
9 participants