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

Unable to scroll down bookmarks tab when populated beyond initial display boundry #14606

Closed
Brave-Matt opened this issue Jun 29, 2018 · 10 comments

Comments

@Brave-Matt
Copy link

Description
After updating (0.23.19) users are unable to scroll down a list of bookmarks when the bookmarks tab is open (and populated with enough links to require scrolling)

Steps to Reproduce

  1. Open Brave
  2. Add as many links to bookmarks drop-down tab can display before scrolling (can display ~25 before the need to scroll, correct me if I'm wrong)
  3. Add [at least] one more bookmark
  4. Actual result:
  5. Bookmark is added but no scroll bar displays

Expected result:
Scroll bar should appear after 1 or more bookmarks added, allowing you to move down tab to see new additions

Reproduces how often:
Always

Brave Version

about:brave info:
Despite asking nobody provided their about:brave info, but I was able to reproduce the error so we'll use mine:
Brave: 0.23.19
V8: 6.7.288.46
rev: 178c3fb
Muon: 7.1.3
OS Release: 10.0.16299
Update Channel: Release
OS Architecture: x64
OS Platform: Microsoft Windows
Node.js: 7.9.0
Tor: 0.3.3.7 (git-035a35178c92da94)
Brave Sync: v1.4.2
libchromiumcontent: 67.0.3396.87

Reproducible on current live release:
Yes

Additional Information
Seems to be a regression as far as I can tell. I was unable to reproduce error one previous release and can produce error 100% on live release.

@Brave-Matt Brave-Matt added bug regression 0.23.x issue first seen in 0.23.x labels Jun 29, 2018
@eljuno
Copy link
Contributor

eljuno commented Jun 30, 2018

+1 from @defcon1776 via brave/brave-browser#455

@eljuno
Copy link
Contributor

eljuno commented Jul 2, 2018

@NejcZdovc
Copy link
Contributor

+1 from me

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Jul 2, 2018

This was broken with 681349d where bookmarksToolbarWithFaviconsHeight was renamed, but it's still in use here https://github.com/brave/browser-laptop/blob/master/app/renderer/components/common/contextMenu/contextMenu.js#L325 and because of that we get
image

cc @bsclifton @petemill

@petemill
Copy link
Member

petemill commented Jul 3, 2018

@NejcZdovc thanks for the find. This should probably be simply positioned at bottom: 0 since the 'top' position is already calculated, with an automatic overflow: scroll. In order to do that the styles for the context menu containing box should move to the parent. I would make that change if this component were to survive for much longer, but since its time is limited, I will make this simple change, even though it is somewhat flakey and does not cover every scenario (such as notifications or menu bar)

petemill added a commit that referenced this issue Jul 3, 2018
Fix #14606
Fixes context menu overflow scroll, returning it to previous behavior. Note that previous behavior is flawed as it only calculates an approximation of available space. It does not cover dynamic 'navbar' height such as menu bar or notifications presence. It could be much more gracefully achieved via `bottom: 0` positioning and would look better by creating a container element which has `max-height: 100%` on it so that the inside of the menu box scrolled rather than the border scrolling away wierdly.
@eljuno
Copy link
Contributor

eljuno commented Jul 4, 2018

@bsclifton
Copy link
Member

@LaurenWags
Copy link
Member

LaurenWags commented Jul 10, 2018

Verified with macOS 10.12.6 using

  • 0.23.32 1d1df96
  • Muon 7.1.5
  • libchromiumcontent 67.0.3396.103

Verified on Windows x64 with

  • 0.23.32 1d1df96
  • Muon 7.1.5
  • libchromiumcontent 67.0.3396.103

Verified on Ubuntu 17.10 x64

  • 0.23.32 1d1df96
  • Muon 7.1.5
  • libchromiumcontent 67.0.3396.103

Verified on Windows x64 with
• 0.23.34 a471718
• Muon 7.1.6
• libchromiumcontent 67.0.3396.103

@kjozwiak
Copy link
Member

+1 from a private PM on community.brave... mentioned that it's been fixed and will be released sometime Monday 👍

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