Problem/Motivation

We have a problem with the next rule coming from `blazy/css/components/blazy.ratio.css`:

/* Touch me not! Or boom when outside .blazy such as within lightboxes. */
.media {
  position: relative;
}

This will be applied to all `.media` elements and not just the one that are in blazy component or that have `.media--ratio` CSS class. I guess we need a relative position because of ratio rules? If this is a case can we move this to `.media.media--ratio` selector?

Comments

pivica created an issue. See original summary.

pivica’s picture

Status: Active » Needs review
StatusFileSize
new652 bytes

Here is a patch.

pivica’s picture

And here is a patch against rc7, we need this for a composer. Just ignore it.

gausarts’s picture

You are right.

Out of curiosity, any real case that the rule bugs anything?

It is there since 1.x, and nobody complains till now.

I took the risk since it is not over-specified and can be easily overridden as required down the road. Aside from the fact, it is not as bad as absolute or static. Relative positioning will not affect its children unlike static or absolute till its children require absolute positioning.

The same thing has been already done by core contextual links module since D7, too.

Having said that, I am fine to not be too global to avoid side issues.

FYI, it is not only for aspect ratio, but also loading indicator. Perhaps need extra rules?

berdir’s picture

> Out of curiosity, any real case that the rule bugs anything?

Something seems to have changed, we have a slider on top of a background image, something between 1.0.0-rc4 and 2.0.0-rc7 caused it to break.

justclint’s picture

> Out of curiosity, any real case that the rule bugs anything?

We've just set this up and enabled blazy on our text filter.

What Im noticing is that any inline image that is styled with position absolute is breaking when getting wrapped with the wrapper divs.

I think in normal wysiwyg situations users probably aren't positioning inline images with absolute but in our case, we use wysiwygs for ab testing and do quite a bit of custom markup in there so it does have an impact in our case.

gausarts’s picture

Status: Needs review » Needs work

What Im noticing is that any inline image that is styled with position absolute is breaking when getting wrapped with the wrapper divs.

I understand what you meant, and had also mentioned the problem here partially.

There are a few hacks that I knew before I started using this particular hack: padding top vs. bottom, pseudo selectors vs. non. They all try to solve the same issues -- layout reflow, responsiveness and collapsed lazyloaded images, etc. This is noted anywhere in project homepage, docs, codebase. Ignorant people can live happily with these issues, but not themers like me.

One of these hacks was even noted with a copyright by Google inc., that is using pseudo selectors with the opposite padding top while Blazy uses padding bottom with non-pseudo selectors, hence what you referred with inline images.

You can find in any of blazy-related issues, people repeatedly reported these issues. And the solution is there all along named `Aspect ratio`, just not immediately obvious. Some are enforced like BlazyFilter.

The reason I chose the current hack was because I needed to make them manipulated by JS as noted in blazy.load.js related to multi-breakpoint images as otherwise JS has no idea about pseudo selectors, not even the latest jQuery can handle this.

I meant it is not a rolled-by-dice choice, it is a conscious one. Also recommended by bLazy author here. Knowing it is solving problems, and also understanding side issues. And they are mentioned in the docs.

I have my own ideal with this hack when browsers supports on attr() function are not sparse. Both latest Chrome and FF are not even supporting this as of this writing, except for pseudo-elements which Blazy doesn't use with this hack.

What I am saying is that what you thought to be a problem is actually a deliberately attempted solution to a few deeper and bigger problems.

However as it is named hack, I do understand side issues. A few are noted within the codebase if you dig deeper enough:https://git.drupalcode.org/project/blazy/blob/8.x-2.0/css/blazy.css#L50
https://git.drupalcode.org/project/blazy/blob/8.x-2.x/docs/TROUBLESHOOTI...

I noticed a promising modern approach to this, still _partial, mostly for modern browsers:
https://github.com/WICG/intrinsicsize-attribute/issues/16.

If you think you have seen the big picture and found a better solution than the current hack, I would be very grateful for a better solution. Should that happen, patches are very much welcome.

Marking this Need work for it is not about Aspect ratio alone as mentioned above. The too global approach mentioned here is a consciously lazy one to address issues, of course.

pivica’s picture

There is way too much information to dig here and try to understand, i don't think i have time for that. However, as i understand this hack is related to solving some JS code problems? If this is a case why just not defining a new CSS class rule for this, something like

.media--relative {
  position: relative;
}

and applying that class with JS code or in preprocessor for the cases where you need this?

> I took the risk since it is not over-specified and can be easily overridden as required down the road.

Yes, it is easily overridable but i think this should be solved where the problem is and not enforcing other people to override CSS, if it is not too complicated ;)

gausarts’s picture

Yes, we should correct this, thank you.

Feel free to update it with the relevant loading indicator issue if you are going to follow up.

However, as i understand this hack is related to solving some JS code problems?

No. To solve browser limitations since the rise of RWD years ago. The most recent I found was mentioned above: https://github.com/WICG/intrinsicsize-attribute/issues/16

What I appreciate about this hack is you don't need another plugin like jQuery fitVids, or alike. The same hack applies to any known elements, images or videos, which might have known dimensions, or even to a HTML .block with fixed dimensions.

.. and applying that class with JS code or in preprocessor for the cases where you need this?

No need. What you did is already great, thank you. Just CSS. Only lacking of loading indicator support for when .blazy code is not there. I haven't rechecked anything else, but I guess that will do.

  • gausarts committed 7801bc2 on 8.x-2.x authored by pivica
    Issue #3117132 by pivica: Media position relative rule is too general
    
gausarts’s picture

Status: Needs work » Fixed

It appears the media--loading is already relative. No extra work is needed. The line is duplicated due to they can live separately by settings. Maybe we should merge these two lines later into the newly created blazy.css introduced at 2.0 exactly to address these dups.

If anything I missed, feel free to re-open.
Committed. Thank you for contribution!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.