@sgiehl opened this Pull Request on September 29th 2020 Member

fixes #16436

@tsteur commented on September 29th 2020 Member

@sgiehl I think it needs a minor patch to align the icon when using the comparison feature? It seems now to high instead of too low

image

@mattab commented on September 29th 2020 Member

Could we backport this fix to 3.x, as it's a visual regression that almost everyone will notice, and some will use 3.x for another year or more?

@tsteur commented on September 29th 2020 Member

@mattab we'll see. We might not even release another 3.X anytime soon and in general it's probably not too important.

@sgiehl commented on September 30th 2020 Member

@tsteur is that from the UI tests? Not sure why it's not positioned correctly there. But it looks correct locally in Chrome and Firefox 🤷

@tsteur commented on September 30th 2020 Member

@sgiehl it looks fine in some reports when comparing (eg pages) but eg in "Websites" there seems to be a slight issue for me in my local install

image

also like in the tests in subtables the issue is happening
image

@sgiehl commented on October 1st 2020 Member

Ok. Hope everything looks good now.

Note: Bandwidth screenshots need to be updated before merging...

@tsteur commented on October 1st 2020 Member

image
image

The icons still seem to be a bit off for me eg in websites report @sgiehl ? I made sure the CSS changes were applied by deleting the generated css file unless it's otherwise not applied. Can you have a look again maybe?

In pages report it needs to maybe one pixel down as well not sure but noticed when moving 1px it actually does nothing and setting a margin top of 2px is too much again so it might be fine

image

@sgiehl commented on October 2nd 2020 Member

@tsteur Then I guess we may need to refactor the whole HTML / CSS around the icons in datatables. Seems the position currently kind of depends on the rendering of the font. That's also the reason why it looks good on my machine, but slightly different in the UI tests.

That's how it looks for me on Windows (same for Chrome, Firefox and IE)
image

Could you maybe check how it looks if you remove the apple fonts from the active font list?

@tsteur commented on October 5th 2020 Member

@sgiehl would it help to use font icons?

There are indeed differences depending on which font is used.

I wonder though why it used to work? There must be something that changed? I suppose could go back in versions to find out which version introduced the regression?

I was going to say maybe should no longer use system fonts by default and rather use a font that is available everywhere for a more consistent look across operating systems and browsers but finding a good font that is available everywhere might not be that easy either.

@Findus23 commented on October 5th 2020 Member

I was going to say maybe should no longer use system fonts by default and rather use a font that is available everywhere for a more consistent look across operating systems and browsers but finding a good font that is available everywhere might not be that easy either.

I don't really think it is possible to use fonts that are the same on every operating system. Without webfonts, there is no font that is available on every OS. And even with webfonts on has the flash during loading of the font.

Personally, I also prefer fonts to look like users expect them and choose them (for their environment) rather than the same everywhere.

Maybe an alternative would be (inline) SVG files? They are pretty well-supported, but work more like images than fonts. If one doesn't want to duplicate the same SVG over and over, one could use defs and use like this: https://css-tricks.com/lodge/svg/13-svg-icon-system-use-element/

@sgiehl commented on October 5th 2020 Member

I've now included the icons in the Matomo font. Hope it now also works on a mac.
The UI tests currently failing, are those from the submodules. Will need to update them before merging.

@tsteur commented on October 5th 2020 Member

image

@sgiehl on Mac - Chromium it's now a pixel or two too high but at least the behaviour seems to be consistent across all reports, comparison feature, subtables. Would still need to move the icon though
image

Might also be good to use browserstack if needed?

@sgiehl commented on October 6th 2020 Member

@tsteur It still looks good on my side. So maybe it's still font related 🤷
btw. don't have access to browserstack

@tsteur commented on October 6th 2020 Member

Sent you the login data @sgiehl maybe you can have a look there? Wonder though how it used to work. There must have been some change?

@sgiehl commented on October 8th 2020 Member

@tsteur I've looked through the UI screenshot history and it seems at least the misplacement of the link icon in action tables came up with the comparison feature.
I've tried a lot now to get that looking the same on all devices / browsers, but that is not really possible, as the font used on each device might be another one. The problem seems to be that each font has another vertical alignment, which causes the icons not to be aligned them same as the font used for the text. Tried to improve that by using the same font size and line height on all elements, but it still does not look the same for all fonts... You can easily reproduce that by changing the used font in the developer tools...
Maybe it might be possible to do that with a clean new layout, but it's currently simply not possible with the amount of css rules that have effect on the datatables...
If we want to have a solution that looks the same on all devices quickly we might need to consider to use the same font everywhere 🤷

@tsteur commented on October 8th 2020 Member

OK sounds good to make these changes. Was having a very quick look as well since it works for the plus image nicely. Something like this almost worked quite well for me except for maybe 1px in another font which may be fixed by using a square image as well. But then I didn't test it in subtables etc. It'll be great to use consistently the same css for all icons etc 👍
image

@sgiehl commented on October 9th 2020 Member

@tsteur Did you have a look at the changes of this PR? The link and plus/minus images were actually replaced with a font icons here. So it's handled the same way for all datatables. Before action tables used an image for plus/minus images, but all other tables used a background image instead. I've also improved the handling of showing the plus/minus icon in action tables, so it's more like that for normal tables...
I'll look through the UI screenshot changes and updated them. Afterwards guess it would imho be good enough...

@tsteur commented on October 11th 2020 Member

@sgiehl great. It looks definitely better now and great to see icons there instead of images.

The plus/minus icon looks still 1px too high.
image

This is also the case for me for all other fonts except for "sans-serif". Not sure what font Mac than is falling back to. I see though in the screenshots it seems to look fine (eg in https://media.githubusercontent.com/media/matomo-org/matomo/0e4626bf289fd16172ccbcbb787fe6923da34bef/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_pageview_percentages.png). Although maybe it could also be moved one pixel further down if any possible? (In icomoon we could if needed move the icon one pixel down when generating the font AFAIK).

It's nicely aligned in the tests but wonder if it was possible to move one pixel down because mostly we would be having maybe lower case and the plus is quite a bit bigger than the lower letter?
image

Don't know what it looks like though on Linux/Windows.

fyi on mac with upper case letters
image

when using sans serif then it looks really well on Mac and wouldn't want to have it moved a pixel down. I assume this is the case for Windows/Linux too? So maybe we wouldn't want to move it down a pixel.

@sgiehl commented on October 12th 2020 Member

@tsteur I've moved the icons a pixel down in the font... Let me know if it's good to merge. Would then update the expected screenshots in the submodules before merging...

@tsteur commented on October 12th 2020 Member

@sgiehl I'm still kind of having the same issue (100% cache was not used and using updated file). I think it moved it maybe slightly down. Like when I zoom in I can see at least the plus is on the same line as the font
image

but when viewing it in original size it's not 100% correct and still bit too high. I don't know if it can be moved again a bit further down or so?

image
image

I wonder actually if a different font has a better icon. This icon doesn't seem to be as much suited for being displayed so small. Even on a retina display it looks a bit pixelated due to some aliasing or so
image

On my regular screen the icon looks quite blurry
image

It's not as much visible in a screenshot I think. Other fonts might have the same problem though considering we show it so small (and my regular screen is not the best but users might have similar screens).

Tested some other icons like from material design
image

but they have issue too see above regular screen vs below retina (looking a lot better)
image

Is there maybe some font smoothing or so that can be disabled to make this better or maybe something needs to be enabled?

Funny thing: The old image that we were using looks good on my regular screen but bad on the retina screen :)

It might not be too important for now just thought I mention it. I don't know what it looks like on Windows/Linux and whether it can be again moved down another pixel the plus / minus icon. The outlink is fine and not needs changing

@sgiehl commented on October 13th 2020 Member

On windows the align is actually fine for the plus/minus icon. I've added some additional css for the icon to improve the smoothing... Can't see a difference on Windows, but maybe it helps for Mac

@tsteur commented on October 13th 2020 Member

Doesn't really change anything here. I'm thinking the icon might not be best but I guess most other icons would have same problem since it's shown so small. Could we maybe show the icon bigger? Like below (font-size:16px). In my case it even improves the position of the icon a bit. I guess the current 13px results in a lot of "rounding/smoothing" and is generally not as much visible.

image

compared to standard 13px
image

@sgiehl commented on October 14th 2020 Member

increasing the icon size sounds fine. will update the css and expected screenshots...

@tsteur commented on October 14th 2020 Member

That works well on Mac 👍 not sure what it's like on Windows or Linux but for me it looks good to merge

This Pull Request was closed on October 15th 2020
Powered by GitHub Issue Mirror