Menu item text gets hidden if it is too long

[matejcik]

Describe the bug
problem:

after a local hacky fix:

offending code:

trezor-firmware/core/embed/rust/src/ui/model_mercury/component/button.rs

Lines 624 to 635 in 7005129

	if area.width() > (Self::ICON_SPACE + Self::TEXT_MARGIN + width) {
	//display both icon and text
	text_pos = Point::new(area.top_left().x + Self::ICON_SPACE, text_pos.y);
	use_text = true;
	use_icon = true;
	} else if area.width() > (width + Self::TEXT_MARGIN) {
	use_text = true;
	} else {
	//if we can't fit the text, retreat to centering the icon
	icon_pos = area.center();
	use_icon = true;
	}

Why do we even have this? ISTM we should default to showing both icon and text in all cases.

To reproduce

trezorctl btc get-address -n m/44h/0h/0h/0/0 -d
then click the menu

[matejcik]

UI tests pass with the local hack, which just unconditionally runs the first branch in the above code. This tells me that the other branches are dead code.

[obrusvit]

Why do we even have this? ISTM we should default to showing both icon and text in all cases.

Taken from model_t UI.

UI tests pass with the local hack, which just unconditionally runs the first branch in the above code. This tells me that the other branches are dead code.

Or rather the menu is not walked-through in UI tests?

[ibz]

Indeed, not all menus are tested.

But what is worse? I don't see a case for a menu item with no text - a menu item with incomplete text is certainly better.

So, I see 3 issues here:

1. Is there a case against showing the text all the time? If not, maybe we should bite the bullet and just do it?
2. What should we do if we detect that text does not fit? (can we even detect that - easily - in code?) I would argue that - if possible - we should probably just somehow crash (only in testing / emulator?) - that would make the CI pipeline fail, and would make us shorten the strings. Probably a pain, but we would do it once and know that it is done.
3. Increasing test coverage is something we should be constantly striving for, but unrelated to this issue - so that should not hold us back from actually fixing this issue which is quite bad.

[matejcik]

Is there a case against showing the text all the time? If not, maybe we should bite the bullet and just do it?

agreed, or @mmilata @obrusvit do you know of a case where we want to render icon without text?

we should probably just somehow crash (only in testing / emulator?) - that would make the CI pipeline fail, and would make us shorten the strings. Probably a pain, but we would do it once and know that it is done.

we can crash in debug mode, if detecting this is easy at run-time. i'm not sure about that, @obrusvit @mmilata can you advise?

[mmilata]

do you know of a case where we want to render icon without text?

nope

we can crash in debug mode, if detecting this is easy at run-time

Seems to be easy in this particular case since we already have the widths in variables? For general case we'd probably have to extend the text renderer. Or perhaps graphics driver to detect drawing off-screen, but that sounds like it will have its own problems.

[ibz]

Seems to be easy in this particular case since we already have the widths in variables?

Right, it was a silly question from my side to start with. That's what the if in question does - detect when the text does not fit and hide the text. So it knows it does not fit.

[ibz]

On the other hand, I am thinking what is the gain of crashing if the menus are not walked-through by the tests / CI anyway.

There is a very slim chance somebody will manually get to that point while testing, and if they do, they will still notice the menu looks bad, no? Crashing at that point will just annoy somebody that is trying to do something unrelated.

So maybe the best way is to simply show the incomplete text?

[Hannsek]

So maybe the best way is to simply show the incomplete text?

Most probably, as on every other places.

[matejcik]

On the other hand, I am thinking what is the gain of crashing if the menus are not walked-through by the tests / CI anyway.

a lot of tests actually do involve menus

the point of this is to crash if a translation causes a menu item to not render; we don't really check all languages screen by screen, so the crash will indicate that some translation string doesn't fit and we need to change it

[ibz]

a lot of tests actually do involve menus

Yes, but then shouldn't you have seen some UI tests diffs when you disabled the other branches?

[matejcik]

Yes, but then shouldn't you have seen some UI tests diffs when you disabled the other branches?

i didn't run all the languages

[ibz]

OK, I'll do that then and see what happens. The way it is now is very bad, anyway.

[obrusvit]

Alternative is implementing multi-line texts. As was also desired by Figma designs. See e.g.

I think the combination of approaches would be the best if feasible:

1. try to fit on one line
2. if fails, try to fit on two lines
3. if fails, crash in debug, show incomplete text in prod

[Hannsek]

@lapohoda I think we can do the multiline items, right?