Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

text: change line ascent and descent to fit font size #123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

morlay
Copy link

@morlay morlay commented Sep 14, 2023

With this patch, the example will render correct like

image

with v0.3.0,same font size and line height not stacked.

image

For widget.Editor,
baseline will change, when CJK char input after nonCJK ones.
This patch could fix this too.

Example:

package main

import (
	"image/color"
	"log"
	"os"

	"gioui.org/app"
	"gioui.org/font"
	"gioui.org/io/system"
	"gioui.org/layout"
	"gioui.org/op"
	"gioui.org/op/paint"
	"gioui.org/text"
	"gioui.org/unit"
	"gioui.org/widget"
	"gioui.org/widget/material"
	"gioui.org/x/richtext"

	"gioui.org/font/gofont"
)

var textSize = unit.Sp(100)
var lineHeight = textSize * 3

func main() {
	th := NewTheme(gofont.Collection())

	ui := UI{
		Theme:  th,
		Window: app.NewWindow(app.Size(1000, unit.Dp(lineHeight))),
	}

	go func() {
		if err := ui.Loop(); err != nil {
			log.Fatal(err)
		}
		os.Exit(0)
	}()
	app.Main()
}

type (
	C = layout.Context
	D = layout.Dimensions
)

// UI specifies the user interface.
type UI struct {
	Theme  *Theme
	Window *app.Window
	Editor *widget.Editor
}

// Theme contains semantic style data.
type Theme struct {
	// Base theme to extend.
	Base *material.Theme
	// cache of processed markdown.
	cache []richtext.SpanStyle
}

func NewTheme(font []font.FontFace) *Theme {
	th := material.NewTheme()
	th.Shaper = text.NewShaper(text.WithCollection(font))
	return &Theme{
		Base: th,
	}
}

func (ui UI) Loop() error {
	var ops op.Ops
	for e := range ui.Window.Events() {
		switch e := e.(type) {
		case system.DestroyEvent:
			return e.Err
		case system.FrameEvent:
			gtx := layout.NewContext(&ops, e)
			ui.Layout(gtx)
			e.Frame(gtx.Ops)
		}
	}
	return nil
}

func (ui *UI) Layout(gtx C) D {
	if ui.Editor == nil {
		ui.Editor = &widget.Editor{}
	}

	ui.Theme.Base.TextSize = textSize

	ui.Editor.LineHeight = lineHeight
	ui.Editor.LineHeightScale = 1
	ui.Editor.SingleLine = true

	return layout.Stack{}.Layout(gtx,
		layout.Stacked(func(gtx layout.Context) layout.Dimensions {
			l := widget.Label{}

			l.LineHeight = ui.Editor.LineHeight
			l.LineHeightScale = ui.Editor.LineHeightScale

			m := op.Record(gtx.Ops)
			paint.ColorOp{Color: color.NRGBA{R: 0xff, A: 0x30}}.Add(gtx.Ops)
			call := m.Stop()

			return l.Layout(
				gtx,
				ui.Theme.Base.Shaper,
				font.Font{
					Typeface: ui.Theme.Base.Face,
				},
				ui.Theme.Base.TextSize,
				"SayHi 你好",
				call,
			)
		}),
		layout.Stacked(func(gtx layout.Context) layout.Dimensions {
			l := widget.Label{}

			l.LineHeight = ui.Editor.LineHeight
			l.LineHeightScale = ui.Editor.LineHeightScale

			m := op.Record(gtx.Ops)
			paint.ColorOp{Color: color.NRGBA{B: 0xff, A: 0x30}}.Add(gtx.Ops)
			call := m.Stop()

			return l.Layout(
				gtx,
				ui.Theme.Base.Shaper,
				font.Font{
					Typeface: ui.Theme.Base.Face,
				},
				ui.Theme.Base.TextSize,
				"SayHi",
				call,
			)
		}),
		layout.Expanded(func(gtx layout.Context) layout.Dimensions {
			m := op.Record(gtx.Ops)
			paint.ColorOp{Color: color.NRGBA{G: 0xff, A: 0x30}}.Add(gtx.Ops)
			call := m.Stop()

			if ui.Editor.Text() == "" {
				ui.Editor.SetText("Sa")
			}

			dims := ui.Editor.Layout(
				gtx,
				ui.Theme.Base.Shaper,
				font.Font{
					Typeface: ui.Theme.Base.Face,
				},
				ui.Theme.Base.TextSize,
				call,
				op.CallOp{},
			)

			return dims
		}),
	)
}

@eliasnaur
Copy link
Contributor

Please follow the Go style for changes, and sign off your commits. Example:

text: change line ascent and descent to ...

Also, can you add a test so this behaviour doesn't regress?

I'll let @whereswaldon decide on the change itself.

@morlay morlay changed the title Fix: ascent and descent should fit the font size and leading around line to make line height work well for first line text: change line ascent and descent to fit font size Sep 14, 2023
@morlay
Copy link
Author

morlay commented Sep 14, 2023

@eliasnaur Sure. Updated.

And I have drop the leading around line, like line-height of css did.
It is another feature: https://www.w3.org/TR/css-inline-3/#text-edges

Copy link
Member

@whereswaldon whereswaldon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch and test. I've tried to carefully review this change and its implications, and I'm not certain that we want to go this direction.

This change will force a run shaped with size X to have an effective bounds height of X even if the font for that run displays glyphs larger than or smaller than X. We lose information provided by the font designer here by overriding their preferences.

Additionally, even if every run of text on a line is the same size (not guaranteed), I believe this change will not guarantee that the line as a whole has an ascent and descent that sum to X. If varying runs of text have differently-distributed ascents and descents with respect to the baseline, we may take the branch on line 905 for one run, and the branch on 908 for a different run, resulting in the most extreme ascent and descent available on the line being set as the line's overall ascent/descent. This is also the current behavior, but I think is something that the changeset wanted to alter.

When I sanity-check this behavior against CSS, it seems that CSS also does not normalize baseline positions.

image

I believe that the correct way to achieve what you wanted is to align the stacked text by their respective baselines. Sadly, layout.Stack does not have a feature for that, but we can at least demonstrate the idea with layout.Flex.

image

You could write a layout type that allows stacking content aligned by baseline by borrowing relevant portions of code from layout.Flex and layout.Stack.

In the general case, I don't think text is safe to align purely by matching the upper-left corner of the bounding box in many GUI software toolkits, so I'm reluctant to make this change within Gio.

@morlay What do you think? Am I missing some important context or use-cases?

@morlay
Copy link
Author

morlay commented Sep 29, 2023

@whereswaldon

The biggest problem isn't the layout, it's in the text input.

Chars is dancing when text mixed inputing, because of this problem.

@morlay
Copy link
Author

morlay commented Sep 29, 2023

For css, it could fixed by line height

https://codepen.io/morlay/pen/jOXKKrw

But css line height model is different.
Which gio is not used.

https://iamvdo.me/en/blog/css-font-metrics-line-height-and-vertical-align

So I choose the resizing to fix.

@whereswaldon whereswaldon force-pushed the main branch 4 times, most recently from 0d543a0 to 1686874 Compare October 7, 2023 00:14
@morlay
Copy link
Author

morlay commented Oct 8, 2023

@whereswaldon

How do you think?
Should we normalize the baseline? or implement line-height like css?

@whereswaldon
Copy link
Member

@morlay Thank you for your patience.

Gio's line height model currently defines the distance between baselines, unlike CSS. This has the consequence of not defining the distance between the first baseline and the logical top of the text. To get a logical top, we're currently using the highest ascender value of any run on the first line. Since varying fonts have varying ascender heights, we get the jumping as we switch between fonts that you describe within the editor.

None of Gio's specific text metric behaviors are set in stone. My goal in choosing them has always been to try to learn from other text stacks and choose the (seemingly) best model I could find. Our line height has both LineHeight (matching the semantics of a pixel quantity in CSS) and LineHeightScale (matching the semantics of a unitless quantity in CSS) in order to provide flexibility. I found good arguments for using both versions of the CSS semantic, so I implemented mechanisms to allow the behavior of both. This article was a major source of inspiration, but I didn't copy everything.

Should we normalize the baseline? or implement line-height like css?

I don't love normalizing the baseline because it discards font information, so I think I'd prefer to explore making our line height definition solve this problem. Can you elaborate on what you would like to change about the line height semantics in order to address this?

@morlay
Copy link
Author

morlay commented Oct 25, 2023

@whereswaldon

May consider to support this feature

https://github.com/jantimon/text-box-trim-examples

I think we could define the baseline easier.
Even implement line height like css did.

@SchnWalter
Copy link
Contributor

SchnWalter commented Oct 27, 2023

@morlay, what version of gioui.org/font/gofont are you using?

EDIT: Actually, the font is from golang.org/x/image. what version do you have in your sum file?

EDIT2: With text.NoSystemFonts(), the extra glyphs are rendered as squares, so it makes sense why the line height changes, with your patch, you are changing gio from using line spacing (under the wrong terminology), to line height, but your patch is incomplete for the change.

@whereswaldon
Copy link
Member

whereswaldon commented Oct 31, 2023

The status of this PR, from my point of view, is:

  1. We do have a problem with jumping text during text editing when font fallback causes the use of a font with different metrics.
  2. We do not have a straightforward plan to address this shortcoming within Gio's text stack.

Though I'm learning, I'm by no means a text layout expert, especially in the spacing/alignment area. I'm open to input on how we should evolve our model to address this.

Thus far, I think I've heard a few proposed solutions:

  • Normalizing line height to always match font size: I think this destroys font-specific layout info, so I'm reluctant to implement it.
  • Implement CSS's line height model: The CSS model is pretty complex. If we must, we can go this route, but I wonder whether we can adopt a simplified and modern subset of the CSS approach. I would appreciate any concrete advice from someone with more expertise in the CSS model on this.
  • Implement text box trim: This is doable, but I don't see exactly how it would help. Even if we trimmed the text box of an editor when typing latin characters, adding taller CJK characters would make the text box height increase, would it not?

@morlay
Copy link
Author

morlay commented Nov 6, 2023

  • Implement text box trim: This is doable, but I don't see exactly how it would help. Even if we trimmed the text box of an editor when typing latin characters, adding taller CJK characters would make the text box height increase, would it not?

It is to resolve line-height issue of css. if we don't want to implement line-height like css. I think we could ignore it.

  • Normalizing line height to always match font size: I think this destroys font-specific layout info, so I'm reluctant to implement it.

Yes. But we could do normalizing for align the baseline only (fix offset-y).

@whereswaldon whereswaldon force-pushed the main branch 9 times, most recently from 3d36537 to 74ccc9c Compare June 27, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants