Fix text for empty @tokens -- “@google -- Search with Google” vs. “Search with Google”
Categories
(Firefox :: Address Bar, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: mikedeboer, Assigned: adw)
References
Details
Attachments
(3 files)
For more info, see the design doc over at https://docs.google.com/document/d/11c-OTsscmArnqL02Kq1TUPKtkHl4avmC3pDdSJDC6JI/edit
Comment 1•5 years ago
|
||
I think we decided to directly do this in the product, right?
Assignee | ||
Comment 2•5 years ago
|
||
Yes
Assignee | ||
Comment 3•5 years ago
|
||
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•5 years ago
|
||
Just remember, in the future we may have rows with different heights.
Comment 5•5 years ago
|
||
(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?
Assignee | ||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
Comment 8•5 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #6)
Created attachment 9065105 [details] [diff] [review]
patchNo, 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•5 years ago
|
||
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•5 years ago
|
||
Ah, that's because we reduce the font size for the secondary text. Do we actually want that here?
Assignee | ||
Comment 11•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
Comment 15•5 years ago
|
||
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
Comment 16•5 years ago
|
||
bugherder |
Description
•