Fix z-index stack for .pt-button-group and .pt-control-group elements #592

cmslewis · 2017-02-01T09:11:26Z

No official issue filed, but originally discovered problems here while working on #249 and #189

Checklist

Include tests (it would be awesome to add unit tests to verify the z-index stacking order, but we'd need chai-enzyme to access computed styles for mounted React components - should we add it as a dependency?).
Update documentation (@llorca - can you edit examples to your liking?)

Changes proposed in this pull request:

Clearly specify the z-index stack of all possible .pt-button-group elements to ensure sharp edges in all cases and states:

  (lowest z-index)

  default buttons
    :disabled, .pt-disabled
    normal
    :focus
    :hover
    :active, .pt-active

  intent buttons
    :disabled, .pt-disabled
    normal
    :focus
    :hover
    :active, .pt-active

  (highest z-index)

Clearly specify the z-index stack of all possible .pt-control-group elements to ensure sharp edges in all cases and states:

  (lowest z-index)

  input fields
     :disabled, .pt-disabled
     normal

  regular buttons / pt-select > select
    :disabled, .pt-disabled
    normal
    :focus
    :hover
    :active, .pt-active

  pt-input intent

  intent buttons (selects can't be intented yet, but if they could be, they'd be here too)
    :disabled, .pt-disabled
    normal
    :focus
    :hover
    :active, .pt-active

  input fields (NOT readonly, :disabled, .pt-disabled)
     :focus

  intent input fields (NOT readonly, :disabled, .pt-disabled)
     :focus

  input-group children

  .pt-select::after (i.e. the caret)

  (highest z-index)

Reviewers should focus on:

Definitely look at the diff in "Split" mode, not "Unified" mode.

How does the new z-index stack feel?
Is it implemented correctly?
Are we good with the z-index variable names? Thought about combining them, but the two group types had different enough stacking semantics that it was clearer to spell everything out separately.
Are there any weird edge cases with certain combinations of grouped elements?
Are there any regressions?

Screenshot

blueprint-bot · 2017-02-01T09:16:22Z

De-dup and consolidate CSS rules

Preview: docs | table
Coverage: core | datetime

cmslewis

@llorca Here are some explanatory comments to guide you through the CSS rule-shuffles.

cmslewis · 2017-02-01T09:34:02Z

packages/core/src/components/forms/_control-group.scss

  .pt-button,
  .pt-input,
-  .pt-select select {
+  .pt-select {


Moved from old line 107.

cmslewis · 2017-02-01T09:35:06Z

packages/core/src/components/forms/_control-group.scss


-  &:not(.pt-vertical) > * {


Moved this block to the bottom of the file to keep z-index-related stuff in one clump.

cmslewis · 2017-02-01T09:35:57Z

packages/core/src/components/forms/_control-group.scss

-    margin-right: -$button-border-width;
-  }
+    // define disabled styles last so that they override all other
+    // equally-specific styles.


Will delete one of these explanations (it's also explained in the top-level comment above).

cmslewis · 2017-02-01T09:36:51Z

packages/core/src/components/forms/_control-group.scss

@@ -185,11 +340,4 @@ Styleguide components.forms.control-group
      border-radius: 0 0 $pt-border-radius $pt-border-radius;
    }
  }
-
-  .pt-dark & {


Moved to line 212 to keep z-index-related rules in one clump.

cmslewis · 2017-02-01T09:37:52Z

packages/core/src/components/forms/_control-group.scss


-  > :first-child {


Moved to line 289.

cmslewis · 2017-02-01T09:41:11Z

packages/core/src/components/forms/_control-group.scss

  .pt-button,
-  .pt-select {
-    position: relative;


Moved to the top of the file.

cmslewis · 2017-02-01T09:42:43Z

packages/core/src/components/forms/_control-group.scss

    }

-    &:not([readonly], :disabled, .pt-disabled):focus {
-      z-index: 6;
-      border-radius: $pt-border-radius;


Moved to line 197.

cmslewis · 2017-02-01T09:43:28Z

packages/core/src/components/forms/_control-group.scss


-  // establish new stacking context so focus state covers neighbors
-  .pt-button:focus,
-  select:focus {


Now handled with higher z-indexes in the same stacking context.

cmslewis · 2017-02-01T09:44:31Z

packages/core/src/components/forms/_control-group.scss

  .pt-select::after {
-    z-index: 7;
+    z-index: $control-group-z-index-select-caret;


Moved the carets to a z-index all their own that explicitly sits them on top of everything, just to be absolutely safe.

cmslewis · 2017-02-01T09:45:39Z

packages/docs/src/styles/_sections.scss

@@ -54,7 +61,7 @@
  display: flex;
  flex-direction: column;
  justify-content: space-between;
-  height: $pt-grid-size * 13;
+  height: $pt-grid-size * 50;


Feels weird that we have to explicitly set a height. Why not just have docs containers expand and contract automatically with the height of the child contents?

they should do that... casual flex box. why do you have to set a height?

It's already the case. Setting a height here will evenly space out the children so there's spacing in between them. Without this line:

So what we can do instead is remove this line and add an additional rule in the same block:

.pt-control-group-example { > .pt-control.group:not(:last-child) { margin-bottom: $pt-grid-size * 2; } }

Derp, don't know why I didn't think to try that. Thanks, @llorca.

adidahiya · 2017-02-01T15:36:33Z

Not opposed to adding chai-enzyme as a devDependency. Sounds like it could be useful.

llorca · 2017-02-01T20:15:47Z

packages/core/src/components/forms/_control-group.scss

+
+  // have consecutive elements share a border
+  &:not(.pt-vertical) > * {
+    margin-right: -$button-border-width;


That's actually only correct for light theme.

The visuals look different in dark theme, depending on whether you use a button group or single buttons for consecutive buttons (top is bad, bottom is good):

So we can keep that line for the light theme, but we need to take care of dark theme. Proposal (to be refactored in the appropriate place):

.pt-dark &:not(.pt-vertical) { > * { margin-right: 0; } > .pt-button + .pt-button { margin-left: $button-border-width; } }

We also need to handle this situation in the .pt-vertical case.

Fixed and incorporated as follows:

// have consecutive elements share a border &:not(.pt-vertical) { > * { margin-right: -$button-border-width; } .pt-dark & { > * { margin-right: 0; } // conseuctive buttons within a button group look okay out of the box, but // we need need to make a small correction for non-grouped buttons > .pt-button + .pt-button { margin-left: $button-border-width; } } }

Great! For clarity, maybe also mention that we're basically replicating what's happening in dark button groups?

llorca · 2017-02-01T20:16:07Z

packages/core/src/components/forms/_control-group.scss

+//
+//   (highest z-index)
+
+$control-group-z-index-input-disabled: 1;


2 Qs about maintainability:

if our visual design changes and we ever need to update the stacking of elements in control groups, we can simply modify the values of these variables, right? No need to mess with the selectors and CSS specificities below?

these variables wouldn't get exported, and I'm pretty sure they wouldn't be used anywhere else. Maybe we should use a map object instead of single variables for every value?

Yep, just change the z-indexes!

Moving everything into a list so we can just use the index() helper.

llorca · 2017-02-01T20:16:36Z

packages/docs/src/styles/_sections.scss

@@ -38,6 +38,13 @@

 // Section-Specific Styles

+#{section-id("components.button-group")} {
+  .docs-button-group-example-set .pt-button-group {
+    margin-top: 20px;


$pt-grid-size * 2

llorca · 2017-02-01T20:19:27Z

packages/docs/src/styles/_sections.scss

@@ -54,7 +61,7 @@
  display: flex;
  flex-direction: column;
  justify-content: space-between;
-  height: $pt-grid-size * 13;
+  height: $pt-grid-size * 50;


It's already the case. Setting a height here will evenly space out the children so there's spacing in between them. Without this line:

So what we can do instead is remove this line and add an additional rule in the same block:

.pt-control-group-example { > .pt-control.group:not(:last-child) { margin-bottom: $pt-grid-size * 2; } }

giladgray · 2017-02-01T22:21:47Z

packages/core/src/components/button/_button-group.scss

+  "intent-button-default",
+  "intent-button-focus",
+  "intent-button-hover",
+  "intent-button-active";


i see two duplicated lists here. what happened to a single intent-boost variable?

then you go index(list, "disabled") + $intent-boost (pseudocode).

then you could create a mixin that wraps up lines 151-172 and reuse it in the intent block by passing a single fixed "booster" value. @include z-index-state-layers($boost)

This was meant to be just an incremental change. Will play with it.

@giladgray chatted and worked through a potential refactor with Antoine. The intent-boost approach works well for button groups, but it's more tricky than helpful in control groups, where differently ordered .pt-input layers get in the way.

One approach we're considering now is to use a single list of named layers that is shared between control groups and button groups, since the relative ordering of elements doesn't actually change between the two contexts.

👍 TLDR: 1 list with everything ($control-group-stack or something), and button groups use a subset of that list via index()

llorca · 2017-02-02T00:22:53Z

packages/core/src/components/button/_button-group.scss

+//
+//   (highest z-index)
+
+$-button-group-z-index-stack:


Remove extra - before button. Also, can we just go for $button-group-stack or something simpler? Is z-index really necessary in the name?

@llorca Fixed. I ended up creating a single $control-group-stack variable, putting it in forms/_common.scss, and importing it into _control-group.scss and _button-group.scss. LMK how that looks.

…bles

blueprint-bot · 2017-02-02T07:30:30Z

Use a single -group-stack list, pull it out into forms/_common

Preview: docs | landing | table
Coverage: core | datetime

blueprint-bot · 2017-02-02T07:32:22Z

Delete unused -state-stack variable

Preview: docs | landing | table
Coverage: core | datetime

llorca · 2017-02-02T07:45:36Z

packages/core/src/components/forms/_control-group.scss

+        margin-right: 0;
+      }
+
+      // conseuctive buttons within a button group look okay out of the box, but


consecutive

dark theme

llorca · 2017-02-02T07:46:56Z

I'm a fan of this approach! Much cleaner, less code, 1 reusable list. Big comment explanation no longer needed, code speaks for itself 💯

giladgray · 2017-02-02T17:32:18Z

packages/core/src/components/forms/_control-group.scss

-      <input type="text" class="pt-input" value="from:ggray to:allorca" />
+      <span class="pt-icon pt-icon-people"></span>
+      <input type="text" class="pt-input pt-intent-success" placeholder="I am a text input..." style="padding-right:94px" />
+      <div class="pt-input-action">


.pt-input-action shouldn't be needed when wrapping a .pt-button. InputGroup always uses it because it can't know about the contents but should be safe to remove in these static HTML examples.

Fixed. 👍

Just kiddingggg, I need to revert. Forgot that we need these .pt-input-action wrappers in all cases to address issues like #236 and #237. If you delete the .pt-input-action wrapper, you get a weird flicker on focus.

giladgray · 2017-02-02T17:32:50Z

packages/core/src/components/forms/_control-group.scss

  </div>
  <div class="pt-control-group">
    <div class="pt-input-group">
      <span class="pt-icon pt-icon-people"></span>
-      <input type="text" class="pt-input" placeholder="Find collaborators..." style="padding-right:94px" />
+      <input type="text" class="pt-input pt-intent-success" placeholder="I am a text input..." style="padding-right:94px" />


why the placeholder change? kinda breaks the example metaphor

In the development phase, it was tricky to tell at a glance what was a button and what was an input when either was disabled:

Explicit placeholder text helped with that. Totally not wedded to these examples, by the way. They're here just to allow easy testing during the development phase, but I'd prefer CSS unit tests as a longer-term solution.

giladgray · 2017-02-02T17:33:41Z

packages/core/src/components/forms/_control-group.scss

+      <button class="pt-button pt-intent-warning">Button</button>
+      <button class="pt-button pt-intent-success">Button</button>
+      <button class="pt-button">Button</button>
+    </div>


@cmslewis are we planning to ship all these example changes? on one hand, they're useful for testing, but on the other they're somewhat unsightly and are usages that we may not want to promote so visibly.

On that note, the 3 examples we currently serve in the docs are more interesting indeed. Once we're confident everything looks good, let's bring back those examples, and maybe we can think of additional 1 or 2 to showcase as well

Yep, totally down to revert to our prior examples before merging.

giladgray · 2017-02-02T17:35:49Z

packages/core/src/components/forms/_common.scss

+  "intent-input-focus",
+  "input-group-children",
+
+  "select-caret";


@cmslewis planning to remove this in a future increment?

giladgray · 2017-02-02T17:36:12Z

⭐️ the single stack variable approach works nicely!

cmslewis · 2017-02-02T21:40:31Z

@giladgray and @llorca. LMK if this looks good to you now; if so, I'll revert to the previous examples and merge.

blueprint-bot · 2017-02-02T21:43:49Z

Address PR comments (comment fixes, delete rule of select caret)

Preview: docs | landing | table
Coverage: core | datetime

giladgray · 2017-02-02T22:57:44Z

packages/core/src/components/forms/_common.scss

+// for best visual results, button group and control group elements should be
+// stacked in the following order to ensure sharp edges in all cases and states:
+
+$control-group-stack:


i think it'd be helpful to wrap this in parens, which is the convention for an explicit sass list.

$control-group-stack: ( ... );

Sweet. Doing so now.

cmslewis · 2017-02-03T08:53:06Z

@giladgray @llorca Oh no! When I reverted the examples to the originals, I found a caret bug on focus:

Having position: relative on .pt-select doesn't help; looks like we need that select-caret layer in $control-group-stack after all. Happy to revisit in a future PR though.

blueprint-bot · 2017-02-03T09:01:58Z

Bugfix: Reintroduce select-caret z-index to fix focus-state bug

Preview: docs | landing | table
Coverage: core | datetime

giladgray · 2017-02-03T19:47:53Z

packages/core/src/components/forms/_control-group.scss

+  // very particular order for best visual results. in each level of selector
+  // specificity, we define disabled styles last so that they override all other
+  // equally-specific styles (e.g. we don't want mouse interactions or focus
+  // changes to change the z-index of a disabled element).


does it make sense to use !important for disabled styles like @leebyp is doing in #565?

cmslewis added 5 commits January 27, 2017 17:12

Create z-index variables and tweak stacking order in button groups

d85a08e

Add exhaustive examples for button groups

205b37e

Tweak z-indexes for control groups

fdd021b

De-dup and consolidate CSS rules

9254eeb

De-dup and consolidate CSS rules

04cbcc1

cmslewis commented Feb 1, 2017

View reviewed changes

llorca suggested changes Feb 1, 2017

View reviewed changes

Address PR feedback (including moving to SCSS lists for stack variables)

4397f5c

giladgray reviewed Feb 1, 2017

View reviewed changes

llorca reviewed Feb 2, 2017

View reviewed changes

cmslewis added 4 commits February 1, 2017 16:39

Simplify stacking order in control groups

6bf19a4

Refactor button-group stacking with state-list and intent-boost varia…

a8c5f53

…bles

Use a single -group-stack list, pull it out into forms/_common

ec4f07b

Delete unused -state-stack variable

bb70ea4

llorca reviewed Feb 2, 2017

View reviewed changes

giladgray suggested changes Feb 2, 2017

View reviewed changes

Address PR comments (comment fixes, delete rule of select caret)

6ae73fb

llorca approved these changes Feb 2, 2017

View reviewed changes

giladgray reviewed Feb 2, 2017

View reviewed changes

Add explicit parens around the -group-stack list

8e779f3

Revert example to originals

9570dbb

Bugfix: Reintroduce select-caret z-index to fix focus-state bug

6c40abd

giladgray added the P0 label Feb 3, 2017

giladgray reviewed Feb 3, 2017

View reviewed changes

giladgray approved these changes Feb 3, 2017

View reviewed changes

giladgray merged commit a8d6f52 into master Feb 3, 2017

giladgray deleted the cl/css-group-z-indexes branch February 3, 2017 22:18

giladgray removed the P0 label Feb 3, 2017

giladgray mentioned this pull request Feb 3, 2017

Prepare Release v1.8.0 #616

Merged

cmslewis mentioned this pull request Feb 6, 2017

[Numeric Input] Fix dark-theme style bugs #632

Merged

Fix z-index stack for .pt-button-group and .pt-control-group elements #592

Fix z-index stack for .pt-button-group and .pt-control-group elements #592

Conversation

cmslewis commented Feb 1, 2017 • edited

No official issue filed, but originally discovered problems here while working on #249 and #189

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

blueprint-bot commented Feb 1, 2017

De-dup and consolidate CSS rules

cmslewis 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

adidahiya commented Feb 1, 2017

llorca Feb 1, 2017 • edited

Choose a reason for hiding this comment

cmslewis Feb 1, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Feb 2, 2017 • 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

blueprint-bot commented Feb 2, 2017

Use a single -group-stack list, pull it out into forms/_common

blueprint-bot commented Feb 2, 2017

Delete unused -state-stack variable

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorca commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Feb 2, 2017 • 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

giladgray commented Feb 2, 2017

cmslewis commented Feb 2, 2017

blueprint-bot commented Feb 2, 2017

Address PR comments (comment fixes, delete rule of select caret)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis commented Feb 3, 2017 • edited

blueprint-bot commented Feb 3, 2017

Bugfix: Reintroduce select-caret z-index to fix focus-state bug

Choose a reason for hiding this comment

cmslewis commented Feb 1, 2017 •

edited

llorca Feb 1, 2017 •

edited

cmslewis Feb 1, 2017 •

edited

cmslewis Feb 2, 2017 •

edited

cmslewis Feb 2, 2017 •

edited

cmslewis Feb 2, 2017 •

edited

cmslewis commented Feb 3, 2017 •

edited