Closed Bug 1547673 Opened 5 years ago Closed 5 years ago

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


(Firefox :: Address Bar, enhancement, P2)




Firefox 68
69.1 - May 13 - 26
Tracking Status
firefox68 --- fixed


(Reporter: mikedeboer, Assigned: adw)




(3 files)

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


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

Attached video

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

Created attachment 9065105 [details] [diff] [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.

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?

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?

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
Pushed by
Quantumbar: For empty @token searches, show only the "Search with" text in the first result, not also the token. r=dao
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.