Skip to content
This repository was archived by the owner on Dec 19, 2024. It is now read-only.

Add an expandSizingTargetForScrollbars property that loosens the width / height constraints if scrollbars appear after the element is positioned.#104

Merged
bicknellr merged 38 commits into
masterfrom
measure-with-scrollbars
Mar 2, 2021
Merged

Add an expandSizingTargetForScrollbars property that loosens the width / height constraints if scrollbars appear after the element is positioned.#104
bicknellr merged 38 commits into
masterfrom
measure-with-scrollbars

Conversation

@bicknellr
Copy link
Copy Markdown
Contributor

@bicknellr bicknellr commented Jan 11, 2021

When iron-fit-behavior takes the initial measurement of the element it intends to position, the element is positioned at position: fixed; top: 0; left: 0; and may have overflow: hidden; applied by (e.g.) iron-dropdown. Both of these situations can cause the element not to have overflowing content when this measurement is taken, even if the element would have overflowing content after it is positioned. If the element does have overflowing content after being positioned and having its max-width / max-height constrained, the size of the added scrollbars take space from the measured natural size of the content and can force the content to wrap awkwardly, even when there is enough space such that this wouldn't be necessary.

This PR works around this issue (in basically the same way as #82) by measuring again after the initial positioning: if a scrollbar appears, it adjusts the position and max-width / max-height to account for this (continuing to respect the fitInto bounds and alignment).

This feature is gated behind the expandSizingTargetForScrollbars property, since expanding the max-width / max-height to account for scrollbars isn't always desirable - for example, if the element being sized is intentionally set to a particular size to align with another element.

Fixes #81.

@google-cla google-cla Bot added the cla: yes label Jan 11, 2021
…ly use the real change in size to adjust position for non-top/left alignments.
`offsetWidth` provides the distance between the outer left border edge to the
outer right border edge. `clientWidth` provides the distance between the outer
left padding edge and the outer right padding edge - the same as `offsetWidth`,
but excluding borders and scrollbars.[^1] The difference between `offsetWidth`
and `clientWidth` includes the size of the scrollbars contributing to (or
consuming) the width.

To measure if a scrollbar was added between two different layout states, we
measure the difference between (`offsetWidth` - `clientWidth`) in each of the
two states. If the border width is constant, then the difference between these
two measurements is the difference between the scrollbar sizes at these two
states.

This can only detect a change in scrollbar size: it can't distinguish between
the case where a scrollbar exists during both measurements and the case where a
scrollbar does not exist during both measurements.

[^1]: This MDN page has some nice diagrams showing exactly what contributes to
each of `offsetWidth` and `clientWidth`:
https://developer.mozilla.org/en-US/docs/Web/API/CSS_Object_Model/Determining_the_dimensions_of_elements
@bicknellr bicknellr marked this pull request as ready for review January 19, 2021 23:07
@bicknellr bicknellr changed the title Loosen the width / height constraints if scrollbars appear after the element is positioned. Add an expandSizingTargetForScrollbars property that loosens the width / height constraints if scrollbars appear after the element is positioned. Jan 19, 2021
Comment thread iron-fit-behavior.js Outdated
};
})();

const scrollbarSize = (() => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This probably wouldn't work in all cases since css can change scrollbar width and the scrollbar can be different on the body compared to the dropdown

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's true. Fortunately, this is only used when the browser has the vertical scrollbar + max-width bug mentioned above and IE 11 doesn't support modifying the scrollbar size (AFAICT), so I think it's ok to cache this value. However, I combined scrollbarSize and hasVerticalScrollbarMaxWidthBug so that nobody later might come along and try to use scrollbarSize in some other case where they might really need to read it live.

@bicknellr
Copy link
Copy Markdown
Contributor Author

bicknellr commented Feb 25, 2021

The 're-request review' button isn't doing anything (maybe a GitHub org membership thing?) so I'll just ping you in the top level comments like this I guess. Anyways, PTAL @helenxywu

Comment thread iron-fit-behavior.js Outdated
// Unlike other browsers, IE11 doesn't support changing the size of scrollbars
// with CSS, so we don't need to read this value live from the element in
// question when trying to work around the bug.
const getVerticalScrollbarMaxWidthBugOffset = (() => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the IIFE? Since this is a module, offset can be a top-level sibling declaration to getVerticalScrollbarMaxWidthBugOffset

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, not strictly necessary, I removed it.

@bicknellr bicknellr force-pushed the measure-with-scrollbars branch from 9887409 to 96eb1e6 Compare March 2, 2021 21:08
@bicknellr bicknellr merged commit 54ce68c into master Mar 2, 2021
@bicknellr bicknellr deleted the measure-with-scrollbars branch March 2, 2021 21:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sizing does not account for scrollbars

3 participants