Fix overlay issues #276

cmslewis · 2016-11-30T04:23:04Z

PR checklist

Fixes Overlay doesn't stretch 100% height #183
Bugfix
Includes tests
Documentation update

What changes did you make?

Introduce a pt-overlay class to serve as the top-level wrapper descriptor for the overlay component.
As a consumer of Blueprint in the past, I was always much happier when everything had a clear CSS class attached to it. CSS rules downstream are much easier to grok and maintain when applied to meaningfully named elements, rather than opaque HTML tag names.
Introduce a .pt-overlay-inline class and apply it to the .pt-overlay element iff the overlay is being rendered inline.
This allows us to set .pt-overlay-backdrop and/or overlay content to position: absolute when the overlay is rendered inline, so that the overlay intuitively positions itself relative to its parent in that case.
Introduce a .pt-overlay-content class that we apply under-the-hood to overlay content containers to manage CSS position mode on behalf of the developer.
Otherwise, Blueprint users would have to explicitly set position:fixed for non-inline and position:absolute for inline rendering. That'd be brittle.
Set .pt-overlay-backdrop to position:fixed if the overlay is not rendered inline.
This is the crucial fix for Overlay doesn't stretch 100% height #183.
Disable document scrolling beneath the overlay backdrop when hasBackdrop is true.
Reorganize CSS a bit to ensure that overflow-scrolling still works.
Tweaked unit tests:
- Added tests for the new body scrolling behavior
- Explicitly unmount an errant tooltip that was causing .pt-overlay-open to stay on the body after all the tests ran via gulp karma-unit-core
- Bonus: Noticed that some <Portal> tests aren't properly detaching containers after they run. Fixed.

Abandoned Changes:

Introduce a backgroundScrollingDisabled prop on IBackdropProps that, when true, disables scrolling of content beneath the overlay backdrop. Underneath the hood, this adds a .pt-overlay-open class to the document body that sets overflow: hidden when applied.

Is there anything you'd like reviewers to focus on?

Does this approach make sense?
Could anything be named more clearly?

llorca · 2016-11-30T04:43:37Z

packages/docs/src/styles/_sections.scss

  top: $overlay-example-width / 2;
  left: calc(50% - #{$overlay-example-width / 2});
  z-index: $pt-z-index-overlay + 1;
  width: $overlay-example-width;
+
+  .pt-overlay-inline & {


Is this necessary? Looks like line 105 in _overlay.scss does the same thing

Yeah this is necessary. Line 105 applies this style to .pt-overlay-backdrop; this .doc-overlay-example-transition container is a sibling of .pt-overlay-backdrop, and it needs to be styled separately.

This does bring up a broader annoyance that users will need to explicit;y style overlay content with position: absolute if they want it to render properly inline. Maybe we should add a class (e.g. pt-overlay-content) on all siblings of .pt-overlay-backdrop that handles this position styling automatically?

Indeed, users shouldn't have to explicitly style their overlay content, we should handle that automatically for them

Done. 👍 See the updated PR description. I've introduced a .pt-overlay-content class that we add to the overlay content container(s) automatically, to govern fixed/absolute position mode on behalf of Blueprint users.

llorca · 2016-11-30T04:44:42Z

packages/core/src/components/overlay/overlay.tsx

@@ -84,6 +84,11 @@ export interface IBackdropProps {
    backdropProps?: React.HTMLProps<HTMLDivElement>;

    /**
+     * Whether underlying content should be scrollable while the backdrop is visible.
+     */
+    backgroundScrollingDisabled?: boolean;


Why is this configurable? Have you seen (or can you imagine) any use case where false would make sense?

Not off the top of my head, no. @giladgray said something in #183 about making this class prop-configurable. Not quite sure in what that meant - Gilad can you clarify? Happy to get rid of this if we want.

For Overlay, I think it should be configurable because it is a low level component. I can imagine a sidebar created with Overlay which allows scrolling on the main content. It should be set to false for Dialogs, though.

i don't see a need for this prop. an overlay with a backdrop should always prevent scrolling. i can't see a case where you'd want a backdrop and the ability to scroll the document underneath.

Hmm, I feel like we already have a prop for that: hasBackdrop. If it's set to true, you have a backdrop behind and you can't scroll. If it's false, you don't have a background behind and you can scroll.

Enabling this extra backgroundScrollingDisabled prop means that we think there's a use case for having a backdrop behind and being able to scroll what's beneath it. I'm skeptical about that but open to it.

What about canOutsideClickClose found in Overlay, and isModal found in Popover? I feel like there's a lot of repetition going on... Maybe isModal is what we want instead of backgroundScrollingDisabled.

Killing this prop. 👍

Done. FWIW we can easily add this back later if we observe a need for it.

EDIT: Relying on hasBackdrop instead. If true, disable document scrolling beneath the backdrop; if false, don't.

cool, you're right, this makes sense @giladgray @llorca

blueprint-bot · 2016-11-30T08:37:58Z

Unmount lingering portal elements after their unit tests complete

Preview: docs | table Coverage: core | datetime

blueprint-bot · 2016-11-30T08:51:22Z

Delete superfluous call to portal.unmount()

Preview: docs | table Coverage: core | datetime

adidahiya · 2016-11-30T15:24:02Z

packages/core/src/components/overlay/_overlay.scss

@@ -72,9 +72,14 @@ Styleguide components.overlay.scroll

 $overlay-background-color: rgba($black, 0.7);

+// restricts scrolling of underlying content while the overlay is open


this feels a little invasive. is it absolutely necessary?

nvm, ignore me, I caught up on the linked thread

Done (no changes made)

adidahiya · 2016-11-30T15:25:25Z

packages/core/src/components/overlay/overlay.tsx

@@ -84,6 +84,11 @@ export interface IBackdropProps {
    backdropProps?: React.HTMLProps<HTMLDivElement>;

    /**
+     * Whether underlying content should be scrollable while the backdrop is visible.
+     */
+    backgroundScrollingDisabled?: boolean;


For Overlay, I think it should be configurable because it is a low level component. I can imagine a sidebar created with Overlay which allows scrolling on the main content. It should be set to false for Dialogs, though.

giladgray · 2016-11-30T18:24:25Z

packages/core/src/components/overlay/_overlay.scss

+  // when rendered inline, we want the overlay to position itself relative to
+  // its parent container, not relative to the whole window.
+  .pt-overlay-inline & {
+    @include position(absolute, 0);


position: absolute should be enough.
this mixin generates five lines, four of which are the same as line 92.

giladgray · 2016-11-30T18:25:11Z

packages/core/src/components/overlay/overlay.tsx

@@ -172,8 +178,14 @@ export class Overlay extends React.Component<IOverlayProps, IOverlayState> {
            </CSSTransitionGroup>
        );

+        const mergedClassName = classNames({
+            [Classes.OVERLAY]: true,


classNames(Classes.OVERLAY, { ... }, className)

no need to include it in the object if it's always true.

Yeah realized that after opening the PR but forgot to change it. Good catch. Done. 👍

giladgray · 2016-11-30T18:26:58Z

packages/core/test/overlay/overlayTests.tsx

@@ -19,6 +19,7 @@ describe("<Overlay>", () => {
    let wrapper: ReactWrapper<IOverlayProps, any>;

    afterEach(() => {
+        document.body.classList.remove(Classes.OVERLAY_OPEN);


why in the test? i'd expect the component to handle removing the class.
probably means you need to also be removing it in componentWillUnmount()

Ah, yes, I noticed that when I was fixing other tests but didn't come back to omit it here. Done. 👍

llorca · 2016-11-30T20:12:57Z

Besides the few comments, seems to work like wonder! As I understand, this solves #183 entirely and makes #187 irrelevant, correct? If that's the case, it's pretty great that we're able to keep body as the scroll container.

cmslewis · 2016-11-30T23:06:10Z

@llorca yep, this completely solves #183 and also supersedes #187...to some extent. But that PR also fixes #231, #240, and #273 IIRC—this PR does NOT fix those issues, so there's still some work to do after we merge this.

cmslewis · 2016-11-30T23:09:43Z

@giladgray @adidahiya @llorca ready for re-review. See the updated PR description for an overview of the new scope of changes.

cmslewis · 2016-11-30T23:11:27Z

packages/core/src/components/overlay/overlay.tsx

+        // so that our class will override the position style of other classes that might
+        // have set unnecessary position rules (note that CSS specificity plays a role here;
+        // our class will only override an existing position rule if it has the same or
+        // greater selector specificity).


If we proceed with this approach, might be worth mentioning in the docs that there's no need to explicitly set position on your overlay content container, as we manage that ourselves.

yes definitely worth mentioning

cmslewis · 2016-11-30T23:12:05Z

packages/docs/src/styles/_sections.scss

@@ -263,7 +263,6 @@
 .docs-overlay-example-transition {
  $overlay-example-width: $pt-grid-size * 40;

-  position: absolute;


No longer necessary now that we manage the position mode internally (via .pt-overlay-content).

cmslewis · 2016-11-30T23:13:09Z

packages/core/src/components/overlay/_overlay.scss

-
-  .pt-overlay-backdrop {
-    // fixed position so it won't scroll under the overlay
-    position: fixed;


Deleted this rule (and therefore this .pt-overlay-backdrop block), because the backdrop is set to position: fixed by default now.

cmslewis · 2016-11-30T23:13:38Z

packages/core/src/components/overlay/_overlay.scss

@@ -72,19 +72,14 @@ Styleguide components.overlay.scroll

 $overlay-background-color: rgba($black, 0.7);

-// scroll container for non-inline overlay
-.pt-overlay-scroll-container.pt-overlay-open {


Moved this down to the bottom of the file because it's a special case.

blueprint-bot · 2016-12-01T01:18:50Z

Merge branch 'master' into cl/overlay-fixed

Preview: docs
Coverage: core | datetime

giladgray

mostly comments on comments 😄

giladgray · 2016-12-01T21:37:47Z

packages/core/src/components/overlay/_overlay.scss

 }

+// fixed position so it won't scroll underneath the overlay content


not true... fixed position so it always covers the entire viewport (irrespective of page scroll position)

This was a relic from line 81 in the old code. Will remove. 👍

giladgray · 2016-12-01T21:38:47Z

packages/core/src/components/overlay/_overlay.scss

+
+// scroll container for non-inline overlay. it needs an explicit z-index to
+// ensure that the backdrop renders on top of any navbars.
+.pt-overlay-scroll-container.pt-overlay-open {


i wonder if we still need this class? but that's another issue, about scrolling dialogs etc

Possibly not, can look into that more after this PR if that sounds good.

Done (no change for now)

yes not in this PR

giladgray · 2016-12-01T21:40:18Z

packages/core/src/components/overlay/overlay.tsx

@@ -159,6 +159,19 @@ export class Overlay extends React.Component<IOverlayProps, IOverlayState> {

        const { children, className, inline, isOpen, transitionDuration, transitionName } = this.props;

+        // add a special class to each child that will automatically set the appropriate
+        // CSS position mode under the hood. we add this class *after* the existing classes
+        // so that our class will override the position style of other classes that might


this is not how CSS works, which you call out later... it's entirely about specificity. if the user sets position in a selector with three classes it'll win out over ours every time.

i think you could safely remove all but the first sentence from this comment.

Yeah this comment felt a little ham-handed. Will edit.

giladgray · 2016-12-01T21:40:29Z

packages/core/src/components/overlay/overlay.tsx

+        // so that our class will override the position style of other classes that might
+        // have set unnecessary position rules (note that CSS specificity plays a role here;
+        // our class will only override an existing position rule if it has the same or
+        // greater selector specificity).


yes definitely worth mentioning

giladgray · 2016-12-01T21:41:00Z

packages/core/src/components/overlay/overlay.tsx

+        // our class will only override an existing position rule if it has the same or
+        // greater selector specificity).
+        const decoratedChildren = React.Children.map(children, (child: React.ReactChild) => {
+            const childEl = child as React.ReactElement<any>;


can you do this cast in the function signature? (child: React.ReactElement<any>) => {

Sure thing.

giladgray · 2016-12-01T21:42:48Z

packages/core/test/overlay/overlayTests.tsx

+            assert.isFalse(isBodyScrollingDisabled());
+        });
+
+        function mountOverlay(inline?: boolean, hasBackdrop?: boolean) {


no need for ? on either param, they're always specified

Oh does TS not freak when I pass undefined if a boolean is expected?

mm probably would. nvm then... inline?: boolean === inline: boolean | undefined

giladgray · 2016-12-01T21:43:59Z

packages/core/src/components/overlay/_overlay.scss

+  .pt-overlay-content {
+    position: absolute;
+  }
+}


this file looks great now! excellent refactor and comments.

though it should end with a newline...

Hmm, I thought my editor handled that. Will add.

yeah that's why i asked about EditorConfig 😄

giladgray · 2016-12-01T21:46:28Z

packages/core/src/components/overlay/_overlay.scss

@@ -72,19 +72,14 @@ Styleguide components.overlay.scroll



the words "absolute positioning" are used a few times in these docs, please update accordingly.

see line 59 above

cmslewis · 2016-12-01T22:49:33Z

@giladgray is this too verbose, or does this look okay?

blueprint-bot · 2016-12-01T22:52:19Z

Fix stuff per PR comments

Preview: docs | table
Coverage: core | datetime

giladgray · 2016-12-01T22:52:25Z

yes too verbose. say it chooses fixed or absolute based on inline.
make it pt-intent-primary

cmslewis · 2016-12-01T22:55:59Z

Eh?

giladgray · 2016-12-01T22:59:30Z

remove the first sentence? and clarify that it's absolute when inline to respect document flow and fixed otherwise to cover the entire viewport.

blueprint-bot · 2016-12-01T23:00:37Z

Reword the callout text

Preview: docs | table
Coverage: core | datetime

cmslewis · 2016-12-02T02:22:34Z

@giladgray v3

blueprint-bot · 2016-12-02T02:27:34Z

Reword the callout text v3

Preview: docs | table
Coverage: core | datetime

adidahiya · 2016-12-02T02:37:15Z

Unrelated: it would be nice to make the inline rendering examples look nicer in a separate PR.

blueprint-bot · 2016-12-02T18:41:09Z

spaces after colons

Preview: docs | table
Coverage: core | datetime

cmslewis added 12 commits November 24, 2016 21:52

Fix CSS for button inside .pt-control-group > .pt-input-group

c65d41a

Tweak the example to include a pt-input-action parent

03b1732

Fix rule order

c9ff022

Offer a better fix now that I've perused how things work more closely

9f8110d

Delete old comment

54d6120

Merge branch 'master' into cl/control-input-group-button

440779d

Use position:fixed on overlay backdrop

794cb75

Add pt-overlay and pt-overlay-inline, styling each appropriately

65327f4

Add this.props.backgroundScrollingDisabled

754984e

Fix position property of docs overlay example

1da21c3

Unnest and simplify styles

a87bf1e

Oops, undo changes to unrelated _control-group.scss file

945cfc4

llorca reviewed Nov 30, 2016

View reviewed changes

llorca added Package: core Type: bug Type: enhancement labels Nov 30, 2016

cmslewis added 3 commits November 29, 2016 23:45

Fix overlay tests

0b30156

Unmount svg tooltip in test to make sure body is scrollable afterward

0d8a10a

Unmount lingering portal elements after their unit tests complete

015c136

Delete superfluous call to portal.unmount()

2480cc7

adidahiya reviewed Nov 30, 2016

View reviewed changes

giladgray suggested changes Nov 30, 2016

View reviewed changes

cmslewis added 4 commits November 30, 2016 12:57

Delete the backgroundScrollingDisabled prop

87a1ab2

Fix little things per PR comments

a3b6e5b

Use hasBackdrop to dictate whether we disable body scrolling

d33e159

Fix tests to check for inline/hasBackdrop pairings

35ab4b4

cmslewis commented Nov 30, 2016

View reviewed changes

Merge branch 'master' into cl/overlay-fixed

8877770

cmslewis mentioned this pull request Dec 1, 2016

Opening any popover scrolls to bottom of page in Firefox #231

Closed

llorca removed the Type: enhancement label Dec 1, 2016

cmslewis mentioned this pull request Dec 1, 2016

[POC] Fix popover issues on docs page in Firefox and Chrome #287

Closed

giladgray suggested changes Dec 1, 2016

View reviewed changes

giladgray reviewed Dec 1, 2016

View reviewed changes

Fix stuff per PR comments

130d8c4

Reword the callout text

1b6965d

Reword the callout text v3

4e03a71

spaces after colons

1bccea4

giladgray approved these changes Dec 2, 2016

View reviewed changes

cmslewis merged commit 7a827bc into master Dec 2, 2016

cmslewis deleted the cl/overlay-fixed branch December 2, 2016 19:01

adidahiya mentioned this pull request Dec 4, 2016

[POC] use .docs-root as scroll container #187

Closed

greglo pushed a commit to greglo/blueprint that referenced this pull request Dec 12, 2016

Fix overlay issues (palantir#276)

6807a90

leebyp mentioned this pull request Jan 18, 2017

Fix toast autofocus #506

Merged

1 task

		@@ -72,9 +72,14 @@ Styleguide components.overlay.scroll

		$overlay-background-color: rgba($black, 0.7);

		// restricts scrolling of underlying content while the overlay is open

		}

		// fixed position so it won't scroll underneath the overlay content

Fix overlay issues #276

Fix overlay issues #276

Conversation

cmslewis commented Nov 30, 2016 • edited

PR checklist

What changes did you make?

Is there anything you'd like reviewers to focus on?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Nov 30, 2016

Unmount lingering portal elements after their unit tests complete

blueprint-bot commented Nov 30, 2016

Delete superfluous call to portal.unmount()

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorca commented Nov 30, 2016 • edited

cmslewis commented Nov 30, 2016 • edited

cmslewis commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Dec 1, 2016

Merge branch 'master' into cl/overlay-fixed

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray Dec 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis commented Dec 1, 2016

blueprint-bot commented Dec 1, 2016

Fix stuff per PR comments

giladgray commented Dec 1, 2016

cmslewis commented Dec 1, 2016

giladgray commented Dec 1, 2016

blueprint-bot commented Dec 1, 2016

cmslewis commented Nov 30, 2016 •

edited

cmslewis Nov 30, 2016 •

edited

cmslewis Nov 30, 2016 •

edited

cmslewis Nov 30, 2016 •

edited

llorca commented Nov 30, 2016 •

edited

cmslewis commented Nov 30, 2016 •

edited

giladgray Dec 1, 2016 •

edited