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

textinput: placeholder changes component width #358

Open
seanbanko opened this issue Mar 23, 2023 · 5 comments · May be fixed by #484
Open

textinput: placeholder changes component width #358

seanbanko opened this issue Mar 23, 2023 · 5 comments · May be fixed by #484

Comments

@seanbanko
Copy link
Contributor

I'm seeing strange behavior using a Placeholder with the textinput bubble. Here is a minimum reproducible example:

package main

import (
	"os"

	"github.com/charmbracelet/bubbles/textinput"
	tea "github.com/charmbracelet/bubbletea"
	"github.com/charmbracelet/lipgloss"
)

type model struct {
	input textinput.Model
}

func main() {
	m := newModel()
	p := tea.NewProgram(m)
	if _, err := p.Run(); err != nil {
		os.Exit(1)
	}
}

func newModel() model {
	input := textinput.New()
	input.Prompt = " "
	input.Width = 5
	input.Placeholder = "123"
	input.Focus()
	return model{input: input}
}

func (m model) Init() tea.Cmd {
	return nil
}

func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
	switch msg := msg.(type) {
	case tea.KeyMsg:
		switch msg.String() {
		case "ctrl+c":
			return m, tea.Quit
		}
	}
	var cmd tea.Cmd
	m.input, cmd = m.input.Update(msg)
	return m, cmd
}

func (m model) View() string {
	return lipgloss.NewStyle().Border(lipgloss.RoundedBorder()).Render(m.input.View())
}

Changing the length of the Placeholder string changes the way that the textinput is rendered before it receives the first character, triggering a resize after the first character is received. See examples with various Placeholders.

Placeholder = "123"
demo-1

Placeholder = "12345"
demo-2

Placeholder = "1234567890"
demo-3

Placeholder = "12345 " (with trailing space)
demo-4

no Placeholder
demo-5

So to avoid a resize it seems there needs to either be no Placeholder at all, or the length of the Placeholder must be equal to Width + 1.

@residualmind
Copy link
Contributor

residualmind commented Jul 29, 2023

I just looked into fixing this - and it is a simple fix to basically cut the placeholder off, so that the width never changes. But the current behavior could have benefits: You can have input fields that are narrow to not take up a lot of space but when you focus them, you can set a longer placeholder to describe the field - and unset that placeholder again when you unfocus... So people might depend on this behavior?

This would be a very simple possible patch: (Not making a PR because I'm really not sure what the desired behavior is. But I'd happy to create a PR.)

diff --git a/textinput/textinput.go b/textinput/textinput.go
index e7bd08b..2d863c0 100644
--- a/textinput/textinput.go
+++ b/textinput/textinput.go
@@ -651,8 +651,8 @@ func (m Model) placeholderView() string {
    m.Cursor.SetChar(p[:1])
    v += m.Cursor.View()
 
-   // The rest of the placeholder text
-   v += style(p[1:])
+   // The rest of the placeholder text, cut off to fit the width
+   v += style(p[1:m.Width] + " ")
 
    return m.PromptStyle.Render(m.Prompt) + v
 }

@Static-Flow
Copy link

Took me a long time to find the issue that was causing this bug. Personally, the behavior I would expect is that if a placeholder string is smaller than the defined width of the component, it shouldn't shrink the component width.

@seanbanko
Copy link
Contributor Author

@residualmind nice find. Personally, I agree with @Static-Flow that if a placeholder is smaller than the text input, it shouldn't shrink the text input. That seems counter to the notion of setting a width on the text input in the first place, and the current behavior feels pretty inconsistent with, well, most other text inputs I've come across. Would you mind making a PR (or I'd be glad to make one with your change) and maybe someone from the core team can merge or decline if they feel that the current behavior is the desired behavior?

@meowgorithm
Copy link
Member

Hey everyone. So if I understand this correctly the thought is that the placeholder should get truncated if the width of the textinput is narrower than the width of the placeholder, correct?

If so, that's definitely the desired behavior; please do make a PR. Use lipgloss.Width to measure widths.

@mikelorant
Copy link
Contributor

mikelorant commented Feb 3, 2024

Is this issue actually resolved now? Seems that the fix was merged.

I've run up the test app in the original message and it seems to be working correctly now.

@meowgorithm You should be able to close this one.

JaredReisinger added a commit to JaredReisinger/bubbles that referenced this issue Feb 28, 2024
I noticed that the cursor seemed to be adding an extra space which the
width calculations weren't accounting for. This also led me to discover
an edge case with the view (`offset`/`offsetRight`) calculation when the
cursor (`pos`) sits at the very end. Additionally, `View()` was always
rendering the cursor, even when `focus` was false, which the comment
suggests should not be the case.

These should all be resolved, such that Width is now fully respected in
both `View()` and `placeholderView()`: they should _never_ render a
string that exceeds the requested width. (Well... with one caveat: I did
not touch the suggestion logic, as I've never used it. I did leave a
comment, however, that perhaps suggestions shouldn't even render when
the component does not have focus.)

Fixes charmbracelet#358, charmbracelet#307
@JaredReisinger JaredReisinger linked a pull request Feb 28, 2024 that will close this issue
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 a pull request may close this issue.

5 participants