Render nav icons in IE in white like the other supported browsers #7731

ppisljar · 2016-07-13T13:09:38Z

internet explorer doesnt support css3 filter property (which is used to invert images)
i created a directive which uses SVG filters to invert the image in case css3 filter property is not supported.

Bargs · 2016-07-26T15:49:38Z

src/ui/public/directives/ie_invert.js

+        opts.svgHeight = $elem.height();
+        opts.svgSource = $elem.attr('src');
+
+        opts.svgContent = `<svg xmlns="http://www.w3.org/2000/svg" class="${$elem[0].className}" viewBox="0 0 ${opts.svgWidth} ${opts.svgHeight}" width="${opts.svgWidth}" height="${opts.svgHeight}">`


Since this is fairly large, how about pulling it out into a template file?

Bargs · 2016-07-26T16:05:53Z

Rather than applying the invert filter in two different places (.app-icon css rule and this directive), how about we call this directive filter-invert, give it a % parameter, and have it take care of doing the invert in all browsers? That'll make things easier for anyone who wants to apply an invert in the future.

Bargs · 2016-07-26T16:07:08Z

src/ui/public/directives/ie_invert.js

+        $(opts.rootElm).addClass(opts.rootFlag);
+      }
+
+      if (!isFilterSupported()) {


There should also be a check for whether the element is actually an SVG or not.

it works with all kind of images

Then we should do a check to confirm it's being applied to an img tag

ppisljar · 2016-07-26T16:08:13Z

makes sense

Bargs · 2016-07-26T16:10:20Z

src/ui/public/chrome/directives/app_switcher/app_switcher.less

+      > svg {
+        height: 18px;
+        margin-top: 10px;
+      }


The directive should work without needing additional styles applied in an external stylesheet.

Bargs · 2016-07-26T16:21:01Z

If this workaround came from a blog or library somewhere, I'd recommend linking to the source either in the code or in your commit message so other devs can read the details in the future if need be.

Bargs · 2016-07-26T16:26:40Z

src/ui/public/directives/ie_invert.js

+
+      if (!isFilterSupported()) {
+        $elem.css('visibility', 'hidden');
+        $elem.on('load', () => { invert($elem); });


Does this really need to wait for onload?

i need image width& height

ppisljar · 2016-07-28T08:37:47Z

src/ui/public/directives/filter_invert.js

+
+      function invert($elem) {
+        $scope.opts = {
+          idHash: (Math.random().toString(36) + '00000000000000000').slice(2, 7),


do we have a method to generate random string ?

Not that I know of, but does this really need to be random? As far as I can tell, IDs defined inside the SVGs defs element are scoped to the individual SVG, so this can probably just be hard coded?

Bargs · 2016-07-28T13:48:14Z

src/ui/public/directives/filter_invert.js

+          svgWidth: $elem.width(),
+          svgHeight: $elem.height(),
+          svgSource: $elem.attr('src'),
+          className: $elem[0].className


$elem.attr('class') doesn't work?

Bargs · 2016-07-28T13:54:32Z

Looks like this branch needs to be rebased on master as well

ppisljar · 2016-07-29T08:18:15Z

i've tried to make it work with different values but it gives different result than css filter property. i don't really understand how css filter property works ... what should invert(50%) do ?

invert(100%) would do 255-c for each color channel ....

epixa · 2016-08-20T16:13:21Z

This needs to be rebased or have master merged.

Bargs · 2016-08-23T21:38:28Z

@ppisljar it seems like the unsupported browser test isn't done yet, is that right? Just making sure this isn't blocked on me at the moment.

Bargs · 2016-08-26T19:14:49Z

Please re-assign me when this is ready for another review

for browsers that dont support css3 filter property image is inverted with SVG filters

Bargs · 2016-09-08T22:51:18Z

Sorry for the delay on getting back to this. LGTM! 🎉 🎉 🎉 🎉 🎉

thomasneirynck · 2016-09-09T22:55:17Z

LGTM,

Perhaps I'm issing something crucial, but I would consider one thing though. This is a LOT of code for a minor cosmetic annoyance. I know it's nice to isolate styling in CSS, but we're adding a whole new compile step for something IMHO is quite minor.

Why not (?)

consider just making a white version of the svg icon offline (add fill=white to the svg-icons) (this would leave 3rd party plugin-icons un-inverted. But are they OK to begin with that we are inverting their icon color on-the-fly?)
consider rendering the SVG icon raw. Then we only need to change the existing 'global_nav_link.html' file along the lines of.

      //remove this junky IE-incompatile html
      <!--<img-->
      <!--ng-if="icon"-->
      <!--class="global-nav-link__icon-image"-->
      <!--kbn-src="{{ '/' + icon }}"-->
      <!--&gt;-->


      //replace it with this raw SVG
      <div class="global-nav-link__icon-image">
        <svg width="20" height="20" viewBox="0 0 520 520">
          <defs>
            <filter id="matrix-invert">
              <feColorMatrix in="SourceGraphic" type="matrix" values="-1 0 0 0 1 0 -1 0 0 1 0 0 -1 0 1 0 0 0 1 0"/>
            </filter>
          </defs>

             <!--do need to change the rewrite the kbn-src conversion to a kbn-href conversion-->
          <image x="0" y="0" width="520" height="520" preserveAspectRatio="true" filter="url(#matrix-invert)" xlink:href="{{ '/bzg/' + icon }}"/>
        </svg>
      </div>

Both approaches would just be a spot change, iso. creating a whole new programmatic component just to redress this single coloring issue.

But again, maybe I'm missing something obvious.

cjcenizal · 2016-09-09T23:02:36Z

I think @thomasneirynck is 100% correct here... the problem is the assets are black, but we want them rendered white. So why not just make the assets white? :)

ppisljar · 2016-09-10T05:42:16Z

i couldn't agree more @thomasneirynck ... if everyone is ok with it i am gonna throw this away and create a new PR where i'll just change the assets to white. It doesnt seem that we use them black at all. i was wondering if dark theme has any effect on how this is rendered, but it doesnt.

@Bargs what do you think ?

Bargs · 2016-09-12T14:11:28Z

Sounds fine, we don't use the filter property for this purpose anywhere else anyway.

ppisljar · 2016-09-12T14:48:48Z

closing in favour of #8221

ppisljar added the review label Jul 13, 2016

ppisljar assigned Bargs Jul 13, 2016

Bargs reviewed Jul 26, 2016
View reviewed changes

Bargs added updates_needed and removed review labels Jul 26, 2016

Bargs assigned ppisljar and unassigned Bargs Jul 26, 2016

ppisljar reviewed Jul 28, 2016
View reviewed changes

ppisljar assigned Bargs and unassigned ppisljar Jul 28, 2016

ppisljar added review and removed updates_needed labels Jul 28, 2016

Bargs reviewed Jul 28, 2016
View reviewed changes

Bargs assigned ppisljar and unassigned Bargs Jul 28, 2016

Bargs added updates_needed and removed review labels Jul 28, 2016

ppisljar force-pushed the fix/7528 branch from fafcfab to 9a63a3a Compare July 29, 2016 07:58

Bargs removed the review label Aug 18, 2016

ppisljar added review and removed updates_needed labels Aug 18, 2016

epixa added updates_needed and removed review labels Aug 20, 2016

ppisljar force-pushed the fix/7528 branch from 22168a9 to c4f8691 Compare August 21, 2016 08:10

ppisljar added review and removed updates_needed labels Aug 21, 2016

Bargs removed their assignment Aug 26, 2016

ppisljar added 4 commits August 27, 2016 16:48

fix elastic#7528 - IE shows BLACK nav icons instead of white

277d186

for browsers that dont support css3 filter property image is inverted with SVG filters

trying to fix tests

ad9dd83

trying to fix tests

6b45f6f

fixing tests

a93b190

ppisljar force-pushed the fix/7528 branch from f955db6 to a93b190 Compare August 27, 2016 14:48

ppisljar assigned Bargs Aug 27, 2016

Bargs removed their assignment Sep 8, 2016

ppisljar assigned cjcenizal and thomasneirynck and unassigned cjcenizal Sep 9, 2016

epixa changed the title ~~fix #7528 - IE shows BLACK nav icons instead of white~~ Render nav icons in IE in white like the other supported browsers Sep 9, 2016

ppisljar closed this Sep 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render nav icons in IE in white like the other supported browsers #7731

Render nav icons in IE in white like the other supported browsers #7731

ppisljar commented Jul 13, 2016 •

edited by epixa

Loading

Bargs Jul 26, 2016

Bargs commented Jul 26, 2016

Bargs Jul 26, 2016

ppisljar Jul 28, 2016

Bargs Jul 28, 2016

ppisljar commented Jul 26, 2016

Bargs Jul 26, 2016

Bargs commented Jul 26, 2016

Bargs Jul 26, 2016

ppisljar Jul 28, 2016

ppisljar Jul 28, 2016

Bargs Jul 28, 2016

Bargs Jul 28, 2016

Bargs commented Jul 28, 2016

ppisljar commented Jul 29, 2016

epixa commented Aug 20, 2016

Bargs commented Aug 23, 2016

Bargs commented Aug 26, 2016

Bargs commented Sep 8, 2016 •

edited

Loading

thomasneirynck commented Sep 9, 2016

cjcenizal commented Sep 9, 2016

ppisljar commented Sep 10, 2016

Bargs commented Sep 12, 2016

ppisljar commented Sep 12, 2016

Render nav icons in IE in white like the other supported browsers #7731

Render nav icons in IE in white like the other supported browsers #7731

Conversation

ppisljar commented Jul 13, 2016 • edited by epixa Loading

Choose a reason for hiding this comment

Bargs commented Jul 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Jul 26, 2016

Choose a reason for hiding this comment

Bargs commented Jul 26, 2016

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

Bargs commented Jul 28, 2016

ppisljar commented Jul 29, 2016

epixa commented Aug 20, 2016

Bargs commented Aug 23, 2016

Bargs commented Aug 26, 2016

Bargs commented Sep 8, 2016 • edited Loading

thomasneirynck commented Sep 9, 2016

cjcenizal commented Sep 9, 2016

ppisljar commented Sep 10, 2016

Bargs commented Sep 12, 2016

ppisljar commented Sep 12, 2016

ppisljar commented Jul 13, 2016 •

edited by epixa

Loading

Bargs commented Sep 8, 2016 •

edited

Loading