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

Improve galleries for IE11. #9622

Merged
merged 1 commit into from Sep 10, 2018

Conversation

Projects
None yet
3 participants
@jasmussen
Contributor

jasmussen commented Sep 5, 2018

This PR is a followup to feedback in #7465 (comment).

Notably there is an issue with image cropping in IE11 galleries. Simply, it doesn't really work because IE11 doesn't support object-fit. This means in some situations — for example a 2 column gallery with landscape, portrait, portrait images — the landscape image is skewed.

This PR takes a somewhat radical consequence and simply disables cropping altoghether on IE11. But it works fine in Edge.

This PR does a few other improvements too — it changes the IE11 hack to be a more solid one, using @supports(position:sticky) to augment the CSS for capable browsers, rather than relying on a contrast hack that fails if a user actually uses high contrast mode. In addition, it moves this hack from the editor style to the stylesheet file, so benefits affect both editor and theme.

Chrome:

screen shot 2018-09-05 at 10 24 40

IE11:

screen shot 2018-09-05 at 10 24 27

Edge:

screen shot 2018-09-05 at 10 29 06

Improve galleries for IE11.
This PR is a followup to feedback in #7465 (comment).

Notably there is an issue with image cropping in IE11 galleries. Simply, it doesn't really work because IE11 doesn't support object-fit. This means in some situations — for example a 2 column gallery with landscape, portrait, portrait images — the landscape image is skewed.

This PR takes a somewhat radical consequence and simply disables cropping altoghether on IE11. But it works fine in Edge.

This PR does a few other improvements too — it changes the IE11 hack to be a more solid one, using @supports(position:sticky) to augment the CSS for capable browsers, rather than relying on a contrast hack that fails if a user actually uses high contrast mode. In addition, it moves this hack from the editor style to the stylesheet file, so benefits affect both editor and theme.

@jasmussen jasmussen self-assigned this Sep 5, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 5, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 5, 2018

Contributor

As a followup to this, here's an idea for how we can hide a feature if IE11 doesn't support it. Simply, add a classname to the component you want to hide for IE, such as is-ie11-disabled, and then combine it with this:

// If a feature is disabled for IE11, hide it by default then unhide it for capable browsers.
.is-ie11-disabled {
	display: none;

	@supports (position: sticky) {
		display: inherit;
	}
}

With the above in place, Chrome and Edge would show this:

screen shot 2018-09-05 at 10 41 09

But IE11 would show this:

screen shot 2018-09-05 at 10 42 41

What do you think? I feel like you might have some ideas here, @tofumatt, woudl appreciate your eyes.

Contributor

jasmussen commented Sep 5, 2018

As a followup to this, here's an idea for how we can hide a feature if IE11 doesn't support it. Simply, add a classname to the component you want to hide for IE, such as is-ie11-disabled, and then combine it with this:

// If a feature is disabled for IE11, hide it by default then unhide it for capable browsers.
.is-ie11-disabled {
	display: none;

	@supports (position: sticky) {
		display: inherit;
	}
}

With the above in place, Chrome and Edge would show this:

screen shot 2018-09-05 at 10 41 09

But IE11 would show this:

screen shot 2018-09-05 at 10 42 41

What do you think? I feel like you might have some ideas here, @tofumatt, woudl appreciate your eyes.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 5, 2018

Member

That's basically what I've done elsewhere already (see ef25165).

Really what we ought to have though is a mixin that we use for IE11 fixes instead of using @supports (position: sticky) {}. Sure it works but it requires comments or might seem like we're targeting that CSS support instead of just targeting IE.

I don't think we need the .is-ie11-disabled bit because we can just set the default style anyway and set an IE11 one after inside the @supports (position: sticky) {}. But we should make it a mixin, like:

.my-class {
  width: 100%;

  @include not-ie11() {
    width: auto;
  }
}
Member

tofumatt commented Sep 5, 2018

That's basically what I've done elsewhere already (see ef25165).

Really what we ought to have though is a mixin that we use for IE11 fixes instead of using @supports (position: sticky) {}. Sure it works but it requires comments or might seem like we're targeting that CSS support instead of just targeting IE.

I don't think we need the .is-ie11-disabled bit because we can just set the default style anyway and set an IE11 one after inside the @supports (position: sticky) {}. But we should make it a mixin, like:

.my-class {
  width: 100%;

  @include not-ie11() {
    width: auto;
  }
}
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 5, 2018

Member

In my opinion, we should want it to be painful to make IE-specific exceptions, because otherwise it serves as a convenient crutch when it's not always reasonable to make said exception.

Member

aduth commented Sep 5, 2018

In my opinion, we should want it to be painful to make IE-specific exceptions, because otherwise it serves as a convenient crutch when it's not always reasonable to make said exception.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 5, 2018

Member

It's already easy to create the exceptions (@supports (position: sticky) {} is quite painless) and flagging them specifically as IE11 hacks means when we eventually drop IE11 support they'll be easy to find+delete.

Plus: being able to create exceptions for the only "outdated" browser we still support is probably easier than spending our time finding a less immediately obvious cross-browser solution we'll eventually be able to ditch anyway.

Member

tofumatt commented Sep 5, 2018

It's already easy to create the exceptions (@supports (position: sticky) {} is quite painless) and flagging them specifically as IE11 hacks means when we eventually drop IE11 support they'll be easy to find+delete.

Plus: being able to create exceptions for the only "outdated" browser we still support is probably easier than spending our time finding a less immediately obvious cross-browser solution we'll eventually be able to ditch anyway.

@tofumatt

Makes sense to me; tested locally in IE11 and it works.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 6, 2018

Contributor

In my opinion, we should want it to be painful to make IE-specific exceptions, because otherwise it serves as a convenient crutch when it's not always reasonable to make said exception.

Agree with this.

The huge upside to using @supports here is that that one doesn't work at all in IE11, let alone position sticky. Which means IE11 ignores the entire rule, whereas moder browsers do it in a clean and standardized way.

However the downside is that this essentially means we design for IE11 by default, and override for modern browsers.

If only we could use @supports not (https://developer.mozilla.org/en-US/docs/Web/CSS/@supports):

@supports not (position: stiky) {
// IE11 specific rules
}

But alas we can't do that. In absence of that I agree that we shouldn't use mixins.

Contributor

jasmussen commented Sep 6, 2018

In my opinion, we should want it to be painful to make IE-specific exceptions, because otherwise it serves as a convenient crutch when it's not always reasonable to make said exception.

Agree with this.

The huge upside to using @supports here is that that one doesn't work at all in IE11, let alone position sticky. Which means IE11 ignores the entire rule, whereas moder browsers do it in a clean and standardized way.

However the downside is that this essentially means we design for IE11 by default, and override for modern browsers.

If only we could use @supports not (https://developer.mozilla.org/en-US/docs/Web/CSS/@supports):

@supports not (position: stiky) {
// IE11 specific rules
}

But alas we can't do that. In absence of that I agree that we shouldn't use mixins.

@jasmussen jasmussen modified the milestones: 3.8, 3.9 Sep 6, 2018

@jasmussen jasmussen merged commit f2fa319 into master Sep 10, 2018

2 checks passed

codecov/project 50.96% remains the same compared to 4fec0d2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/fix-ie11-galleries branch Sep 10, 2018

kjellr added a commit that referenced this pull request Sep 10, 2018

Adjust gallery caption flex alignment.
Gallery captions are meant to appear at the bottom of gallery items, but a recent change in #9622 changed that and introduced a visual bug. This sets things back to the way they were before.

Fixes #9752

jasmussen added a commit that referenced this pull request Sep 13, 2018

Adjust gallery caption flex alignment (#9762)
* Adjust gallery caption flex alignment.

Gallery captions are meant to appear at the bottom of gallery items, but a recent change in #9622 changed that and introduced a visual bug. This sets things back to the way they were before.

Fixes #9752

* Fix issue with non-cropped galleries.

* Fix issues with captions.

Props for the code to @webmandesign.

* Fix another issue with captions.

* Set a bottom position value gallery captions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment