Fixed scrollbar disappearing after changing content

[s77rt]

Description:

If a scrollbar content is changed, the whole widget content will be replaced (including the scrolling bars, shadows) with just the new content.
The fix is to change only the content of the scrollbar (keeping other objects untouched: scrollbars, shadows, etc)

Fixes #2268

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[s77rt]

DocTabs didn't have any scrollbars, making this change will fix the mentioned issue, but will also make DocTabs able to have scrollbars
I like how DocTabs look without scrollbars however with scrollbars it does not look as good (having two bars, bottomline of DocTabs and the scrollbar)

any ideas?

[andydotxyz]

If content is scrollable it should have scroll bars I think. Otherwise how do people see that the content can scroll. I don’t know if this is an omission in DocTabs, perhaps @stuartmscott can recall?

[andydotxyz]

The test failures need to be addressed before this can be considered. You’ll see the same issues using go test locally. You’ll need to copy files from testdata/failed over to testdata/ if the change is intended. It then appears as part of the PR change set.

[s77rt]

okay, but what about the design?
it does not look as good as it was, having two bars is not a good thing IMHO

[andydotxyz]

it does not look as good as it was, having two bars is not a good thing IMHO

I go with the idea that scrollbars should be used to indicate scrollable content. I don't think we should remove the shadow line as it will not always be scrollable.
Perhaps we will be forced into adding a "Hide Scrollbars" feature?

[s77rt]

or maybe the scrollbar must have it's own dedicated space, not an overlay

[zdima]

I agree the scroll bar doesn't look good with DocTabs, and perhaps we should disable the scroll bar in DocTabs.
However, the DocTabs implementation seems incomplete. The AppTabs and DocTabs are very much alike. But you will see the difference in how the bottom line is handled. In the AppTabs, when "invisible" tab selected, the bottom line moved under "...". The DocTabs bottom line just disappeared.
Also for both DocTabs and AppTabs, when "invisible" tab selected and user open popup with "...", there is no indication which tab is currently actually.

[andydotxyz]

AppTabs and DocTabs are indeed different. The app tabbing is designed for a more static layout (the main areas of an app at the beginning of a list, others accessible through popup if screen space is too small).
The doc tabs however are designed for lots of documents that may be opened or closed. When you choose an item from the doctabs menu it should scroll in to view.

[Jacalz]

I had a look at this and I think DocTabs certainly look better without the scrollbar. If it isn't too much of a hassle to turn it off for that use case, I think it would be useful to implement that. Is there any broader use case for having that option available in a public API?

[andydotxyz]

If we want this to land in 2.0.4 then we should ignore DocTabs for now, as it does not exist until 2.1... @Jacalz?

[Jacalz]

That is a very good point. We can tackle that separately.

[andydotxyz]

Hey @s77rt we are looking to prep v2.0.4 this week - do you think we could wrap this up (fixing tests etc) and we can review DocTabs as we move to 2.1 after.

[s77rt]

Hi,
should i just copy the new results as the expected results

You’ll need to copy files from testdata/failed over to testdata/ if the change is intended.

[andydotxyz]

should i just copy the new results as the expected results

Yes. When you push that into the PR we can see the change in expected results to confirm what has altered.

[s77rt]

hmm what about mobile tests?

[Jacalz]

You have to run the tests with go test -tags mobile . to get those as well.

[andydotxyz]

Great update thanks. However it looks like one or two of the tests have additional changes.

[andydotxyz]

Apologies some other features have landed so you'll need to merge develop and re-run the tests.
Perhaps the unexpected change goes away then, I'm not sure what caused it.

[s77rt]

this change also produces a new test failure:
TestWindow_HandleOutsideHoverableObject : (testdata has not been replaced for this one) i think this is something just from my side

[andydotxyz]

I have started to see that test fail as well, so I don't think its your fault.

[andydotxyz]

Excellent thanks

[andydotxyz]

Are you happy too @Jacalz ?

[Jacalz]

Yep :)

[andydotxyz]

FYI for future PRs please don't force push after a review is started as it can make it harder to do subsequent review passes.