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

Fix wide bravery panel to stop covering the webview #11880

Merged
merged 2 commits into from
Nov 20, 2017
Merged

Fix wide bravery panel to stop covering the webview #11880

merged 2 commits into from
Nov 20, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Nov 9, 2017 •
edited
Loading

Fixes #11878
Fixes #11899
Follow-up to #9839

Auditors:

Test Plan 1:

  1. Open https://chiebukuro.yahoo.co.jp/
  2. Click the adblock count
  3. Make sure panel width does not change

Test Plan 2:

  1. Open https://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/
  2. Open the wide bravery panel
  3. Make sure the panel does not cover the webview

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.

Test Plan:

Reviewer Checklist:

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

Set braveryPanel list width to zero
fb34aaf 
Fixes #11878

Auditors:

Test Plan:
1. Open https://chiebukuro.yahoo.co.jp/
2. Click the adblock count
3. Make sure panel width does not change
@luixxiul luixxiul added 0.22.x issue first seen in 0.22.x feature/shields labels Nov 9, 2017
@luixxiul luixxiul self-assigned this Nov 9, 2017
@luixxiul luixxiul requested a review from srirambv November 9, 2017 16:19
@srirambv
Copy link
Collaborator

srirambv commented Nov 9, 2017

Just one concern, shields panel has scroll bars which is quite distracting and not very appealing
image

@luixxiul
Copy link
Contributor Author

I think it's not a regression. This is on 0.19.88.

32432301-6cfa0a9a-c31a-11e7-87ca-52b4b3ea69ea

Why wouldn't we talk about that on another issue and leave it as it is for now?

@luixxiul luixxiul added 0.20.x issue first seen in 0.20.x and removed 0.22.x issue first seen in 0.22.x labels Nov 10, 2017
@srirambv
Copy link
Collaborator

@luixxiul not the horizontal scroll bar in the details section, the vertical scroll bar on the shields menu itself from my screenshot above.

@luixxiul
Copy link
Contributor Author

I am wondering if it is due to the new row of fingerprinting protection. would you mind checking if it is not?

@luixxiul luixxiul mentioned this pull request Nov 11, 2017
Closed
Avoid a very long domain name from covering the webview
38cbbe3 
Auditors:

Test Plan:
1. Open https://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/
2. Open the wide bravery panel
3. Make sure the panel does not cover the webview
@luixxiul luixxiul changed the title Set braveryPanel list width to zero Avoid the wide bravery panel from covering the webview Nov 11, 2017
@luixxiul luixxiul requested a review from bsclifton November 12, 2017 07:59
@luixxiul luixxiul changed the title Avoid the wide bravery panel from covering the webview Fix wide bravery panel to stop covering the webview Nov 17, 2017
@srirambv srirambv mentioned this pull request Nov 18, 2017
Closed
@luixxiul luixxiul added this to the 0.20.x (Beta Channel) milestone Nov 18, 2017
@srirambv
Copy link
Collaborator

manually verified test plan 1 & 2. Works fine. But with more ad block count, vertical scrollbar is still seen and hides Edit settings and Reload buttons. Setting braveryPanelBodyList to 220px from 300px doesn't show the vertical scrollbar
scrollbar

@luixxiul
Copy link
Contributor Author

Fine! Since that is out of scope of this PR (ie. the issue has existed before this PR), I'm going to address that stuff with another PR.

srirambv
srirambv approved these changes Nov 20, 2017
View reviewed changes
Copy link
Collaborator

@srirambv srirambv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@srirambv srirambv merged commit 21fba11 into brave:master Nov 20, 2017
@NejcZdovc
Copy link
Contributor

@srirambv don't forget to cherry pick it into 0.21 and 0.20

@luixxiul
Copy link
Contributor Author

@srirambv if you are not how to, mind asking @NejcZdovc to do so (as I'm not quite sure how to and I don't want to break anything)?

NejcZdovc pushed a commit that referenced this pull request Nov 20, 2017
@luixxiul @NejcZdovc
Fix wide bravery panel to stop covering the webview (#11880)
f40134a 
* Set braveryPanel list width to zero

Fixes #11878

Auditors:

Test Plan:
1. Open https://chiebukuro.yahoo.co.jp/
2. Click the adblock count
3. Make sure panel width does not change

* Avoid a very long domain name from covering the webview

Auditors:

Test Plan:
1. Open https://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/
2. Open the wide bravery panel
3. Make sure the panel does not cover the webview
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Nov 20, 2017 •
edited by srirambv
Loading

master 21fba11
0.21 f40134a
0.20 e031461

srirambv pushed a commit that referenced this pull request Nov 20, 2017
@luixxiul @srirambv
Fix wide bravery panel to stop covering the webview (#11880)
e031461 
* Set braveryPanel list width to zero

Fixes #11878

Auditors:

Test Plan:
1. Open https://chiebukuro.yahoo.co.jp/
2. Click the adblock count
3. Make sure panel width does not change

* Avoid a very long domain name from covering the webview

Auditors:

Test Plan:
1. Open https://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/
2. Open the wide bravery panel
3. Make sure the panel does not cover the webview
@luixxiul luixxiul deleted the fix-braveryPanel__body__ul__li-width branch November 20, 2017 11:17
@luixxiul luixxiul removed the request for review from bsclifton November 20, 2017 11:18
@luixxiul
Copy link
Contributor Author

thanks guys :-)

@luixxiul luixxiul mentioned this pull request Nov 20, 2017
Closed
@srirambv
Copy link
Collaborator

srirambv commented Dec 20, 2017 •
edited
Loading

This is still happening on the latest beta build (a2ba3e8)
Here's how it looks https://youtu.be/OGfow9czqUA

Edit: works fine on 0.20.8

@luixxiul
Copy link
Contributor Author

is it broken on 0.20.9 ?

@srirambv
Copy link
Collaborator

Nope not broken on 0.20.9 Works fine

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@srirambv srirambv srirambv approved these changes

Assignees

@luixxiul luixxiul

Labels
0.20.x issue first seen in 0.20.x feature/shields QA/test-plan-specified
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

Successfully merging this pull request may close these issues.
3 participants
@luixxiul @srirambv @NejcZdovc