1547673 - Fix text for empty @tokens -- “@google -- Search with Google” vs. “Search with Google”

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P2)

Description

[Mike de Boer [:mikedeboer]]

For more info, see the design doc over at https://docs.google.com/document/d/11c-OTsscmArnqL02Kq1TUPKtkHl4avmC3pDdSJDC6JI/edit

Comment 1

[Marco Bonardo [:mak]]

I think we decided to directly do this in the product, right?

Comment 2

[Drew Willcoxon :adw]

Yes

Comment 3

[Drew Willcoxon :adw]

When the title is empty, the row's height shrinks slightly -- by 1px, I think, at least here on macOS. It looks like that's because row height isn't defined per se anywhere but instead depends on padding between the inner edges of the row and its content. Icons are 16px tall and the title (on macOS at least) is 17px tall, which explains the 1px change I'm seeing when the title doesn't have any content.

I'm probably going to ask you for review on this, so how would you like to fix it? Some ways I can think of:

(1) Hardcode a row height or min-height somewhere.

(2) Something like .urlbarView-title:empty { height: 17px; } or just .urlbarView-title { line-height: 17px; }. Simple, but we'd possibly need different rules per platform, assuming that the title line heights may vary across platforms. This also doesn't seem very robust in the face of users with non-standard font faces/sizes.

(3) Instead of leaving the title empty, put a zero-width space (U+200B) in it. I like this because it just works, but it seems a little hacky. Also, we'd need to come up with another way of hiding the em dash, like setting a class/attribute somewhere to indicate that the title is logically empty.

Comment 4

[Marco Bonardo [:mak]]

Just remember, in the future we may have rows with different heights.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #3)

Why is the title special here in the first place? It's a box with text just like the secondary / url box and should behave similarly. Does removing these lines (which I think bug 1534738 obsoleted) fix the inconsistency?

Comment 6

[Drew Willcoxon :adw]

Attachment: patch

No, removing those lines doesn't help. The title is the tallest thing in the row. When there's no title, the next tallest thing is the icon. There's a 1px difference between the two.

I'll attach the patch so you can see, assuming it happens the same way on your platform (I think you use Linux?). I'll also attach a video.

Comment 7

[Drew Willcoxon :adw]

Attachment: recording.mov

Comment 8

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #6)

Created attachment 9065105 [details] [diff] [review]
patch

No, removing those lines doesn't help. The title is the tallest thing in the row. When there's no title, the next tallest thing is the icon. There's a 1px difference between the two.

If there's no title, there's still the secondarily text, right? (In this case "Search with Google", I assume.) Why does that not have the same height?

Comment 9

[Drew Willcoxon :adw]

They have different line heights. The title (and title separator) line height is 17px, but the secondary (and tags) line height is 15px.

Comment 10

[Dão Gottwald [:dao]]

Ah, that's because we reduce the font size for the secondary text. Do we actually want that here?

Comment 11

[Drew Willcoxon :adw]

I assume so, it's been like that for a long time now... Assuming we don't want to change that, how should we fix this?

Comment 12

[Dão Gottwald [:dao]]

I mean the notion of a secondary text doesn't really make sense when that's the only text.. As we're removing the primary text here, maybe the formerly-secondary text should take its place?

Regardless I can try to put something together to ensure both occupy the same height. (Neither of your proposals seems ideal, with (3) being the most robust although it feels a little dirty as you said.)

Comment 13

[Dão Gottwald [:dao]]

I didn't get very far... I could make the heights match using a relative line-height that works for different OSes and settings, but there are vertical alignment issues that seem hard or impossible to solve in a robust way as text is always centered within the box derived from its line-height.

Comment 14

[Drew Willcoxon :adw]

Attachment: Bug 1547673 - Quantumbar: For empty @token searches, show only the "Search with" text in the first result, not also the token.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef90ec1b75ea
Quantumbar: For empty @token searches, show only the "Search with" text in the first result, not also the token. r=dao

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ef90ec1b75ea