[Button] Inconsistent Text/Background Colors When Using CSS Variables on Edge

michaeldrotar commented 5 years ago

Bug report

edgeOptOut is enabled for button container fill color but not icon or ink color, leading to buttons that have poor contrast due to the inconsistency in Edge

@mixin mdc-button-container-fill-color($color, $query: mdc-feature-all()) {
  $feat-color: mdc-feature-create-target($query, color);

  // :not(:disabled) is used to support link styled as button
  // as link does not support :enabled style
  &:not(:disabled) {
    @include mdc-feature-targets($feat-color) {
      @include mdc-theme-prop(background-color, $color, $edgeOptOut: true);
    }
  }
}

@mixin mdc-button-ink-color($color, $query: mdc-feature-all()) {
  $feat-color: mdc-feature-create-target($query, color);

  &:not(:disabled) {
    @include mdc-feature-targets($feat-color) {
      @include mdc-theme-prop(color, $color);
    }
  }
}

Note the mdc-theme-prop calls in the mdc code above.

Steps to reproduce

Go to https://codepen.io/michaeldrotar/pen/dybzWBv

Actual behavior

The "themed button" has a light red background with dark text in Chrome/Firefox, but a dark purple background with dark text in Edge (I use browserstack to compare)

Expected behavior

The "themed" button should ideally have a light red background in Edge (css variables appear to be supported now)... or at least keep the white text from the sass configuration

Screenshots

Your Environment:

Software	Version(s)
MDC Web	3.1.0
Browser	Edge 18
Operating System	Windows 10

Additional context

None I can think of

Possible solution

Preferably remove edgeOptOut functionality... if I'm wrong and there's some edge cases that still don't work then possibly make it global so people can opt in or not? If global isn't a good option, possibly every mixin needs to pass it down so people can override it on a case-by-case basis?

abhiomkar commented 5 years ago

Thanks for reporting this issue.

You suggestion to add global flag to edgeOptIn or edgeOptOut looks good to me. PR Welcome! :)

michaeldrotar commented 5 years ago

I tried reaching out on Discord but doesn't seem very active so trying here too..

I've read through the various READMEs, made a fork, got npm i successfully (after doing the build tools and python stuff for win10), ran npm start and npm run test:watch, made my edits... have been double and triple reading the READMEs to see if I missed anything...

The tests say they all pass, but I'm not sure if there's test coverage for some things that I've changed.

I thought I could use the localhost:8080 site to do some manual testing for some peace of mind, but looks like my out dir is empty so I tried some of the build commands in package.json

If I stop the npm start process and run npm run screenshot:build manually, I get this error:

$ npm run screenshot:build

> @ screenshot:build C:\projects\material-components-web
> node test/screenshot/run.js build

Compiling source files

pbjs --target=static-module --wrap=commonjs --keep-case --out=test/screenshot/infra/proto/cbt.pb.js test/screenshot/infra/proto/cbt.proto

ERROR: Error: pbjs process exited with code 1
    at ProcessManager.spawnChildProcessSync (C:\projects\material-components-web\test\screenshot\infra\lib\process-manager.js:88:15)
    at ProtoCommand.runAsync (C:\projects\material-components-web\test\screenshot\infra\commands\proto.js:49:22)
    at buildRightNow (C:\projects\material-components-web\test\screenshot\infra\commands\build.js:94:58)
    at BuildCommand.buildProtoFiles_ (C:\projects\material-components-web\test\screenshot\infra\commands\build.js:98:13)
    at BuildCommand.runAsync (C:\projects\material-components-web\test\screenshot\infra\commands\build.js:68:16)
    at <anonymous>

Run time: 454 milliseconds

I assume this is the issue.. I'm on Win10 using Git Bash. Tried setting breakpoints and looking into pbjs and I'm just circling now... anyone have any ideas?

Here's the failing code.. fails on first loop iteration with the cbt file

    for (const protoFilePath of protoFilePaths) {
      const jsFilePath = protoFilePath.replace(/.proto$/, '.pb.js');
      processManager.spawnChildProcessSync(
        cmd, args.concat(`--out=${jsFilePath}`, protoFilePath), undefined, isWatching
      );
    }

abhiomkar commented 5 years ago

What error are you getting when running npm run start? It'll just bring up the local development server at 8080 port.

michaeldrotar commented 5 years ago

I don't get an error, but it's also not working...

$ npm run start

> @ start C:\projects\material-components-web
> npm-run-all --parallel screenshot:serve screenshot:watch

> @ screenshot:serve C:\projects\material-components-web
> node test/screenshot/run.js serve

> @ screenshot:watch C:\projects\material-components-web
> node test/screenshot/run.js build --watch

==========================================================
Local development server running on http://localhost:8080/
==========================================================

Compiling source files

npm run screenshot:webpack -- --watch --mode=development
test/screenshot/infra/proto/cbt.pb.js
pbjs --target=static-module --wrap=commonjs --keep-case --out=test/screenshot/infra/proto/cbt.pb.js test/screenshot/infra/proto/cbt.proto
pbjs process exited with code 1
test/screenshot/infra/proto/github.pb.js
pbjs --target=static-module --wrap=commonjs --keep-case --out=test/screenshot/infra/proto/github.pb.js test/screenshot/infra/proto/github.proto
pbjs process exited with code 1
test/screenshot/infra/proto/mdc.pb.js
pbjs --target=static-module --wrap=commonjs --keep-case --out=test/screenshot/infra/proto/mdc.pb.js test/screenshot/infra/proto/mdc.proto
pbjs process exited with code 1
test/screenshot/infra/proto/selenium.pb.js
pbjs --target=static-module --wrap=commonjs --keep-case --out=test/screenshot/infra/proto/selenium.pb.js test/screenshot/infra/proto/selenium.proto
pbjs process exited with code 1

material-components / material-components-web