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

[BUG] Equalize - Update doesn't work anymore #9607

Open
agagnon opened this issue Jun 1, 2023 · 6 comments
Open

[BUG] Equalize - Update doesn't work anymore #9607

agagnon opened this issue Jun 1, 2023 · 6 comments

Comments

@agagnon
Copy link

agagnon commented Jun 1, 2023

I have a page with dynamic content and I used to use $( ".wb-eqht" ).trigger( "wb-update.wb-eqht" ); to update Equalize for visible elements.
It worked with WET 12.6.0, but I switched to 13.2.2 last week and the update seemed to not be working anymore.

I use the eqht-trgt and it seemed that there was a fix for that in version 13.2.1 and it broke the update feature. I actually tried with v13.1.0 and it worked properlly and it is broker in 13.2.1.

Can it be fixed?

@EricDunsworth
Copy link
Member

EricDunsworth commented Jun 14, 2023

@agagnon Does your page happen to be public? If so, would it be possible to provide a link to it? Or maybe a code sample?

I wouldn't of expected #9589's changes to have impacted wb-update.wb-eqht apart from improving how it calculates equal heights when eqht-trgt is in play.

Does your page contain multiple rows of equalized content? Is your expectation to see every grid across every row being based on the tallest row? If so... my PR deliberately undid that behaviour since it was inherently a bug :(.

@agagnon
Copy link
Author

agagnon commented Jul 7, 2023

Here is my page using v13.2.2 (equalize update not working) : https://www.asc-csa.gc.ca/fra/evenements/
Here is the exact same page using v13.1.0 (equalize update working) : https://www.asc-csa.gc.ca/temp/

There is a list of events, filtered. When you change the filter, I use "Equalize update" to make all visible events the same height. With 13.1.0, is works fine. With 13.2.2, it doesn't work and you can see the events are not the same height.

@agagnon
Copy link
Author

agagnon commented Jul 17, 2023

Just updated the temp page because the "/fra/evenements" page has changed.
You can choose filters "Activités" and "Terminés" and you'll see the issue in the "/fra/evenements" page.

@EricDunsworth
Copy link
Member

EricDunsworth commented Jul 25, 2023

@agagnon I think I figured it out :O.

What seems to have happened is that the new vertical offset logic #9589 added doesn't play nicely with hidden (display: none;) elements that are in the scope of equalization.

The equal heights plugin detects "virtual" rows by comparing the vertical offsets of all elements that are to be equalized:

  • In the past, each element's vertical offset was based on their offsetTop property.
  • Equal heights: Fix eqht-trgt row detection #9589 changed it to be based on a calculation (currentChild.getBoundingClientRect().top + window.pageYOffset). That's more reliable when dealing with visible elements... but apparently not with hidden ones :(. The vertical offset of hidden elements seems to be getting derived from the viewport's top scroll position - which causes their vertical baseline to be inconsistent with those of visible elements. End result is that when the plugin encounters a hidden element after a visible element, it gets tricked into thinking a new virtual row is starting and ends up botching its min-height calculations.

I'll try sending over a PR to fix it sometime this week (hopefully tomorrow).

EricDunsworth added a commit to EricDunsworth/wet-boew that referenced this issue Jul 27, 2023
The plugin detects new "virtual" rows by comparing the vertical offsets of each element designated to be equalized. New rows are established when back-to-back elements are detected as having different offsets.

wet-boew#9589 fixed a bug that prevented it from working properly on deeply-nested elements using the eqht-trgt class. How? By changing how offsets were detected into a calculation representing the viewport's vertical scroll position + each element's top offset relative to the viewport.

The new calculation logic worked fine for visible elements, but not on hidden (display: none;) ones. Why? Because hidden elements don't have a top viewport offset. So inconsistent offsets were getting used when comparing visible elements against hidden ones.

End result was that if a visible element was followed by a hidden one, the plugin would end the current row and establish a new one... even if a subsequent visible element was still on the old row. That caused mismatched equalization and potentially-broken float layouts.

This fixes it by modifying the row detection logic to exclude hidden elements (i.e. anything with an offsetHeight of 0).

Partially addresses wet-boew#9607.
EricDunsworth added a commit to EricDunsworth/wet-boew that referenced this issue Jul 27, 2023
The plugin detects new "virtual" rows by comparing the vertical offsets of each element designated to be equalized. New rows are established when back-to-back elements are detected as having different offsets.

wet-boew#9589 fixed a bug that prevented it from working properly on deeply-nested elements using the eqht-trgt class. How? By changing how offsets were detected into a calculation representing the viewport's vertical scroll position + each element's top offset relative to the viewport.

The new calculation logic worked fine for visible elements, but not on hidden (display: none;) ones. Why? Because hidden elements don't have a top viewport offset. So inconsistent offsets were getting used when comparing visible elements against hidden ones.

End result was that if a visible element was followed by a hidden one, the plugin would end the current row and establish a new one... even if a subsequent visible element was still on the old row. That caused mismatched equalization and potentially-broken float layouts.

This fixes it by modifying the row detection logic to exclude hidden elements (i.e. anything with an offsetHeight of 0).

Partially addresses wet-boew#9607.
@EricDunsworth
Copy link
Member

Just sent over #9639. It fixes messed-up equalization caused by hidden grids in-between visible ones. So min-height is set more consistently across rows now.

@agagnon Based on my local testing, it looks like your GCWeb v13.2.2 page has another twist to it that clashes with equalization :P. It's setup to lazy-load images. When a user plays around with the filters, equalization occurs on rows of grids that are off-screen. If many rows of results appear, the off-screen rows will be equalized without taking the images into consideration (since they don't load in unless they're on-screen).

So with my fix in play, everything technically gets equalized properly... but when a user scrolls down and the lazy-loaded images come into play, their grids grow taller :D. So the min-heights the equalization plugin setup earlier on become outdated.

To get around that issue, you'd probably need to do one of the following (from best to worst):

  • Give up on the equal heights plugin and use one of these instead:
  • Enhance the equal heights plugin to automatically re-equalize when heights change in realtime using something along the lines of a DOM mutation observer
  • Stop lazy-loading images (not ideal)

@EricDunsworth
Copy link
Member

EricDunsworth commented Aug 3, 2023

@agagnon Here's what I'd suggest to transition those events pages to flexbox CSS:

  • Events / Événements pages:
    • Change the <div id="list"> element's class attribute to gc-prtts small
    • Change the ul element's class attribute to list-unstyled d-sm-flex flex-sm-wrap
    • Change the li elements' class attribute to blog col-sm-6 col-md-4 mrgn-bttm-sm d-sm-flex
    • Change the article elements' class attribute to d-sm-flex
    • Change the a elements' class attribute to d-sm-flex mrgn-bttm-lg
    • Change the figure elements' class attribute to thumbnail thumbnail_MOD mrgn-bttm-0
    • Bonus - fix HTML validation errors:
      • Both pages:
        • Don't nest <p class="date"> within "parent" p elements
      • French page:
        • Replace the date element with time
        • Remove duplicate </span> closing tag after "Association of Science Teachers"
  • evenements.css
    • Remove display:block; from the second #list a selector (to stop overriding the d-sm-flex class's display: flex; property)
  • evenements.js
    • Remove $( ".wb-eqht" ).trigger( "wb-update.wb-eqht" )

Hope this helps :)!

duboisp added a commit that referenced this issue Aug 15, 2023
#9639)

* Equal heights: Improve row detection (exclude hidden elements)

The plugin detects new "virtual" rows by comparing the vertical offsets of each element designated to be equalized. New rows are established when back-to-back elements are detected as having different offsets.

#9589 fixed a bug that prevented it from working properly on deeply-nested elements using the eqht-trgt class. How? By changing how offsets were detected into a calculation representing the viewport's vertical scroll position + each element's top offset relative to the viewport.

The new calculation logic worked fine for visible elements, but not on hidden (display: none;) ones. Why? Because hidden elements don't have a top viewport offset. So inconsistent offsets were getting used when comparing visible elements against hidden ones.

End result was that if a visible element was followed by a hidden one, the plugin would end the current row and establish a new one... even if a subsequent visible element was still on the old row. That caused mismatched equalization and potentially-broken float layouts.

This fixes it by modifying the row detection logic to exclude hidden elements (i.e. anything with an offsetHeight of 0).

Partially addresses #9607.

* Equal heights - PR 9639 - Add test cases examples

---------

* Minor: Equal heights: Improve row detection by excluding hidden elements


Co-authored-by: Pierre Dubois <pierre.dubois@servicecanada.gc.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants