Closed Bug 1106054 Opened 5 years ago Closed 5 years ago

Search dropdown should have a minimum width

Categories

(Firefox :: Search, defect)

36 Branch
x86
All
defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: phlsa, Assigned: Felipe)

References

Details

Attachments

(1 file, 1 obsolete file)

When the search bar is very narrow, the dropdown also becomes narrow, making suggestions unreadable and the one-off buttons harder to target.

The dropdown should have a minimum width so that at least 4 one-off icons fit into a row.
(In reply to Philipp Sackl [:phlsa] from comment #0)

> The dropdown should have a minimum width so that at least 4 one-off icons
> fit into a row.

Out of curiosity, is there a reason for the value "4"? (If I had to pick one myself I would have picked 3, because that's the minimum size where the panel is still fully usable, and that may already take a lot of space on a small netbook screen).
A reason to have more than 3: the options are truncated.

In my Firefox with 3 icons, I see:
- "Search for 1... with:" (only one character from my search string)
- "Change Search Settin"
(In reply to Dan Wolff from comment #2)
> A reason to have more than 3: the options are truncated.
> 
> In my Firefox with 3 icons, I see:
> - "Search for 1... with:" (only one character from my search string)
> - "Change Search Settin"

This depends on the font size you have on your system.
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> This depends on the font size you have on your system.

Okay. I'm using the default Ubuntu font size. I'll rephrase. I think that:
- "Search for <term> with:" should show at least 5 normal sized characters from the search term, e.g. "xxxxx..."
- The complete text "Change Search Settings" should be displayed
Flags: firefox-backlog+
Points: --- → 1
IMO, the minimum size should be the largest (Math.max) out of:

- the number of pixels required for the icons to fit
- the number of ems required for the relevant text (Change Search Settings) to fit. (see also: l10n)

so as to avoid issues on e.g. Ubuntu's default text size (which is unfortunately pretty huge, 15px or so, compared to 11/12 pixels on OS X / Windows).
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Flags: qe-verify?
Attached patch Patch (obsolete) — Splinter Review
Keeping it simple for now, without trying to handle bug 1111638
Attachment #8538596 - Flags: review?(florian)
Comment on attachment 8538596 [details] [diff] [review]
Patch

Review of attachment 8538596 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1127,5 @@
> +
> +        let panel = document.getElementById("PopupSearchAutoComplete");
> +        let minWidth = parseInt(panel.width) + 23;
> +        // Ensure the panel can fit at least 3 engines, unless there
> +        // aren't 3 engines to show.

Is there a reason why the "unless there aren't 3 engines to show." part matters? If we've got less than 147px, I don't think the cropped display of suggestions will be of much use.
Attached patch Patch v2Splinter Review
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Is there a reason why the "unless there aren't 3 engines to show." part
> matters? If we've got less than 147px, I don't think the cropped display of
> suggestions will be of much use.

I thought it made sense, but thinking about it more, the bug is not "Show a minimum number of engines", but rather "the panel should have a minimum width". So I removed that.
And included the comment about the 23px as talked on irc
Attachment #8538596 - Attachment is obsolete: true
Attachment #8538596 - Flags: review?(florian)
Attachment #8538656 - Flags: review?(florian)
Comment on attachment 8538656 [details] [diff] [review]
Patch v2

Thanks!
Attachment #8538656 - Flags: review?(florian) → review+
Comment on attachment 8538656 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: new search UI
[User impact if declined]: users who have a small search bar might see a too narrow search panel
[Describe test coverage new/current, TBPL]: :S
[Risks and why]: small risk, there's already code to calculate the min-width for the panel, now it has a lower bound
[String/UUID change made/needed]: none
Attachment #8538656 - Flags: approval-mozilla-aurora?
Comment on attachment 8538656 [details] [diff] [review]
Patch v2

Approval Request Comment
Also requesting beta approval as I think phlsa expected us to fix this for 35.
The patch looks larger than it actually is due to moving around some comments. All it actually does is really:
const ENGINE_WIDTH = 49;
minWidth = Math.max(minWidth, ENGINE_WIDTH * 3);
Attachment #8538656 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/7ec919dc3789
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Attachment #8538656 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8538656 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've verified this on Windows 7 x64, Mac OS X 10.9.5 and Ubuntu 14.04 x64, using Firefox 35 Beta 6 (BuildID=20141222200458) and latest Firefox 37 Nightly (BuildID=20141222030202). I've tested this by reducing the Firefox window to its minimum width that still shows the Search bar (note that the Search bar can still show at minimum/smaller width for larger width of the browser window... it's just easier to test this way). 

The test results are mixed:
- Windows - the dropdown fits just 2 search engines per row, but still fits the suggestions and other labels fairly well
- Mac - the dropdown fits 3 search engines per row, and fits the suggestions and other labels fairly well
- Ubuntu - the dropdown fits 3 search engines per row, fits a relatively ok part of the suggestions, but the "Search for <term> with:" and "Change Search Settings" labels do not fit well

See http://www.screencast.com/t/l1thdb8IAbV for the Ubuntu issue. Felipe, should we reopen this to also fix it better on Linux?
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(felipc)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #16)

> The test results are mixed:
> - Windows - the dropdown fits just 2 search engines per row, but still fits
> the suggestions and other labels fairly well

Could you please file a follow-up bug for this? Did you test on HiDPI Windows or a Windows at the usual resolution?

> - Ubuntu - the dropdown fits 3 search engines per row, fits a relatively ok
> part of the suggestions, but the "Search for <term> with:" and "Change
> Search Settings" labels do not fit well
> 
> See http://www.screencast.com/t/l1thdb8IAbV for the Ubuntu issue. Felipe,
> should we reopen this to also fix it better on Linux?

Reopening this bug would make the tracking here confusing as this patch has been uplifted. I think the issue for Linux (mostly caused by the larger default font size there) will be significantly improved once bug 1111638 is fixed. If you think that bug doesn't fully cover the problem, please file a separate bug. Thanks!
Flags: needinfo?(felipc)
See Also: → 1115012
Thank you Florian! I've filed bug 1115012 for Windows showing the search engines only 2 per row (this is regular resolution, no HDPI), and I think we can track the remaining Linux issue in bug 1111638 (I've added a comment there so we know that Ubuntu is the only platform still affected).

Closing this issue since I've managed to also verify it on the latest Firefox 36 Aurora (BuildID=20141223004006) with the same results as Beta and Nightly.
You need to log in before you can comment on or make changes to this bug.