You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Due to all this, in Nextcloud 19 and lower the footer is found and hidden both when the Talk app is enabled and disabled. In Nextcloud 20-22 the footer is hidden when the Talk app is enabled, but not when it is disabled. And in Nextcloud 23 and later it is hidden when the Talk app is disabled, but not when it is enabled. But wait, the fun has not ended yet ;-)
However, as Talk modifies a bit the public share page layout, it also adjust the CSS rules to accommodate that change. These adjustments are defined in an SCSS file, so in master they currently have no effect due to the SCSS deprecation. The CSS style will be fixed in Talk at a later point, so once that happens it should be checked what else needs to be adjusted in the PDF viewer, if anything.
In previous versions, on the other hand, when the footer is hidden the styles already need to be fixed. In Nextcloud 24 the main content height is wrong when Talk sidebar is shown, and right when Talk sidebar is not shown. But in Nextcloud 23 and 22 it is wrong when Talk sidebar is not shown and right when it is shown.
The problem is that both the PDF viewer and Talk need to set the main content height to different values, the PDF viewer to account for the hidden footer and Talk to account for the layout change. Due to the layout change the style set by Talk is compatible both with a visible and a hidden footer, but the PDF viewer sets the height directly on the main content element, so it overrides the style set by Talk (in Nextcloud 22 and 23 no height is set by the PDF viewer, so that is why the main content height is wrong when the Talk sidebar is not shown).
Note that this can not be fixed like it was done in the past, that is, by setting a CSS class on the main content element and then overriding the CSS rules if needed from Talk. It seems that #element.class and #ancestor .class have the same specificity, so the one declared later is the one that takes effect. The PDF stylesheet was loaded during boot, while the Talk stylesheet is loaded before the template is rendered. This caused the CSS rules defined by Talk to appear after the PDF viewer ones, and thus override them. But now that the #pdframe element is no longer needed the style should be loaded by the PDF viewer too before the template is rendered, so it could happen that the PDF viewer style was loaded after the Talk style (at least, it happened when I was testing it) and thus the proper height would be overriden.
No matter if the Talk sidebar is shown or not the main content height just needs to fill the available space, so now this is done using flexbox. This avoids having to set an explicit height and thus works for both the standard layout and the Talk sidebar layout. Moreover, even if an explicit height is set somewhere, like in the server or Talk styles, the flex-grow property implicitly overrides it and ensures that the main content will fill the available height.
Finally, PDF.js 2.14.305 does not work for me (TypeError: WeakMap key must be an object, got dialog is shown in the browser console when opening the PDF, I have not investigated it further), so I tested the fix for master in the previous version (2afe14e^) and then rebased and rebuilt on latest master.
The style.css file was no longer loaded since the PDF viewer was
adjusted to use the standard viewer app, so it can be removed.
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When the Talk sidebar is shown the footer in public pages is moved into
the "#app-content" element, so this needs to be taken into account when
hiding the footer element.
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Honestly, this was sure something bad was gonna happen.
Apps randomly changing layout is prone to errors and is bad coding practice I'd say :/
No idea what was the reasoning behind this, but that seems far too arbitrary and not properly integrated 😕
Finally, PDF.js 2.14.305 does not work for me (TypeError: WeakMap key must be an object, got dialog is shown in the browser console when opening the PDF, I have not investigated it further), so I tested the fix for master in the previous version (2afe14e^) and then rebased and rebuilt on latest master.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #611
Since the introduction of the Talk sidebar in Talk 7 when Talk sidebar is shown the footer in public pages has been moved into the #app-content element to avoid the footer appearing below the Talk sidebar. In the PDF viewer, before it was moved to use the standard viewer, the footer element was queried with just
footer, so it affected any footer element regardless of where it was in the DOM. When the PDF viewer was moved to use the standard viewer the footer query was changed, probably because the original one was too broad, and set to#app-content > footerinstead. This was later changed tobody > footer, and reverted back to#app-contenton 22, 21 and 20.Due to all this, in Nextcloud 19 and lower the footer is found and hidden both when the Talk app is enabled and disabled. In Nextcloud 20-22 the footer is hidden when the Talk app is enabled, but not when it is disabled. And in Nextcloud 23 and later it is hidden when the Talk app is disabled, but not when it is enabled. But wait, the fun has not ended yet ;-)
In the past, to compensate for the hidden footer, the PDF viewer set the content height to 100% by setting the
full-heightclass. Since the PDF viewer was moved to use the standard viewer the main stylesheet is no longer loaded and the class is no longer set either. This caused the bug fixed in #611 by explicitly settingmin-heightin the element to 100% (note that in the past the modified attribute washeightrather thanmin-height, and it was done using a CSS class rather than setting it in the element itself).However, as Talk modifies a bit the public share page layout, it also adjust the CSS rules to accommodate that change. These adjustments are defined in an SCSS file, so in master they currently have no effect due to the SCSS deprecation. The CSS style will be fixed in Talk at a later point, so once that happens it should be checked what else needs to be adjusted in the PDF viewer, if anything.
In previous versions, on the other hand, when the footer is hidden the styles already need to be fixed. In Nextcloud 24 the main content height is wrong when Talk sidebar is shown, and right when Talk sidebar is not shown. But in Nextcloud 23 and 22 it is wrong when Talk sidebar is not shown and right when it is shown.
The problem is that both the PDF viewer and Talk need to set the main content height to different values, the PDF viewer to account for the hidden footer and Talk to account for the layout change. Due to the layout change the style set by Talk is compatible both with a visible and a hidden footer, but the PDF viewer sets the height directly on the main content element, so it overrides the style set by Talk (in Nextcloud 22 and 23 no height is set by the PDF viewer, so that is why the main content height is wrong when the Talk sidebar is not shown).
Note that this can not be fixed like it was done in the past, that is, by setting a CSS class on the main content element and then overriding the CSS rules if needed from Talk. It seems that
#element.classand#ancestor .classhave the same specificity, so the one declared later is the one that takes effect. The PDF stylesheet was loaded during boot, while the Talk stylesheet is loaded before the template is rendered. This caused the CSS rules defined by Talk to appear after the PDF viewer ones, and thus override them. But now that the #pdframe element is no longer needed the style should be loaded by the PDF viewer too before the template is rendered, so it could happen that the PDF viewer style was loaded after the Talk style (at least, it happened when I was testing it) and thus the proper height would be overriden.No matter if the Talk sidebar is shown or not the main content height just needs to fill the available space, so now this is done using flexbox. This avoids having to set an explicit height and thus works for both the standard layout and the Talk sidebar layout. Moreover, even if an explicit height is set somewhere, like in the server or Talk styles, the flex-grow property implicitly overrides it and ensures that the main content will fill the available height.
Finally, PDF.js 2.14.305 does not work for me (
TypeError: WeakMap key must be an object, got dialogis shown in the browser console when opening the PDF, I have not investigated it further), so I tested the fix for master in the previous version (2afe14e^) and then rebased and rebuilt on latest master.