Skip to content
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

BlockUI: The content outerHeight is not computed #9496

Closed
vonnai opened this issue Dec 13, 2022 · 19 comments · Fixed by #9520 or #9961
Closed

BlockUI: The content outerHeight is not computed #9496

vonnai opened this issue Dec 13, 2022 · 19 comments · Fixed by #9520 or #9961
Assignees
Labels
🐞 defect Bug...Something isn't working workaround A workaround has been provided
Milestone

Comments

@vonnai
Copy link
Contributor

vonnai commented Dec 13, 2022

Describe the bug

Jquery cannot compute the height of the element that the parent element has display:none.

<div style="position:absolute; display:none; height:100px; width:100px; top:20px; left:40px;" class="ui-blockui">
  <div style="position:absolute; display:none" class="ui-blockui-content">
    <span>In progress</span>
  </div>
</div>

The ui-blockui-content has the wrong position.

  /**
    * Align the overlay so it covers its target component.
    * @private
    */
    alignOverlay: function() {
        ...

        //center position of content
        for (var i = 0; i < this.target.length; i++) {
            ...

            blocker.css(sizeAndPosition);

            content.css({
                'left': ((blocker.width() - content.outerWidth()) / 2) + 'px',
                'top': ((blocker.height() - content.outerHeight()) / 2) + 'px', // THE OUTERHEIGHT IS NOT COMPUTED
                'z-index': PrimeFaces.nextZindex()
            });
        }

The ui-blockui-content element is inner because there is the implementation of multiple targets, maybe it's not correct. In the render function, the content is appended to the ui-blockui element and it's wrong.

The blockUI implementation in PF 11 works well, the height of the ui-blockui-content is computed correctly.

Reproducer

  1. https://www.primefaces.org/showcase/ui/misc/blockUI.xhtml?jfwid=06f1d
  2. open developer tools
  3. open the components.js file
  4. put the breakpoint to the 2837 row ('top': ((blocker.height() - content.outerHeight()) / 2) + 'px',)
  5. change the page in the Custom content table
  6. check the outerHeight()

or

  1. https://www.primefaces.org/showcase/ui/misc/blockUI.xhtml?jfwid=06f1d
  2. open developer tools, console
  3. execute $('#j_idt343\\:j_idt365').outerHeight();
  4. it returns 0

Expected behavior

The content of the blocker has the correct position, it's well-centered.

PrimeFaces edition

Community

PrimeFaces version

12.0.0

Theme

No response

JSF implementation

Mojarra

JSF version

2.3

Java version

11

Browser(s)

Chromium 106.0.5249.119

@vonnai vonnai added ‼️ needs-triage Issue needs triaging 🐞 defect Bug...Something isn't working labels Dec 13, 2022
@melloware
Copy link
Member

Please provide an executable example using the PrimeFaces Test project. It is the only way developers can debug your problem to help.

It worked in 11.0 before we refactored to prevent duplicate DOM ID's. the old way was kinda hacky and not guaranteed to work in future browsers: #8548

Any PR or fix you come up with will be appreciated.

@melloware melloware removed the ‼️ needs-triage Issue needs triaging label Dec 13, 2022
@vonnai
Copy link
Contributor Author

vonnai commented Dec 14, 2022

Hi, I provided two working reproducers. I don't think more things should be done. It's really clear and simple. As I described above, JQUERY cannot compute the height of the element that parent has display:none. Your solution in blockui.js relies on something that really doesn't work. I think you should know that because more bugs can be created in the future.

The 11.0 works fine because the .ui-blockui-content is not nested in the .ui-blockui element. I don't think I have too many skills to fix this bug. I don't know why it's nested. Maybe the author should check it.

@melloware
Copy link
Member

melloware commented Dec 14, 2022

@vonnai but the Custom Content example is working for me? What I am asking is do you have something that shows me something is broken? I can page that datatable fine? I can scroll up and down the page and scroll that datatable and the block ui is fine? I can resize the browser and the block UI on datatable works fine page to page?

I am asking for HOW to reproduce an actual bug. Yes I can see $('#j_idt343\\:j_idt365').outerHeight(); = 0 but the showcase is working fine so I can't see anything is wrong to debug it.

@melloware melloware added Status: Needs Reproducer Needs a reproducer showing the issue and removed 🐞 defect Bug...Something isn't working labels Dec 14, 2022
@vonnai
Copy link
Contributor Author

vonnai commented Dec 16, 2022

So I can try to explain what is wrong. Let's imagine we have the blocker 500x500px. The inner content has size 200x200px; The position of the inner content should be centered, the position should be top: 150px; left: 150px.
But the computed left, top values are different.

content.css({
                'left': ((500 - 0) / 2) + 'px', //  250px
                'top': ((500 - 0) / 2) + 'px', //  250px
            });

The content is not well-centered. You can see that in the showcase - custom content table, please, don't tell me, that you really don't see that the loader content of the blocked area has the wrong position. The loader content of the blocker is not well-centered. The bigger the size of the content, the bigger the error in calculating the position left, top.

@melloware
Copy link
Member

@vonnai I do not. Looks perfectly centered to me?

image

@melloware
Copy link
Member

When I resize my browser still perfectly centered:
image

@vonnai
Copy link
Contributor Author

vonnai commented Dec 16, 2022

The square with the loader icon is not centered precisely in the blocked area.
blockui

@melloware
Copy link
Member

OK I thought you were saying there was some way off error. Visually it looks centered to me but you are saying down to the pixel its not perfectly centered. I see now.

@melloware melloware added 🐞 defect Bug...Something isn't working and removed Status: Needs Reproducer Needs a reproducer showing the issue labels Dec 16, 2022
@vonnai
Copy link
Contributor Author

vonnai commented Dec 16, 2022

thanks a lot, it's not centered perfectly due to misuse of the outerHeight, outerWidth on the element that the parent has display:none.

melloware added a commit to melloware/primefaces that referenced this issue Dec 18, 2022
@melloware
Copy link
Member

melloware commented Dec 18, 2022

OK can you try this MonkeyPatch:

if (PrimeFaces.widget.BlockUI) {
    PrimeFaces.widget.BlockUI.prototype.alignOverlay = function() {
        this.target = PrimeFaces.expressions.SearchExpressionFacade.resolveComponentsAsSelector(this.cfg.block);
        if (this.blocker) {
            this.blocker.css('z-index', PrimeFaces.nextZindex());
        }

        //center position of content
        for (var i = 0; i < this.target.length; i++) {
            var currentTarget = $(this.target[i]),
                blocker = $(this.blocker[i]),
                content = $(this.content[i]);

            // configure the target positioning
            var position = currentTarget.css("position");
            if (position !== "fixed" && position !== "absolute") {
                currentTarget.css('position', 'relative');
            }

            // set the size and position to match the target
            var height = currentTarget.height(),
                width = currentTarget.width(),
                offset = currentTarget.offset();
            var sizeAndPosition = {
                'height': height + 'px',
                'width': width + 'px',
                'left': offset.left + 'px',
                'top': offset.top + 'px'
            };
            blocker.css(sizeAndPosition);

            var contentHeight = content.outerHeight();
            var contentWidth = content.outerWidth();
            // #9496 if display:none then we need to clone to get its dimensions
            if (content.height() <= 0) {
                var currentWidth = this.content[i].getBoundingClientRect().width;
                var styleWidth = currentWidth ? 'width: ' + currentWidth + 'px' : '';
                var clone = this.content[i].cloneNode(true);
                clone.style.cssText = 'position: fixed; top: 0; left: 0; overflow: auto; visibility: hidden; pointer-events: none; height: unset; max-height: unset;' + styleWidth;
                document.body.append(clone);
                var jqClone = $(clone);
                contentHeight = jqClone.outerHeight();
                contentWidth = jqClone.outerWidth();
                jqClone.remove();
            }

            content.css({
                'left': ((blocker.width() - contentWidth) / 2) + 'px',
                'top': ((blocker.height() - contentHeight) / 2) + 'px',
                'z-index': PrimeFaces.nextZindex()
            });
        }
    }
}

@vonnai
Copy link
Contributor Author

vonnai commented Mar 24, 2023

Hi, I have checked the fix, but it seems it still doesn't work well.

The outerHeight, outerWidth respects padding of .ui-block-ui-content. The primefaces default padding is 1 rem. The if condition for contentHeight will never be passed. Could you change it to height(), width()?

var contentHeight = content.outerHeight();
var contentWidth = content.outerWidth();
// #9496 if display:none then we need to clone to get its dimensions
if (contentHeight === 0) {

@melloware
Copy link
Member

I wonder if it should do this since we still need to block outerHeight.

var contentHeight = content.outerHeight();
var contentWidth = content.outerWidth();
// #9496 if display:none then we need to clone to get its dimensions
if (content.height() === 0) {

@vonnai
Copy link
Contributor Author

vonnai commented Mar 24, 2023

It works well if I remove this part:

var currentWidth = this.content[i].getBoundingClientRect().width;

currentWidth is always 0.

style without width:

clone.style.cssText = 'position: fixed; top: 0; left: 0; overflow: auto; visibility: hidden; pointer-events: none; height: unset; max-height: unset;';

@melloware
Copy link
Member

OK how about this...

var currentWidth = this.content[i].getBoundingClientRect().width;
var styleWidth = currentWidth ? 'width: ' + currentWidth + 'px' : '';
var clone = this.content[i].cloneNode(true);
clone.style.cssText = 'position: fixed; top: 0; left: 0; overflow: auto; visibility: hidden; pointer-events: none; height: unset; max-height: unset;' + styleWidth;

just to defend if width is 0 then it won't add any width but if width does have value then we append it defensively. Can you try that?

melloware added a commit to melloware/primefaces that referenced this issue Mar 24, 2023
@vonnai
Copy link
Contributor Author

vonnai commented Mar 24, 2023

of course

@melloware
Copy link
Member

Awesome I updated the MonkeyPatch above with all these changes in case others want to use it. Waiting for build to finish and I will merge these changes

@vonnai
Copy link
Contributor Author

vonnai commented Mar 24, 2023

it works fine. Thanks a lot.

@vonnai
Copy link
Contributor Author

vonnai commented Mar 31, 2023

Hello, I have still a problem with the blocker. The code content.height() returns a negative value, due padding option on .ui-blockui-content. So the if condition with making clone is never passed. I suggest changing the condition if (content.height() === 0) { to if (content.height() <= 0) {. What do you think about this fix?

The default padding of .ui-blockui-content is 1em.
Thanks.

@melloware
Copy link
Member

I almost did that originally but thought it would never be negative. 😄 Let me fix

melloware added a commit that referenced this issue Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 defect Bug...Something isn't working workaround A workaround has been provided
Projects
None yet
2 participants