Add "view all" option to download bar

[aeruhxi]

Resolve #9090

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).

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

Description

This tries to resolve issue #9090. The solution is still incomplete.
Before:

After:

Problem

However, a new problem arose with responsiveness.

I do not have any idea where in the code and how this hiding of download items is being done. So, I hope I could use some help from here.

[luixxiul]

@Abs-Zero I opened a PR on your repo here: aeruhxi#1

yet I'm not sure how to solve the problem you mentioned, so I left it as-is.

[cezaraugusto]

hi @Abs-Zero I think @luixxiul covered what was missing, is this ready for review? thanks!

[luixxiul]

@cezaraugusto actually not completely. the timing (viewport width) when the downloaded item should be hidden needs fixing. I just fixed the alignment of the close and show all button.

[aeruhxi]

Yes, the problem I mentioned with a gif still has not been fixed. It would help if I knew where the logic of unmounting of download items is written.

[cezaraugusto]

so calculation to show/hide items takes into account the downloadBar button -- in fact the close (x) button in the right. It is set to 22px which is a little more than the button size (18px).

With the change proposed here, we should take into account the view all button as well, so we need to update variable size to take the new button size as well. For reference, code is described here: https://github.com/brave/browser-laptop/blob/master/less/downloadBar.less#L11

For me, the total size including view all option seems to be around 123px. @Abs-Zero can you test if changing var value to that fix the issue? seems to fix for me. If so pls update PR so we can accept it. btw thanks for the help, PR is looking great!

[cezaraugusto]

btw calculation is defined here: https://github.com/brave/browser-laptop/blob/master/app/renderer/components/download/downloadsBar.js#L34 but I think there's no need to change anything there

[aeruhxi]

Thanks for the pointing out the direction. I will test it.

[cezaraugusto]

btw I think this method is not needed

[aeruhxi]

you mean these two lines:

windowActions.setDownloadsToolbarVisible(false)
webviewActions.setWebviewFocused()

?

[cezaraugusto]

only webviewActions.setWebviewFocused(). For parity, we should auto-hide the downloads bar after clicking "show all", as you did above.

[aeruhxi]

All the problems have been fixed now. Thanks for all the help.

[luixxiul]

For me, the total size including view all option seems to be around 123px

the thing is the value varies on every language. the total width requires l10n viewpoint.

[cezaraugusto]

@luixxiul yes that's correct.

We'll need then to get downloadBarButtons width first, no matter the language. I would go with getBoundingClientRect().width method, so we can find the button wrapper width, and with this value, replace downloadsBarButtons so numItems could make the correct calculation.

@Abs-Zero is this do-able to do? don't want to add extra work so ok if not I can jump in and add these changes. Let me know if you need something from us.

[NejcZdovc]

@cezaraugusto we should really avoid using getBoundingClientRect, because we would be calling it on every resize as well if I understand correctly. We are doing this already on a lot of places. Can we use flex and say that left part where we have items should grow automatically and the left part where we have the button should be 1?

[aeruhxi]

@cezaraugusto Yup, I have pushed a commit using getBoundingClientRect. @NejcZdovc I am sorry I don't understand what you said about flex.

[luixxiul]

Cannot read property 'getBoundingClientRect' of null

[cezaraugusto]

@NejcZdovc your assumption is correct re method will be called each time window is resized. But the way downloadsBar items behave is for parity with Chrome. There's no CSS-only way to emulate the same mechanism without cutting off some element -- which would lead us to bad UI.

I think this addition is very cool, even more coming from a contributor. While I agree with you that adding more and more listeners for window resize would lead us to perf decrease I think it is still too small for this change and if I'm wrong, we can always roll back or even ask Brad for a new spec in downloadsBar.

[NejcZdovc]

@cezaraugusto We are now fixing a lot of perf problems and we don't want to introduce new ones. Maybe it looks like a small one, but adding a lot of small ones we get a big problem. getBoundingClientRect is expensive operation. Can you please write a quick summery what you want to achieve and I will take a look. Maybe I find a way with CSS only 😃

[luixxiul]

@NejcZdovc in that case please don't forget to change lang settings to see your fix is avoiding l10n issues, which are also the big problem. thanks!

Stepping away from pixels to align stuff is the golden rule you should never forget (#2096, #5726 and #8954).

I tried to find a pure CSS solution but I couldn't. I assume removing the fixed width of a downloaded item would be that way.

[NejcZdovc]

@luixxiul I am just finishing pause download PR and I will take a look at this one. Thanks for the heads up regarding l10n.

[cezaraugusto]

@NejcZdovc it should work the same way as currently but including a new element ("view all" button). You can also take a look at how Chrome does it, which is the same way as we do.

The first fix I suggested in #9093 (comment) was simple and inexpensive, css-only. However @luixxiul reminded that locale may change button's size, so any value we add would be just a guess.

[aeruhxi]

If perf is going to be a problem, can we not call getBoundingClientRect only once when the download bar is set visible or when the language setting is changed and cache the width. This way we do not have to call getBoundingClientRect on every resize.

[aeruhxi]

@luixxiul Sorry for the broken commit earlier. I did not encounter that error before maybe because a cached bundle was being used.

[cezaraugusto]

hey @Abs-Zero thanks for fixing that.

can you revert 3219cf2 (but keeping webviewActions.setWebviewFocused() removed) and also 8da955c? We just need to make a tweak on --download-bar-buttons variable size like I said before and we're good to go. It is the same thing you did in a94b8ed but just update it from 127px to 140px. It works ok in that size, sorry for confusion.

That said, once done that change it looks good for me, thanks!

[aeruhxi]

Done. 😃

[bsclifton]

Thanks for working hard on this @Abs-Zero (and huge thanks @cezaraugusto, @luixxiul, and @NejcZdovc for the assists). This will be a great touch when it's all wrapped up 😄 👍

[cezaraugusto]

this looks very good. thanks for the help and looking forward to your next contribution ++