qooxdoo / qooxdoo

qooxdoo - Universal JavaScript Framework
http://qooxdoo.org
Other
764 stars 260 forks source link

Unified webfonts #10546

Closed johnspackman closed 1 year ago

johnspackman commented 1 year ago

Fixes https://github.com/qooxdoo/qooxdoo/issues/10494

The tl;dr of this PR is that:

The benefits of all this is faster compiles, with smaller build targets - and no space consumed by icon fonts at all

oetiker commented 1 year ago

can a package provide pre-extracted glyphes ?

oetiker commented 1 year ago

Is this backward compatible in the sense that we can put it into 7.6 ?

johnspackman commented 1 year ago

can a package provide pre-extracted glyphes ?

yes - the glyph files and the source files are expected to in the resources of the library that declares them

Is this backward compatible in the sense that we can put it into 7.6 ?

yes, it should be because the provides.webfonts is not changed. You should get a deprecated warning when the compiler see you use the old provides.webfonts

hkollmann commented 1 year ago

Is it possible to provide a migration class for 7_5_6?

oetiker commented 1 year ago

how about adding support for https://fontsource.org/ ?

johnspackman commented 1 year ago

Is it possible to provide a migration class for 7_5_6?

done

how about adding support for https://fontsource.org/ ?

I've just tried doing this, but i'm not sure how to implement it - eg I tried with their example npm install @fontsource/open-sans.

AIUI the web page would need to add at least ./node_modules/@fontsource/open-sans/index.css, which references 16 font files as CSS url(...)s; there are other variants that the user could choose (eg different weights, decoration, or alphabet). The font files exist in ./node_modules/@fontsource/open-sans/files/, where there are 237 files.

Unless we expect that the web server will serve the entire of node_modules/@fontsource, we would have to copy node_modules/@fontsource into the build target directory, and then we would also have to parse the CSS to change the url(...).

IMHO fontsource.org support is out of scope for this PR - it's not impossible but it sounds like it would be a lot of work and would increase the build size, which AFAICR was an underlying issue this PR was trying to reduce.

oetiker commented 1 year ago

using fontsource with other js frameworks is exceedingly simple.

import "@fontsource/open-sans/400.css";
import "@fontsource/open-sans/700.css";

this somehow causes that all the bits for these two font weights get copied during the build process.

but what goes on behind the scenes ... no idea.

oetiker commented 1 year ago

@johnspackman the tangible theme requires many of the material icons to work ... (https://github.com/johnspackman/qooxdoo/blob/unified-webfonts/source/class/qx/theme/tangible/Image.js) I guess that file needs some adapting ?

johnspackman commented 1 year ago

@johnspackman the tangible theme requires many of the material icons to work ... (https://github.com/johnspackman/qooxdoo/blob/unified-webfonts/source/class/qx/theme/tangible/Image.js) I guess that file needs some adapting ?

oops, done

goldim commented 1 year ago

image

Compiled qxl.widgetbrowser and letter interval, line interval and etc were decreased in size.

johnspackman commented 1 year ago

I don't know why Widget Browser shows different things with this PR but think it should be fixed.

done

goldim commented 1 year ago

Changing from Indigo -> any other theme -> Indigo in WidgetBrowser get next errors:

MProperty.js:67 Uncaught Error: No such property: EPSILON in qx.bom.Font (qx.bom.Font[821-0])
    at wrapper.set (MProperty.js:67:19)
    at wrapper._applyTheme (Font.js:236:26)
    at wrapper.set (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:3:1075)
    at wrapper.eval [as $$setThemeImpl] (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:6:258)
    at wrapper.eval [as setTheme] (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:3:21)
    at wrapper._applyTheme (Meta.js:81:15)
    at wrapper.set (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:3:1118)
    at wrapper.eval [as $$setThemeImpl] (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:4:258)
    at wrapper.eval [as setTheme] (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:3:21)
    at wrapper.<anonymous> (Header.js:74:45)
    at Direct.js:133:37
    at Function._true [as then] (Utils.js:149:22)
    at Direct.js:132:26
    at Array.forEach (<anonymous>)
    at wrapper.dispatchEvent (Direct.js:107:19)
    at wrapper.wrappedFunction [as dispatchEvent] (Interface.js:529:31)
Native.js:56 006077 qx.ui.core.queue.Layout: Error in the 'Layout' queue:TypeError: Cannot read properties of null (reading 'getInsets') TypeError: Cannot read properties of null (reading 'getInsets')
qx.ui.core.Widget:1314:33,qx.ui.core.Widget:1143:27,qx.ui.core.LayoutItem:566:48,qx.ui.core.queue.Layout:225:40,qx.ui.core.queue.Layout:89:26,qx.ui.core.queue.Manager:181:43,qx.ui.core.queue.Manager:233:11,qx.ui.core.queue.Manager:135:14,qx.event.dispatch.Direct:123:39,qx.event.Utils:190:24,qx.event.dispatch.Direct:122:28,Array.forEach (<anonymous>),qx.event.dispatch.Direct:110:21,qx.Interface:514:33,qx.event.Manager:911:52,qx.event.Registration:350:42
Native.js:56 006749 qx.ui.core.queue.Appearance: Error in the 'Appearance' queue:Error: Error in property decorator of class qx.ui.form.ListItem in method setThemedDecorator with incoming value 'selected': Is invalid! Error: Error in property decorator of class qx.ui.form.ListItem in method setThemedDecorator with incoming value 'selected': Is invalid!
qx.core.Property:773:15,qx.core.Property:820:38,qx.core.Property:820:38,qx.ui.core.Widget:2443:86,qx.ui.core.queue.Appearance:104:17,qx.ui.core.queue.Manager:165:47,qx.ui.core.queue.Manager:233:11,qx.ui.core.queue.Manager:135:14,qx.event.dispatch.Direct:123:39,qx.event.Utils:190:24,qx.event.dispatch.Direct:122:28,Array.forEach (<anonymous>),qx.event.dispatch.Direct:110:21,qx.Interface:514:33,qx.event.Manager:911:52,qx.event.Registration:350:42
goldim commented 1 year ago
Uncaught Error: Error in property decorator of class qx.ui.form.ListItem in method setThemedDecorator with incoming value 'selected': Is invalid!
    at Object.error (Property.js:943:13)
    at wrapper.set (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:3:773)
    at wrapper.eval [as setThemedDecorator] (eval at __installFunctionFromCode__P_57_13 (Property.js:1001:36), <anonymous>:6:258)
    at wrapper.syncAppearance (Widget.js:2463:15)
    at wrapper.removeState (Widget.js:2300:14)
    at wrapper._onPointerOut (ListItem.js:99:12)
    at EventHandler.js:298:35
    at Function._true [as series] (Utils.js:318:24)
    at EventHandler.js:296:31
    at Function._true [as then] (Utils.js:149:22)
    at wrapper._dispatchEvent (EventHandler.js:295:22)
    at AbstractBubbling.js:260:39
    at Function._true [as series] (Utils.js:318:24)
    at AbstractBubbling.js:228:39
    at Function._true [as series] (Utils.js:318:24)
    at AbstractBubbling.js:225:31
Native.js:56 006788 qx.ui.core.queue.Appearance: Error in the 'Appearance' queue:Error: Error in property decorator of class qx.ui.form.ListItem in method setThemedDecorator with incoming value 'selected': Is invalid! Error: Error in property decorator of class qx.ui.form.ListItem in method setThemedDecorator with incoming value 'selected': Is invalid!
qx.core.Property:773:15,qx.core.Property:820:38,qx.core.Property:820:38,qx.ui.core.Widget:2443:86,qx.ui.core.queue.Appearance:104:17,qx.ui.core.queue.Manager:165:47,qx.ui.core.queue.Manager:233:11,qx.ui.core.queue.Manager:135:14,qx.event.dispatch.Direct:123:39,qx.event.Utils:190:24,qx.event.dispatch.Direct:122:28,Array.forEach (<anonymous>),qx.event.dispatch.Direct:110:21,qx.Interface:514:33,qx.event.Manager:911:52,qx.event.Registration:350:42
goldim commented 1 year ago

image

Get ... even in fullscreen mode Changing to TangiableLight: image

And text like cut at the bottom

johnspackman commented 1 year ago

Changing from Indigo -> any other theme -> Indigo in WidgetBrowser get next errors:

fixed

Uncaught Error: Error in property decorator of class qx.ui.form.ListItem in method setThemedDecorator with incoming value 'selected': Is invalid!

I cant reproduce this but it looks like it would be a bug in a theme, ie missing decorator

Get ... even in fullscreen mode

fixed

goldim commented 1 year ago

Checked on 3 apps everything works fine,

hkollmann commented 1 year ago

@derrell could you approve?

derrell commented 1 year ago

I've already approved this. It's @level420 who this one's waiting on.

hkollmann commented 1 year ago

What's with your change request?

goldim commented 1 year ago

Also text of Widget Browser on site qooxdoo (online version) is wider than in Widget Browser which uses PR compiler.

hkollmann commented 1 year ago

Also text of Widget Browser on site qooxdoo (online version) is wider than in Widget Browser which uses PR compiler.

Could this be a ignorable rounding issue? The difference is marginal and all pixel based text are working.

hkollmann commented 1 year ago

@goldim : Could you agree?

goldim commented 1 year ago

@hkollmann tbh I dunno. I would like to know what @johnspackman thinks about it. I am not against this PR would be merged if It will be fixed before new release. But if doesnt I predict some user complains about it.

goldim commented 1 year ago

Especially as It was with hurried removal of MaterialIcons.

oetiker commented 1 year ago

Also text of Widget Browser on site qooxdoo (online version) is wider than in Widget Browser which uses PR compiler.

Could this be a ignorable rounding issue? The difference is marginal and all pixel based text are working.

The bottom portion of the lower case g gets cut off, I don't think this is acceptable. I am pretty sure this indicates a bug somewhere. In any case, if I was going to ship an application where some the letters of the font get cut off, this would not pass muster …

johnspackman commented 1 year ago

it's a bug, ill take a look now

goldim commented 1 year ago

@johnspackman the issue was gone but still in indigo theme text looks like compressed horizontally. Something like forgotten for Indigo theme. Can you see it? I could send screen record showing this problem. And also in tangiable themes the text became bigger by 2px.

oetiker commented 1 year ago

@johnspackman the issue was gone but still in indigo theme text looks like compressed horizontally. Something like forgotten for Indigo theme. Can you see it? I could send screen record showing this problem. And also in tangiable themes the text became bigger by 2px.

could it be that there is some confusion px vs pt ?

johnspackman commented 1 year ago

The bottom portion of the lower case g gets cut off, I don't think this is acceptable.

fixed

Especially as It was with hurried removal of MaterialIcons.

nothing's been removed, this PR is about making the loading order/mechanism much finer grained; the font in use is actually Roboto

the issue was gone but still in indigo theme text looks like compressed horizontally. Something like forgotten for Indigo theme. Can you see it? I could send screen record showing this problem.

I've checked the button fonts are they are definitely the same size in indigo on mine - yes please for a screenshot

And also in tangiable themes the text became bigger by 2px.

I definitely can't see this, when I get it the font sizes are the same - but I have overlaid a couple of screenshots on top of each other and can see that there are some differences in layout, and I seen another group title with ellipsis too :(

goldim commented 1 year ago

https://user-images.githubusercontent.com/22189134/221616422-e4b9b3c2-0984-4c39-a209-4e27e9ca4eab.mp4

johnspackman commented 1 year ago

hopefully this is the last fix; there is still a difference between the widget browser produced by this version compared to the old version, BUT it only happens if we use the Roboto fonts locally and not via a CDN (ie this affects Tangible theme).

AIUI there may be a change in the font definition, but also the CDN delivers multiple fonts (eg same fontFamily but different fontWeight and fontStyle) in a single CSS - this is the reason why there is such a change in the last push, it's because the previous loader was assuming that only a single font in a single family could be loaded.

johnspackman commented 1 year ago

PS hang on before merging please I just remembered I need to update the documentation! :(

johnspackman commented 1 year ago

docs are done now

goldim commented 1 year ago

@johnspackman the "problem" still exists. I combared fonts in this PR and what fonts now in qooxdoo online. The problem happens bc font family for Indigo theme was changed now it is : "Lucida Grande", Tahoma", Verdana", Bitstream Vera Sans" "Liberation Sans" and before: "Lucida Grande", "DejaVu Sans", "Verdana", "sans-serif". If I change new font family to previous the problem is gone. If you changed the font family on purpose I approve the PR. Same about Tangiable themes: newest version lacks sans-serif in font family.

goldim commented 1 year ago

@johnspackman sorry, just have read your comment above

johnspackman commented 1 year ago

@goldim oops, must have copied & pasted! fixed the fonts for indigo now

hkollmann commented 1 year ago

@johnspackman: tests are not working any longer?

johnspackman commented 1 year ago

there is one test which is still failing - but I can't reproduce that on my Mac with Chrome, Firefox, or Safari, or on Fedora 36 with Chromium or Firefox.

Before I install yet another linux VM, does anyone have a Ubuntu VM handy they can test this with?

goldim commented 1 year ago

@johnspackman I could run tests on Debian locally.

johnspackman commented 1 year ago

Yes please; not sure what else to try except reproduce the GitHub environment exactly :(

goldim commented 1 year ago

It's pity but sometimes Debian behavior differs from Ubuntu