Regressions after reordering mixin inclusions above other styles

[kfranqueiro]

What MDC-Web Version are you using?

master, comparing against 0.20.0

What browser(s) is this bug affecting?

Observed in Chrome, presumably affects all

What OS are you using?

OS X

What are the steps to reproduce the bug?

Run the dev server and observe card.html on master vs. how it looks in 0.20.0

What is the expected behavior?

They should look the same; we've done nothing to intentionally change MDC Card

What is the actual behavior?

Some paddings disappeared. Here's a visual diff:

Any other information you believe would be useful?

I git bisected and this was caused by #1265.

We'll have to investigate, but it might be a bad idea to be opinionated on ordering mixin invocations because maybe it can influence ordering of child selectors?

[kfranqueiro]

The issue causing this is that we have conflicting padding styles on &__horizontal-block - we include RTL styles which set padding-left and padding-right, but then we also set padding to 0 explicitly. Due to moving the mixin inclusion, the order swapped and caused this change. I'm not sure why we have both, but I'm assuming the RTL styles are the intended styles since that's what we used before.

I can send out a PR for this, but I'm also inclined to forage through a full diff of the CSS before/after #1265 to see if any other mixin reorderings caused problems. If the extent of our problems is where we had conflicting styles within a single block anyway, then I'm less concerned about #1265 itself.

[kfranqueiro]

Other issues due to @include reordering: (I've listed symptoms right now, I intend to research how straightforward the solutions might be and update this again later)

MDC Drawer

Symptom

In each type of MDC Drawer, .mdc-...-drawer .mdc-list-item now has text-decoration set to none (was inherit). There might not be any visible consequences of this within our demos.

Cause

MDC Typography recently was updated to set text-decoration (to inherit, in most cases, including this one). Technically, that in itself caused a regression in drawer, because it ended up overriding drawer's existing text-decoration: none style. This regression undoes the earlier regression!

Resolution

Don't do anything because it's now working as it was originally intended?

I do wonder if it's a bad thing to always be setting text-decoration in typography styles when in most cases it doesn't need to be set...

MDC Select

Background image

Symptom

background-image: ... now appears before background: none, causing the arrow at the right side of the select drop-down to disappear in the enabled state.

Cause

MDC Select uses an internal mixin to generate background-image data URLs, but it also sets background: none on the root element. I'm not sure why the latter is done; shouldn't that already be the default? Did we ever support applying this directly to a select element and maybe that's why?

Resolution

Remove background: none, presuming it isn't needed (I tried disabling it and it didn't seem to cause any regressions).

We could also contemplate changing this internal mixin to a function instead, making it return specifically the data URL, then use it within individual style rules rather than forcing background-image to be used, since for instance there are a bunch of separated background-* styles on .mdc-select.

Multi-select border

Symptom

border: 1px solid is now below the specific border-color styles, which ends up overriding border-color with its initial value. Easily visible in select.html.

Cause

border-color is being set with mdc-theme-prop. IMO it might not be the best idea for readability to hoist mdc-theme-prop usages since they're more like an individual property than other mixin invocations.

Resolution

Use border-style and border-width separately... or unhoist the include for readability's sake.

Multi-select checked list-item

Symptom

The background styles were reordered; previously was rgba(0, 0, 0, 0.12) then #fff then var(...), now rgba(...) is last and wins. This causes a noticeable difference in our demo page in Edge.

Cause

This was caused by a mdc-theme-prop inclusion for background being reordered above another background setting, when it was previously immediately below.

I'm not actually sure if the other background setting is needed, though...

Resolution

Either remove the other background setting, or we will ultimately need to whitelist mdc-theme-prop inclusions to resolve this.