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

Equal heights: Fix eqht-trgt row detection #9589

Conversation

EricDunsworth
Copy link
Member

The plugin had trouble detecting new rows when using the eqht-trgt class. That sometimes caused it to think all of its grids were on the same row and make them as tall as possible. That's undesirable and is inconsistent with "regular" implementations of the plugin.

For instance:

  • The heights of linearized grid layouts were needlessly increased in smaller views.
  • If a plugin container had enough grids to split across multiple rows, every grid's height was derived from the tallest row's tallest grid... as opposed to the current row's tallest grid.

The cause of this behaviour was the offsetTop property:

  • Everything worked fine in "regular" equal heights implementations since all the equalized elements always have the same parent. The rowTop and currentChildTop variables that get compared to determine if a new row is starting were based on the offsetTop property. Since everything being compared are direct children of the plugin container, their offsetParents (which offsetTop is derived from) were always the same. So all the offsets in contention used the same baseline.
  • eqht-trgt was flawed by comparison. Why? Because the "targeted" equalized elements can have different parents. As a result, the new row comparison logic wasn't always comparing the same "kinds" of offsetTops. If a new row was starting and rowTop + currentChildTop had the same offsetTop relative to their respective offsetParents... the comparison logic would get tricked into thinking everything was still on the same row. So it would treat all equalized elements as belonging to the same row.

This fixes the flaw by replacing offsetTop with a calculation representing each equalized element's top offset at the document-level. So the new row comparison logic should now function the "correct" way all the time.

Screenshots...

  • Extra-small view:
    Before After
    eqht-trgt-xs-before eqht-trgt-xs-after
  • Small view:
    Before After
    eqht-trgt-sm-before eqht-trgt-sm-after

The plugin had trouble detecting new rows when using the eqht-trgt class. That sometimes caused it to think all of its grids were on the same row and make them as tall as possible. That's undesirable and is inconsistent with "regular" implementations of the plugin.

For instance:
* The heights of linearized grid layouts were needlessly increased in smaller views.
* If a plugin container had enough grids to split across multiple rows, every grid's height was derived from the tallest row's tallest grid... as opposed to the current row's tallest grid.

The cause of this behaviour was the offsetTop property:
* Everything worked fine in "regular" equal heights implementations since all the equalized elements always have the same parent. The rowTop and currentChildTop variables that get compared to determine if a new row is starting were based on the offsetTop property. Since everything being compared are direct children of the plugin container, their offsetParents (which offsetTop is derived from) were always the same. So all the offsets in contention used the same baseline.
* eqht-trgt was flawed by comparison. Why? Because the "targeted" equalized elements can have different parents. As a result, the new row comparison logic wasn't always comparing the same "kinds" of offsetTops. If a new row was starting and rowTop + currentChildTop had the same offsetTop relative to their respective offsetParents... the comparison logic would get tricked into thinking everything was still on the same row. So it would treat all equalized elements as belonging to the same row.

This fixes the flaw by replacing offsetTop with a calculation representing each equalized element's top offset at the document-level. So the new row comparison logic should now function the "correct" way all the time.
@EricDunsworth
Copy link
Member Author

FWIW we recently ran into a situation that could only be handled with eqht-trgt that was affected by this bug. Flexbox and grid CSS were unable to save the day.

Check out this page:
Canada and the Sustainable Development Goals

Somewhere in the middle of the content, there are two side-by-side sections containing headings and media players. The first heading is long (2 lines), whereas the other is super short (1 line).

eqht-trgt was used to equalize the headings to prevent their media players from looking misaligned in larger viewports. But the second heading's min-height wasn't decreasing on mobile... resulting in a large empty space appearing between that heading and its media player.

@duboisp
Copy link
Member

duboisp commented May 15, 2023

Pre-approved upon testing and review

@duboisp duboisp self-requested a review May 15, 2023 14:38
@duboisp duboisp self-assigned this May 15, 2023
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Reviewed and tested locally,

Work as expected.

@duboisp
Copy link
Member

duboisp commented May 15, 2023

@EricDunsworth have you tried with our pure CSS version of the equal height plugin? It does support the eqht-trgt feature and are more performant as it leverage CSS rather than JS.

@duboisp duboisp merged commit 70e0f30 into wet-boew:master May 15, 2023
2 checks passed
@EricDunsworth
Copy link
Member Author

@EricDunsworth have you tried with our pure CSS version of the equal height plugin? It does support the eqht-trgt feature and are more performant as it leverage CSS rather than JS.

@duboisp I finally got around to trying this out in a test page. It mostly-worked... but one of the hght-inhrt class' properties messed up the layout :(.

Here's a simplified code sample:

<div class="row wb-eqht-grd">
	<section class="col-md-6">
		<h4 class="eqht-trgt">Canada’s 2022 National Statement to the United Nations Canada’s 2022 National Statement to the United Nations Canada’s 2022 National Statement to the United Nations Canada’s 2022 National Statement to the United Nations Canada’s 2022 National Statement to the United Nations</h4>
		<p>Content</p>
	</section>
	<section class="col-md-6">
		<h4 class="eqht-trgt">Together|<span lang="fr">Ensemble</span></h4>
		<p>Content</p>
	</section>
</div>

Both H4 headings are using the eqht-trgt class.

The second heading ("Together|Ensemble") is bilingual. Therefore, "Ensemble" was placed within a <span lang="fr"> element.

So far, so good... except the hght-inhrt class' flex-direction: column; property causes the second H4's direct children to be vertically-stacked. So "Together|" and "Ensemble" end up appearing on separate lines.

The only good way I can think of to counteract it would be to backport Bootstrap 4-5's flex-row class into WET and use it alongside the eqht-trgt class.

Thoughts?

EricDunsworth added a commit to EricDunsworth/wet-boew that referenced this pull request 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 pull request 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.
duboisp added a commit that referenced this pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants