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

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 months ago
Last month

People

(Reporter: mikedeboer, Assigned: adw)

Tracking

(Blocks 1 bug)

unspecified
Firefox 68
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(3 attachments)

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

Assignee

Comment 2

Last month

Yes

Assignee

Comment 3

Last month

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.

Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 69.1 - May 13 - 26
Flags: needinfo?(dao+bmo)

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

(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?

Flags: needinfo?(dao+bmo)
Assignee

Comment 6

Last month
Posted patch patchSplinter Review

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.

Assignee

Comment 7

Last month
Posted video recording.mov

(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?

Assignee

Comment 9

Last month

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

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

Assignee

Comment 11

Last month

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?

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.)

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.

Depends on: 1552130
Type: task → enhancement

Comment 15

Last month
Pushed by dwillcoxon@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.