Closed
Bug 1106054
Opened 9 years ago
Closed 9 years ago
Search dropdown should have a minimum width
Categories
(Firefox :: Search, defect)
Tracking
()
People
(Reporter: phlsa, Assigned: Felipe)
References
Details
Attachments
(1 file, 1 obsolete file)
2.19 KB,
patch
|
florian
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
(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"
Comment 3•9 years ago
|
||
(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
Updated•9 years ago
|
Flags: firefox-backlog+
Updated•9 years ago
|
Points: --- → 1
Comment 5•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → felipc
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Flags: qe-verify?
Assignee | ||
Comment 6•9 years ago
|
||
Keeping it simple for now, without trying to handle bug 1111638
Attachment #8538596 -
Flags: review?(florian)
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
Comment on attachment 8538656 [details] [diff] [review] Patch v2 Thanks!
Attachment #8538656 -
Flags: review?(florian) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7ec919dc3789
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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?
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ec919dc3789
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8538656 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8538656 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Comment 16•9 years ago
|
||
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)
Comment 17•9 years ago
|
||
(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)
Comment 18•9 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•