Closed m-henderson closed 2 years ago
@rebron can we add "good first issue" label to this bug sense it's simply spacing of the UI elements?
Hello, I tried looking into the issue. Because the br-toolbar-search-field is position absolute, the margin auto on the overlay does not function as expected. If the div with id overlay-content is overwritten to include a margin-right of 24px than the issue is fixed. The issue I'm having is: where are the page files located to do this override? I have looked at the file brave-browser/src/brave/ui/webui/resources/br_toolbar_selection_overlay_overrides.css.
Hi guys! I've been taking a look into this since yesterday and I think I've found a more suitable (and very simple) solution to this.
First of all, @crescit I feel your pain 😅 It took me some time to find the code responsible for rendering the #overlay-content
div. But I did find it, and it actually comes from Chromium! Brave just uses it as is, apart for that minor z-index tweak you saw at browser/src/brave/ui/webui/resources/br_toolbar_selection_overlay_overrides.css
. If you want to see the div, it's here: https://github.com/chromium/chromium/blob/f9b300f814/ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.html.
Second, since this is a component from the default Chromium implementation, I tried to find a way to solve this without needing to change any CSS on that overlay.
I fired up Google Chrome to see how they did it, and they simply don't enable users to search in their history after they've selected any items. At first, I thought "Ok, this is a feature that Braves supports, and Chrome doesn't, nice!", but then I tested it on Brave and it doesn't support this correctly either! If you select an item from your history, and then search and try to select new items, all selected items get unselected once you close the search input. I think this might be a UX issue, and by displaying the little search-icon and enabling users to search while selecting history items, user's just get confused as the items they've selected get unselected.
So, finally, my solution is to prevent Brave from displaying the search-icon once a user selects a history item, letting the overlay completely hide it. And to achieve this, I just had to remove a z-index
attribute that was added to the .page-search_label
CSS class.
I'll open a pull-request in a bit, just running the build process to make sure it works as expected :)
@rebron Just created the PR! But it looks like I can't ask for reviews. How should I proceed?
@victorhmp I can help with that 😄 Will check that out soon
@bsclifton That’s awesome!! Thanks! 😄
www.woodman-decor.com thats soooo goood
When trying to be fancy, please don't kill the UX.
At this point, when using the search, there's no "Delete" button. Is this "a bug or a feature"?
I'd like to work on this and have read the code too and steps to contribute please assign this to me . regards, lakshita
@lakshita15 assigned the issue to you. Please open a PR with your fix
Does not reproduce with
Brave | 1.29.65 Chromium: 92.0.4515.159 (Official Build) dev (x86_64)
-- | --
Revision | 0185b8a19c88c5dfd3e6c0da6686d799e9bc3b52-refs/branch-heads/4515@{#2052}
OS | macOS Version 10.15.7 (Build 19H1323)
Description
The delete button and search icon UI elements within the history screen are overlapping on smaller screens.
Steps to Reproduce
Put a checkmark in one of the boxes of the browsing history:![image](https://user-images.githubusercontent.com/16070192/77701399-2f690780-6f84-11ea-9d62-456f0454543b.png)
Actual result:
The search and delete button overlap![image](https://user-images.githubusercontent.com/16070192/77701613-b6b67b00-6f84-11ea-9e3f-6972f627d88a.png)
Expected result:
I expected the search and delete button to be responsive and have proper spacing even on smaller screens.
Reproduces how often:
Easily reproduced
Brave version (brave://version info)
Version/Channel Information:
Other Additional Information:
Miscellaneous Information:
The issue was first reported within the forum. https://community.brave.com/t/ui-elements-overlapping-each-other/115060