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

Alert box with quite lengthy content cannot be closed #7930

Closed
luixxiul opened this issue Mar 28, 2017 · 9 comments
Closed

Alert box with quite lengthy content cannot be closed #7930

luixxiul opened this issue Mar 28, 2017 · 9 comments

Comments

@luixxiul
Copy link
Contributor

luixxiul commented Mar 28, 2017

Test plan

#8475 (comment)


Describe the issue you encountered:
Alert box with quite lenghty content cannot be closed. This could be used by an attacker (despite of low security concern).

@luixxiul luixxiul added the bug label Mar 28, 2017
@luixxiul luixxiul changed the title Alert box with quite lenghty content cannot be closed. Alert box with quite lenghty content cannot be closed Mar 28, 2017
@NejcZdovc
Copy link
Contributor

Looks ok on macOS, 0.13.5 or maybeI didn't understand the issue

image

@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 28, 2017

It is because the monitor resolution on macOS is higher than Windows. On macOS you can reproduce the issue by adding more text via devtool.

On Windows 7 on 1152x864 monitor I get this:

clipboard01

What we would like to have is:

scroll

Is it clear?

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Mar 28, 2017

Got it. We need overflow. Should be an easy fix

@srirambv
Copy link
Collaborator

Its really bad on Windows. This is on maximized window
image

@NejcZdovc
Copy link
Contributor

We should also add some margin on the side

@bsclifton
Copy link
Member

bsclifton commented Mar 28, 2017

Good catch @luixxiul! I'll pull this into 0.14.2 😄

@bsclifton bsclifton added this to the 0.14.2 milestone Mar 28, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 29, 2017

ReleaseNote should be fixed as well:

clipboard01

STR:

  1. Set BRAVE_UPDATE_VERSION=0.8.3
  2. Start the browser
  3. Update the browser

Currently ReleaseNote is so simply coded that we should upgrade it in a future:

<div>{this.props.metadata.get('notes')}</div>

@bsclifton bsclifton added needs-owner ♞ This issue is tagged for an upcoming release but has no owner. and removed needs-owner ♞ This issue is tagged for an upcoming release but has no owner. labels Apr 4, 2017
@alexwykoff alexwykoff modified the milestones: Backlog, 0.15.1 Apr 18, 2017
@jonathansampson
Copy link
Collaborator

jonathansampson commented Apr 24, 2017

The dialog also needs to have a max-width, IMHO:

image

Supporting a scrollbar would be idea, but the content itself should be the only thing to scroll. The dialog itself should probably have a fixed height too.

@cezaraugusto
Copy link
Contributor

I know this is supposed to be a low hanging fruit for first-time contribs but this is really bad UI so I'm taking

@cezaraugusto cezaraugusto self-assigned this Apr 24, 2017
cezaraugusto added a commit that referenced this issue Apr 24, 2017
@cezaraugusto cezaraugusto modified the milestones: 0.15.1, Backlog Apr 24, 2017
@bsclifton bsclifton changed the title Alert box with quite lenghty content cannot be closed Alert box with quite lengthy content cannot be closed Apr 25, 2017
cezaraugusto added a commit that referenced this issue Apr 26, 2017
cezaraugusto added a commit that referenced this issue Apr 28, 2017
cezaraugusto added a commit that referenced this issue Apr 28, 2017
* prevent Dialog from being uncloseable with long text
- Fix #7930
- Auditors: @luixxiul

* reopen
@luixxiul luixxiul added this to the 0.15.1 milestone Apr 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.