h5p-course-presentation icon indicating copy to clipboard operation
h5p-course-presentation copied to clipboard

Scroll bars do not appear on AdvancedText objects

Open usernamenumber opened this issue 9 years ago • 3 comments

cp.css contains the following:

.h5p-course-presentation .h5p-element .h5p-element-inner:not(.h5p-interactive-video) {
  overflow: hidden;
  overflow-y: auto;
  background: transparent;
}

...which suggests that the intention is for a scrollbar to appear when there is, for example, more text in an AdvancedText than will fit (which I definitely need for some modules). However, for reasons that aren't entirely clear to me, at least in Chrome, this doesn't actually happen unless the element also has height: 100%;.

Setting this doesn't actually affect the size of the element, since "100%" just means to fill the .h5p-element-outer container, which is looks like it would normally do anyway? I haven't seen any instances of having more than one inner element in an outer, at least.

Assuming that's true, it looks changing the stylesheet to the following would cause scrollbars to appear as expected without breaking anything else:

.h5p-course-presentation .h5p-element .h5p-element-inner:not(.h5p-interactive-video) {
  height: 100%;
  overflow: hidden;
  overflow-y: auto;
  background: transparent;
}

Does that sound reasonable?

usernamenumber avatar Mar 07 '16 20:03 usernamenumber

I do not know why it doesn't have height 100 %, but I know that scrollbars are not supposed to show up for Advanced text. The reason for this is that the tiniest change in font renderings, styling, unwanted br or empty p tags etc. might make unwanted ugly scroll bars to appear. We plan to add checkboxes to advanced text in the future so that users may control whether or not scrollbars should be enabled.

falcon-git avatar Mar 08 '16 09:03 falcon-git

This might be worth it to me to implement myself soonish, as I think I understand the internals well enough now to do it. I am working on a tool that generates H5Ps from pre-existing content, so having some kind of option for enabling scroll bars is very important for me.

If I did this, any objections to using height:100% as the solution when that box is checked? I've tested it under Chrome, Firefox, and Safari and it works in all three. Haven't gotten to check in IE yet, but no reason to expect it not work there too. On Mar 8, 2016 4:29 AM, "Svein-Tore Griff With" [email protected] wrote:

I do not know why it doesn't have height 100 %, but I know that scrollbars are not supposed to show up for Advanced text. The reason for this is that the tiniest change in font renderings, styling, unwanted br or empty p tags etc. might make unwanted ugly scroll bars to appear. We plan to add checkboxes to advanced text in the future so that users may control whether or not scrollbars should be enabled.

— Reply to this email directly or view it on GitHub https://github.com/h5p/h5p-course-presentation/issues/19#issuecomment-193686986 .

usernamenumber avatar Mar 08 '16 13:03 usernamenumber

There is a selector it seems that is like this:

.h5p-course-presentation .h5p-element .h5p-element-outer.h5p-advancedtext-outer-element, .h5p-course-presentation .h5p-element .h5p-element-outer.h5p-audio-outer-element

It sets overflow-y: hidden

I think you should consider changing that one instead. Also I think this is a property that doesn't belong in the advanced text content type but in CP, and it might be a bit hard to make it only show up for advanced text, but fully possible. A PR will be well received :)

falcon-git avatar Mar 08 '16 13:03 falcon-git