Skip to content

XWIKI-23332: The width of the required rights modal depends on the displayed rights #4322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -138,34 +138,20 @@
display: none;
}

/* Switch rows and columns for small screens. The width uses em as the required width depends on the font size. */
@media (max-width: 42em) {
#required-rights-dialog .rights-selection ul {
grid-template-columns: repeat(2, minmax(max-content, 1fr));
grid-template-rows: repeat(auto-fill, 1fr);
}

#required-rights-dialog .rights-selection li {
grid-column: span 2;
grid-row: span 1;
}

#required-rights-dialog .rights-selection li p {
text-align: start;
margin: auto 0;
}
}

/* In "wide" layout, make the dialog wide enough to fit the required rights selection. */
@media (min-width: 42em) {
#required-rights-dialog .modal-dialog {
min-width: min-content;
}

/* When setting the minimum width to min-content, the required rights results shouldn't contribute to that width. */
#required-rights-dialog #required-rights-results {
contain: inline-size;
}
/* Vertical layout for when the dialog isn't wide enough to display the rights horizontally. */
#required-rights-dialog .rights-selection ul.vertical {
grid-template-columns: repeat(2, minmax(max-content, 1fr));
grid-template-rows: repeat(auto-fill, 1fr);
}

#required-rights-dialog .rights-selection ul.vertical li {
grid-column: span 2;
grid-row: span 1;
}

#required-rights-dialog .rights-selection ul.vertical li p {
text-align: start;
margin: auto 0;
}

/* Styles for the warning box above the content. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ define('xwiki-requiredrights-dialog', [
event.preventDefault();
this.save();
});

this.#setupResponsiveLayoutInitialization();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this method is called to perform something equivalent to the removed CSS, and to avoid having hardcoded width in the media queries, but it's not so clear to me by reading the code.
Also wdyt of explaining the technical constraints forcing us to do something we usually do in CSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment below #setupResponsiveLayout that might explain what we do here. See also my comments in the PR description, I'm not 100% sure it's the best way to do it.

}

toggleAdvanced()
Expand Down Expand Up @@ -444,6 +446,128 @@ define('xwiki-requiredrights-dialog', [

this.analysisResultsContainer.appendChild(panelGroup);
}

/**
* @returns {number} the minimum width of the rights list in horizontal layout
*/
#computeMinWidth()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about this # prefix. Is it a new naming rule I missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_properties - I'm not sure if we've used them yet but to me, it seemed logical to use them as they are relatively widely supported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I'm used to the private typescript keyword, but indeed the JavaScript equivalent seems widely adopted https://caniuse.com/mdn-javascript_classes_private_class_fields

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the method also does not state clearly that it has side effects on this.rightsList

{
// Temporarily set width to min-content to get the scrollWidth
const prevWidth = this.rightsList.style.width;
this.rightsList.style.width = 'min-content';
// Force reflow
this.rightsList.offsetWidth;
const minWidth = this.rightsList.scrollWidth;
this.rightsList.style.width = prevWidth;
return minWidth;
}

/**
* Update the class of the rights list to 'vertical' if the available width is less than the minimum width.
*
* @param minWidth the minimum width of the rights list in horizontal layout
*/
#updateVerticalClass(minWidth)
{
const availableWidth = this.rightsList.clientWidth;

// Only update the class if the available width is not zero, i.e., the element is visible.
if (availableWidth === 0) {
return;
}

if (minWidth > availableWidth) {
this.rightsList.classList.add('vertical');
} else if (minWidth < availableWidth - 1) {
// Add some tolerance to avoid flickering.
this.rightsList.classList.remove('vertical');
}
}

/**
* Debounce a function to limit how often it can be called.
*
* @param fn the function to debounce
* @param delay the delay in milliseconds
* @returns {(function(...[*]): void)|*} a debounced version of the function
*/
#debounce(fn, delay)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we don't have an existing debounce method we can import from somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found two other places where local debounce methods were declared but nothing I could import.

{
let timeoutId;
return function (...args) {
clearTimeout(timeoutId);
timeoutId = setTimeout(() => fn.apply(this, args), delay);
};
}

/**
* Set up the responsive layout for the rights list. Must be called after the dialog is shown.
*
* We use JavaScript to manage the responsive layout as we don't know in advance the width of the rights
* list in horizontal layout, and thus we can't define a breakpoint in CSS using a media/container query.
* This could be reconsidered when CSS environment variables become available which should greatly simplify
* this code.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the comment that hopefully explains why we use this method.

*/
#setupResponsiveLayout()
{
// Compute the minimum width once, after DOM is ready and content is rendered
let minimumWidth = this.#computeMinWidth();
// Initial update
this.#updateVerticalClass(minimumWidth);

// Debounced resize handler
const onResize = this.#debounce(() => {
this.#updateVerticalClass(minimumWidth);
}, 100);
window.addEventListener('resize', onResize);

// When the radio button changes, the rights list might become visible. In this case, we need to update
// the layout.
const enforceRadios = this.dialogElement.querySelectorAll('input[name="enforceRequiredRights"]');
enforceRadios.forEach(radio => {
radio.addEventListener('change', () => {
// Use setTimeout to run after the CSS display has been updated
setTimeout(() => {
if (minimumWidth === 0) {
// Recompute the minimum width if it was zero, which is the case when the rights list
// wasn't visible before.
minimumWidth = this.#computeMinWidth();
}
this.#updateVerticalClass(minimumWidth);
}, 0);
});
});

// Clean up on dialog close as the dialog is destroyed when it is closed.
$(this.dialogElement).on('hidden.bs.modal', () => {
window.removeEventListener('resize', onResize);
});
}

/**
* Set up the responsive layout initialization. This method needs to be called after the HTML structure has
* been created but before the dialog is shown.
*/
#setupResponsiveLayoutInitialization()
{
// Wait for the "in" class to be added to the dialog element to setup the responsive layout as early as
// possible, before the animation ends.
const classObserver = new MutationObserver(mutationList => {
mutationList.forEach((mutation) => {
if (mutation.target.classList.contains('in')) {
this.#setupResponsiveLayout();
// Unregister the observer after the first mutation to avoid multiple calls.
// The setup of the responsive layout only needs to be called once for the dialog.
classObserver.disconnect();
}
});
});

classObserver.observe(this.dialogElement, {
attributes: true,
attributeFilter: ['class']
});
}
}

return {
Expand Down