Issue 6209 - Updated woff fonts and table status column for long texts

* Old fonts were missing Cyrillic Characters, so most of the fonts were rendered as Arial.
* The fonts are copied from -> https://github.com/adobe-fonts/source-sans-pro/tree/release/WOFF/OTF 
* New fonts are around 120kb each while old fonts were around 17kb each.

[saroyanm, 2018-01-02 15:20:09]

@Thomas can you please let me know if you aware of any font compression tool we
were using for initial/old "woff" files ? My assumption is that old woff files
were only using Latin characters, so that why the file size was so small. For
the refference -> https://codereview.adblockplus.org/29321240/

@Frontend: While having big font size is fine in the extension, considering the
fact that they are hosted locally, I think we should use optimized version in
the website if possible, before updating them, as I can assume same issue might
appear there as well, for example here -> https://adblockbrowser.org/ru/


Note: We might also want to make use of woff2 in addition in current review,
though the file size is only 20kb less.

[saroyanm, 2018-01-02 15:39:23]

https://codereview.adblockplus.org/29654555/diff/29654560/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29654560/skin/desktop-option...
skin/desktop-options.css:23: src: local("Ø"),
url("fonts/Source-Sans-Pro/300.woff2") format("woff2"),
I think we should update the folder structure and adjust @font-face accordingly
in current review, I suggest:
skin/
  fonts/
    Source-Sans-Pro/
      woff/
        light.woff
        regular.woff
        bold.woff
      woff2/
        light.woff2
        regular.woff2
        bold.woff2

[saroyanm, 2018-01-02 17:36:11]

https://codereview.adblockplus.org/29654555/diff/29654568/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29654568/skin/desktop-option...
skin/desktop-options.css:1002: -moz-margin-end: 0.5rem;
I'll reflect this changes in the Specs. accordingly if Jeen agree on using 0.5em
instead (I'm followup after she answer me in IRC).

[Thomas Greiner, 2018-01-08 15:53:39]

On 2018/01/02 15:20:09, saroyanm wrote:
> @Thomas can you please let me know if you aware of any font compression tool
we
> were using for initial/old "woff" files ? My assumption is that old woff files
were only using Latin characters, so that why the file size was so small. For
the refference -> https://codereview.adblockplus.org/29321240/

I'm not aware of any but how would we know which characters we need?

If we think that this needs to be optimized, we could, for instance, split up
the different character sets into separate font files and ensure that only
relevant font files are downloaded/used by using "unicode-range" (see
https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/unicode-range).

In any case, compatibility with all supported browsers should be paramount.

[saroyanm, 2018-01-10 10:49:21]

On 2018/01/08 15:53:39, Thomas Greiner wrote:
> On 2018/01/02 15:20:09, saroyanm wrote:
> > @Thomas can you please let me know if you aware of any font compression tool
> we
> > were using for initial/old "woff" files ? My assumption is that old woff
files
> were only using Latin characters, so that why the file size was so small. For
> the refference -> https://codereview.adblockplus.org/29321240/
> 
> I'm not aware of any but how would we know which characters we need?
> 
> If we think that this needs to be optimized, we could, for instance, split up
> the different character sets into separate font files and ensure that only
> relevant font files are downloaded/used by using "unicode-range" (see
> https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/unicode-range).
> 
> In any case, compatibility with all supported browsers should be paramount.

As was suggested by Thomas, Google fonts API already splits Source Sans Pro for
common character ranges
https://fonts.googleapis.com/css?family=Source+Sans+Pro&subset=cyrillic,cyril...

[saroyanm, 2018-01-10 11:03:20]

https://codereview.adblockplus.org/29654555/diff/29654568/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29654568/skin/desktop-option...
skin/desktop-options.css:23: src: local("Ø"),
url("fonts/Source-Sans-Pro/300.woff2") format("woff2"),
Adjust as was mentioned in the regular Comments of this review ->
https://fonts.googleapis.com/css?family=Source+Sans+Pro&subset=cyrillic,cyril...

https://codereview.adblockplus.org/29654555/diff/29654568/skin/desktop-option...
skin/desktop-options.css:1002: -moz-margin-end: 0.5rem;
On 2018/01/02 17:36:11, saroyanm wrote:
> I'll reflect this changes in the Specs. accordingly if Jeen agree on using
0.5em
> instead (I'm followup after she answer me in IRC).

This change was aligned with Jeen and the issue to reflect this change in the
specification as well is filed:
https://gitlab.com/eyeo/spec/issues/126

[saroyanm, 2018-01-29 16:16:19]

New patch uploaded.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
I added fonts from the URLS bellow accordingly:

*
https://fonts.googleapis.com/css?family=Source+Sans+Pro:300&subset=cyrillic,c...

*
https://fonts.googleapis.com/css?family=Source+Sans+Pro:400&subset=cyrillic,c...
*
https://fonts.googleapis.com/css?family=Source+Sans+Pro:700&subset=cyrillic,c...


@Thomas if you agree with current implementation, I'll add same fonts into
mobile-options.css and issue-reporter.css and remove unused fonts.

As soon we will have SASS support we will be able to move the fonts into a
separate file and avoid duplication.

[Thomas Greiner, 2018-02-14 17:13:46]

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/01/29 16:16:17, saroyanm wrote:
> I added fonts from the URLS bellow accordingly:

Anything we need to consider when using the fonts from this source? Are they
under a different license then the one we already include in the repo?

On 2018/01/29 16:16:17, saroyanm wrote:
> @Thomas if you agree with current implementation, I'll add same fonts into
> mobile-options.css and issue-reporter.css and remove unused fonts.
> 
> As soon we will have SASS support we will be able to move the fonts into a
> separate file and avoid duplication.

I don't think we want to duplicate them across all CSS files so why not just
separate them into a "fonts.css" file already?

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
Can you verify that we actually need each of those unicode ranges?

If there's a language we don't support or generally, if there are characters we
refer to here which we don't use, we don't need to include them here either.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:26: unicode-range: U+0460-052F, U+1C80-1C88, U+20B4,
U+2DE0-2DFF, U+A640-A69F, U+FE2E-FE2F;
Coding style: "Line length: 80 characters or less"

Also applies to other occurrences in this file.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:1144: flex: 5;
If the design doesn't consider long texts, there's not much we can do. Therefore
I'd argue that we shouldn't optimize for exceptions but instead make sure that
the design works for most users.

In this case, for instance, we'd be unnecessarily increasing the column width
for all languages even though most of them may not even have this issue.

What I'd suggest is to feed this issue back to Product and resolve the
underlying issue rather than patching one hole after another (see also
"Disabled" label in Spanish).

[saroyanm, 2018-02-14 18:30:05]

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/14 17:13:45, Thomas Greiner wrote:
> Can you verify that we actually need each of those unicode ranges?
> 
> If there's a language we don't support or generally, if there are characters
we
> refer to here which we don't use, we don't need to include them here either.

Not sure how I can verify ? 
We do support "cyrillic" languages, "greek", "vietnamese", I'm not sure if some
of them are using or might use characters in future that are part of extended
version.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/14 17:13:46, Thomas Greiner wrote:
> On 2018/01/29 16:16:17, saroyanm wrote:
> > I added fonts from the URLS bellow accordingly:
> 
> Anything we need to consider when using the fonts from this source? Are they
> under a different license then the one we already include in the repo?
Only thing I could find is from -> https://developers.google.com/fonts/faq 
The open source fonts in the Google Fonts catalog are published under licenses
that allow you to use them on any website, whether it’s commercial or personal. 
Search queries may surface results from external foundries, who may or may not
use open source licenses.

Source Sans Pro is "OFL" and links to actual license from here ->
https://fonts.google.com/specimen/Source+Sans+Pro 

I'll include the license as a separate file.

> On 2018/01/29 16:16:17, saroyanm wrote:
> > @Thomas if you agree with current implementation, I'll add same fonts into
> > mobile-options.css and issue-reporter.css and remove unused fonts.
> > 
> > As soon we will have SASS support we will be able to move the fonts into a
> > separate file and avoid duplication.
> 
> I don't think we want to duplicate them across all CSS files so why not just
> separate them into a "fonts.css" file already?

I agree, mind if I use Import until we have CSS preprocessor in place
https://developer.mozilla.org/en-US/docs/Web/CSS/%40import ?

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:26: unicode-range: U+0460-052F, U+1C80-1C88, U+20B4,
U+2DE0-2DFF, U+A640-A69F, U+FE2E-FE2F;
On 2018/02/14 17:13:45, Thomas Greiner wrote:
> Coding style: "Line length: 80 characters or less"
> 
> Also applies to other occurrences in this file.

Will fix this.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:1144: flex: 5;
On 2018/02/14 17:13:45, Thomas Greiner wrote:
> If the design doesn't consider long texts, there's not much we can do.
Therefore
> I'd argue that we shouldn't optimize for exceptions but instead make sure that
> the design works for most users.
> 
> In this case, for instance, we'd be unnecessarily increasing the column width
> for all languages even though most of them may not even have this issue.
> 
> What I'd suggest is to feed this issue back to Product and resolve the
> underlying issue rather than patching one hole after another (see also
> "Disabled" label in Spanish).

I agree that this need to be fixed in design, but mind if we keep this as a
stopgap solution, because currently this is the only column that has both
elements next to each other, though we might change this, it was done so to
easily change the state using sibling selector.

I'll followup and create a Gitlab issue to address that as well.

[Thomas Greiner, 2018-02-15 12:17:07]

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/14 18:30:04, saroyanm wrote:
> Not sure how I can verify ? 
> We do support "cyrillic" languages, "greek", "vietnamese", I'm not sure if
some
> of them are using or might use characters in future that are part of extended
> version.

I assumed there'd be resources out there that clarify that since that's quite an
important aspect of which ranges to choose. But all I found was
http://cldr.unicode.org/ which is not too easy to extract information from.

So let's simplify this:
- If there's at least one language for each Unicode block (e.g. "latin"), we
should keep it. Presumably, that's all of them anyway.
- If there's a language we support that's not covered by any of the Unicode
blocks, we're missing characters. It may be sufficient to check for support of
arabic, chinese, cyrillic, greek, japanese and latin but let's check back with
C&T on that.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/14 18:30:04, saroyanm wrote:
> Only thing I could find is from -> https://developers.google.com/fonts/faq 
> The open source fonts in the Google Fonts catalog are published under licenses
> that allow you to use them on any website, whether it’s commercial or
personal. 
> Search queries may surface results from external foundries, who may or may not
> use open source licenses.
> 
> Source Sans Pro is "OFL" and links to actual license from here ->
> https://fonts.google.com/specimen/Source+Sans+Pro 
> 
> I'll include the license as a separate file.

Ok. From what I read in the license text, the requirements are:
- Include the license text in adblockplusui and in the extension build
- Include the copyright notice in adblockplusui and in the extension build

The Google Fonts page doesn't mention though what the copyright notice should
say so I did a quick search and found the font's official site at
https://typekit.com/fonts/source-sans. There you find links on the bottom for
each font variant. Those pages contain the license as well as the copyright
notice we need to include.

This also matches the license file that I found in the repo where you got the
font files from:
https://github.com/adobe-fonts/source-sans-pro/blob/release/LICENSE.txt

We could even include the fonts as an npm package if we want (see
https://github.com/adobe-fonts/source-sans-pro/blob/release/README.md#font-in...
and https://www.npmjs.com/package/source-sans-pro).

> I agree, mind if I use Import until we have CSS preprocessor in place
> https://developer.mozilla.org/en-US/docs/Web/CSS/%40import ?

I think there's a misunderstanding. What I'm suggesting is to include it as
follows:

<head>
  ...
  <link rel="stylesheet" type="text/css" href="fonts.css">
  <link rel="stylesheet" type="text/css" href="desktop-options.css">
  ...
</head>

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:1144: flex: 5;
On 2018/02/14 18:30:04, saroyanm wrote:
> I agree that this need to be fixed in design, but mind if we keep this as a
> stopgap solution, because currently this is the only column that has both
> elements next to each other, though we might change this, it was done so to
> easily change the state using sibling selector.
> 
> I'll followup and create a Gitlab issue to address that as well.

Ok, I'm fine with that.

[saroyanm, 2018-02-15 18:47:19]

@Thomas I think this issue looks more complex than it has seemed initially, I'll
suggest to have a quick call tomorrow if you have time to find out how we can
proceed with this issue.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/15 12:17:06, Thomas Greiner wrote:
> On 2018/02/14 18:30:04, saroyanm wrote:
> > Only thing I could find is from -> https://developers.google.com/fonts/faq 
> > The open source fonts in the Google Fonts catalog are published under
licenses
> > that allow you to use them on any website, whether it’s commercial or
> personal. 
> > Search queries may surface results from external foundries, who may or may
not
> > use open source licenses.
> > 
> > Source Sans Pro is "OFL" and links to actual license from here ->
> > https://fonts.google.com/specimen/Source+Sans+Pro 
> > 
> > I'll include the license as a separate file.
> 
> Ok. From what I read in the license text, the requirements are:
> - Include the license text in adblockplusui and in the extension build
> - Include the copyright notice in adblockplusui and in the extension build
> 
> The Google Fonts page doesn't mention though what the copyright notice should
> say so I did a quick search and found the font's official site at
> https://typekit.com/fonts/source-sans. There you find links on the bottom for
> each font variant. Those pages contain the license as well as the copyright
> notice we need to include.
> 
> This also matches the license file that I found in the repo where you got the
> font files from:
> https://github.com/adobe-fonts/source-sans-pro/blob/release/LICENSE.txt
> 
> We could even include the fonts as an npm package if we want (see
>
https://github.com/adobe-fonts/source-sans-pro/blob/release/README.md#font-in...
> and https://www.npmjs.com/package/source-sans-pro).
If we include Font as an npm package, we don't need to include License ?

> > I agree, mind if I use Import until we have CSS preprocessor in place
> > https://developer.mozilla.org/en-US/docs/Web/CSS/%40import ?
> 
> I think there's a misunderstanding. What I'm suggesting is to include it as
> follows:
> 
> <head>
>   ...
>   <link rel="stylesheet" type="text/css" href="fonts.css">
>   <link rel="stylesheet" type="text/css" href="desktop-options.css">
>   ...
> </head>
Sure, I was referring to use @include rather than manage another file in HTML,
but if you have a strong preference to current method, I'll go for it.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/15 12:17:06, Thomas Greiner wrote:
> On 2018/02/14 18:30:04, saroyanm wrote:
> > Not sure how I can verify ? 
> > We do support "cyrillic" languages, "greek", "vietnamese", I'm not sure if
> some
> > of them are using or might use characters in future that are part of
extended
> > version.
> 
> I assumed there'd be resources out there that clarify that since that's quite
an
> important aspect of which ranges to choose. But all I found was
> http://cldr.unicode.org/ which is not too easy to extract information from.
> 
> So let's simplify this:
> - If there's at least one language for each Unicode block (e.g. "latin"), we
> should keep it. Presumably, that's all of them anyway.
> - If there's a language we support that's not covered by any of the Unicode
> blocks, we're missing characters. It may be sufficient to check for support of
> arabic, chinese, cyrillic, greek, japanese and latin but let's check back with
> C&T on that.

I don't think that C&T ever noticed that we had that problem for more than 3
years.

Result of my tests:
Arabic, Chinese, Japanese is using "Latin" only, because we are not loading
Source Sans Pro for them, I'm not sure if Source Sans Pro has a separate
implementation for them.

Greek and Cyrilic languages I've tested so far looks good.
I think we should fix the issue for Cyrilic and Greek if possible and find a
Font that support all other languages.

Alternatively we can allow usage of Local fonts and rely on user having right
font in the system.

I suggest that we fix for as much languages as possible with the current review
and find optimal solution separately.

[saroyanm, 2018-02-16 21:02:57]

New patch is uploaded,
ready for the review.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/15 18:47:19, saroyanm wrote:
> On 2018/02/15 12:17:06, Thomas Greiner wrote:
> > On 2018/02/14 18:30:04, saroyanm wrote:
> > > Only thing I could find is from -> https://developers.google.com/fonts/faq

> > > The open source fonts in the Google Fonts catalog are published under
> licenses
> > > that allow you to use them on any website, whether it’s commercial or
> > personal. 
> > > Search queries may surface results from external foundries, who may or may
> not
> > > use open source licenses.
> > > 
> > > Source Sans Pro is "OFL" and links to actual license from here ->
> > > https://fonts.google.com/specimen/Source+Sans+Pro 
> > > 
> > > I'll include the license as a separate file.
> > 
> > Ok. From what I read in the license text, the requirements are:
> > - Include the license text in adblockplusui and in the extension build
> > - Include the copyright notice in adblockplusui and in the extension build
> > 
> > The Google Fonts page doesn't mention though what the copyright notice
should
> > say so I did a quick search and found the font's official site at
> > https://typekit.com/fonts/source-sans. There you find links on the bottom
for
> > each font variant. Those pages contain the license as well as the copyright
> > notice we need to include.
> > 
> > This also matches the license file that I found in the repo where you got
the
> > font files from:
> > https://github.com/adobe-fonts/source-sans-pro/blob/release/LICENSE.txt
> > 
> > We could even include the fonts as an npm package if we want (see
> >
>
https://github.com/adobe-fonts/source-sans-pro/blob/release/README.md#font-in...
> > and https://www.npmjs.com/package/source-sans-pro).
> If we include Font as an npm package, we don't need to include License ?
> 
> > > I agree, mind if I use Import until we have CSS preprocessor in place
> > > https://developer.mozilla.org/en-US/docs/Web/CSS/%40import ?
> > 
> > I think there's a misunderstanding. What I'm suggesting is to include it as
> > follows:
> > 
> > <head>
> >   ...
> >   <link rel="stylesheet" type="text/css" href="fonts.css">
> >   <link rel="stylesheet" type="text/css" href="desktop-options.css">
> >   ...
> > </head>
> Sure, I was referring to use @include rather than manage another file in HTML,
> but if you have a strong preference to current method, I'll go for it.

As discussed, I've included "fonts" file separately as you suggested and
included license.

https://codereview.adblockplus.org/29654555/diff/29683655/skin/desktop-option...
skin/desktop-options.css:26: unicode-range: U+0460-052F, U+1C80-1C88, U+20B4,
U+2DE0-2DFF, U+A640-A69F, U+FE2E-FE2F;
On 2018/02/14 18:30:04, saroyanm wrote:
> On 2018/02/14 17:13:45, Thomas Greiner wrote:
> > Coding style: "Line length: 80 characters or less"
> > 
> > Also applies to other occurrences in this file.
> 
> Will fix this.

Done.

[Thomas Greiner, 2018-02-19 14:45:52]

https://codereview.adblockplus.org/29654555/diff/29699667/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29699667/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
Detail: These rules are already included via fonts.css so let's remove them from
here.

https://codereview.adblockplus.org/29654555/diff/29699667/skin/updates.css
File skin/updates.css (right):

https://codereview.adblockplus.org/29654555/diff/29699667/skin/updates.css#ne...
skin/updates.css:16: */
Good catch. Can't believe we missed that.

https://codereview.adblockplus.org/29654555/diff/29699667/skin/updates.css#ne...
skin/updates.css:175: font-weight: normal;
Detail: Since you've been fixing the "font-weight" values across all CSS files,
mind also replacing this one with `400`? (see also #6400)

[saroyanm, 2018-02-20 15:30:10]

Ready for review.

https://codereview.adblockplus.org/29654555/diff/29699667/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29654555/diff/29699667/skin/desktop-option...
skin/desktop-options.css:18: /* cyrillic-ext */
On 2018/02/19 14:45:51, Thomas Greiner wrote:
> Detail: These rules are already included via fonts.css so let's remove them
from
> here.

oops, done.

https://codereview.adblockplus.org/29654555/diff/29699667/skin/updates.css
File skin/updates.css (right):

https://codereview.adblockplus.org/29654555/diff/29699667/skin/updates.css#ne...
skin/updates.css:175: font-weight: normal;
On 2018/02/19 14:45:51, Thomas Greiner wrote:
> Detail: Since you've been fixing the "font-weight" values across all CSS
files,
> mind also replacing this one with `400`? (see also #6400)

Done.

[Thomas Greiner, 2018-02-21 12:38:24]

Let's also remove the duplicated font files which are no longer being used after
this change:
- skin/fonts/SourceSansPro-Light.woff
- skin/fonts/SourceSansPro-Regular.woff
- skin/fonts/SourceSansPro-bold.woff

Other than that LGTM

[saroyanm, 2018-02-21 12:42:29]

On 2018/02/21 12:38:24, Thomas Greiner wrote:
> Let's also remove the duplicated font files which are no longer being used
after
> this change:
> - skin/fonts/SourceSansPro-Light.woff
> - skin/fonts/SourceSansPro-Regular.woff
> - skin/fonts/SourceSansPro-bold.woff
I think I've removed them, If I don't miss something.
> Other than that LGTM

[Thomas Greiner, 2018-02-21 12:48:39]

On 2018/02/21 12:42:29, saroyanm wrote:
> I think I've removed them, If I don't miss something.

You're right, I missed the "R" next to them. Everything's fine then. :)