Closed
Bug 1236372
Opened 9 years ago
Closed 9 years ago
Synced Tabs panel should autosize to fit localized mobile promo content (scrollbars at the moment)
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
Firefox 47
People
(Reporter: aryx, Assigned: markh, Mentored)
References
Details
Attachments
(3 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The German localization of the mobile Sync promo in Firefox Desktop has a vertical scrollbar at the moment. The panel should auto-size. (German localization hasn't been released yet.)
Assignee | ||
Updated•9 years ago
|
Mentor: markh
Whiteboard: [good first bug][lang=css]
Assignee | ||
Comment 1•9 years ago
|
||
Note that this bug only happens when the button is in the toolbar area rather than in the panel, and is due to bug 1224412. As a work-around, we've set a min-height of 30em for the panel - which was supposed to be big enough to handle all the "static" content without scrollbars - see https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/browser/themes/shared/customizableui/panelUIOverlay.inc.css#722
30em already leaves alot of whitespace for English strings, so I'm a little reluctant to increase this too far given the implications of this bug seem small (it's a minor visual glitch and the user doesn't even need to scroll to hit the UI)
So, handball to Ryan - the attachment shows what the panel looks like with English strings and a min-height of 33em (which I *think* will remove the scrollbar in this bug - but possibly not for languages with even longer translations) - Ryan, are you OK with that, or would you prefer to see the scrollbars for some locales?
Flags: needinfo?(rfeeley)
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Priority: -- → P1
hello Markh, I am a newbee, I want to contribute in the open source.so please assign me this bug.
Comment 3•9 years ago
|
||
Definitely prefer without scrollbars if auto-height is not possible.
Flags: needinfo?(rfeeley)
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Parul from comment #2)
> hello Markh, I am a newbee, I want to contribute in the open source.so
> please assign me this bug.
Sorry Paul but work on this bug is already ongoing - I've removed "good first bug" - sorry for the confusion.
Sebastian, is it possible to test that changing https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/browser/themes/shared/customizableui/panelUIOverlay.inc.css#722 to 33em solves your problem? If it's a PITA I can put a try run up.
Assignee: nobody → markh
Flags: needinfo?(aryx.bugmail)
Whiteboard: [good first bug][lang=css]
Reporter | ||
Comment 5•9 years ago
|
||
Thank you, changing it to 33em solves the issue on both Nightly and Aurora.
Flags: needinfo?(aryx.bugmail)
Assignee | ||
Comment 6•9 years ago
|
||
Oops, I dropped this on the floor.
This increases the hard-coded size of the panel by 3em - there's still a change other localizations will see the scrollbar, but German is fairly verbose and seems a reasonable yardstick.
Attachment #8716188 -
Flags: review?(gijskruitbosch+bugs)
Comment 7•9 years ago
|
||
Comment on attachment 8716188 [details] [diff] [review]
0013-Bug-1236372-increase-the-size-of-the-synced-tabs-pan.patch
rs=me based on Aryx's comment.
... however, wouldn't it be better to use calc() ? The image in some of these panes is absolutely (pixel) sized, isn't it? So it should ideally be some combination of that and the em size needed for accompanying text etc.
Attachment #8716188 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #7)
> ... however, wouldn't it be better to use calc() ? The image in some of
> these panes is absolutely (pixel) sized, isn't it? So it should ideally be
> some combination of that and the em size needed for accompanying text etc.
Probably yeah - I'm going to land this as-is and I opened bug 1248506 to investigate that.
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8716188 [details] [diff] [review]
0013-Bug-1236372-increase-the-size-of-the-synced-tabs-pan.patch
Approval Request Comment
[Feature/regressing bug #]: Synced Tabs
[User impact if declined]: Some locales may see scrollbars in the synced tabs panel due to localized text being longer than expected.
[Describe test coverage new/current, TreeHerder]: Existing tests pass.
[Risks and why]: Very low risk, 1 line CSS change limited to synced tabs styles.
[String/UUID change made/needed]: None
Attachment #8716188 -
Flags: approval-mozilla-beta?
Attachment #8716188 -
Flags: approval-mozilla-aurora?
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•9 years ago
|
Flags: qe-verify+
Updated•9 years ago
|
status-firefox45:
--- → affected
status-firefox46:
--- → affected
Comment 12•9 years ago
|
||
Comment on attachment 8716188 [details] [diff] [review]
0013-Bug-1236372-increase-the-size-of-the-synced-tabs-pan.patch
Improve the support for other locales and low risk, taking it
Should be in 45 beta 7
Attachment #8716188 -
Flags: approval-mozilla-beta?
Attachment #8716188 -
Flags: approval-mozilla-beta+
Attachment #8716188 -
Flags: approval-mozilla-aurora?
Attachment #8716188 -
Flags: approval-mozilla-aurora+
Comment 13•9 years ago
|
||
bugherder uplift |
Comment 14•9 years ago
|
||
bugherder uplift |
Comment 15•9 years ago
|
||
Verified fixed on Firefox 47.0a1 (2016-02-22), Firefox 46.0a2 (2016-02-22) and Firefox 45 beta 8 (20160221141421) under Windows 10 64-bit and Mac OS X 10.9.5. Synced Tabs panel appears as intended and there are no scrollbars displayed for German localization.
Sebastian could you please also confirm that this bug is fixed?
Flags: needinfo?(aryx.bugmail)
Reporter | ||
Comment 16•9 years ago
|
||
Fixed with Firefox 45.0 beta 9 on Windows 8.1 (125% dpi).
Status: RESOLVED → VERIFIED
Flags: needinfo?(aryx.bugmail)
Comment 17•9 years ago
|
||
Thanks Sebastian for your testing! Updating flags to reflect the current situation.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•