Closed
Bug 1388813
Opened 7 years ago
Closed 7 years ago
[Onboarding] Tour Notification Description has a scroll bar on FR Locale
Categories
(Firefox :: New Tab Page, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 57
People
(Reporter: jwilliams, Assigned: gasolin)
References
Details
(Whiteboard: [photon-onboarding][photon-onboarding-newui])
Attachments
(5 files)
Not sure if this is a bug or not but wanted to report the issue. It looks a bit weird having a scroll bar in the notification. Please close if this is expected. See Attachment.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-onboarding] → [photon-onboarding][triage]
Comment 2•7 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #1) > Verdi, how do you think? I think the best option is to go with Fischer's idea of a 150px max-height.
Flags: needinfo?(mverdi)
Comment 4•7 years ago
|
||
Make the notification bar height a range between 122px ~ 150px to reduce the possibility seeing a scroll bar. Still show the scroll bar if needed in case that some strings in other language are longer than in the most languages.
Flags: needinfo?(fliu)
Updated•7 years ago
|
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P3 → P1
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
fischer's suggestion fits well
Updated•7 years ago
|
Attachment #8899365 -
Flags: review?(rexboy) → review+
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Drive-by: When writing the comment 4, hadn't done the test, only a proposal. And as what :timdream raised, we should be careful about the vertical overflow issue when doing the height approach. With this patch's approach, it may bring the issue that the notification body overflows out of the notification bar when reaching the max height of 150px. See attachment 8899473 [details]: notification_body_overflow_out_of_notification_bar_on_max_height_150px.png
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8899365 [details] Bug 1388813 - [Onboarding] expand Tour Notification description width to allow longer descrition; Since we experienced spec change and now the fox icon at left is removed, we will have more space at left side. I'd plan to alter this bug to update notification layout and style to fit new spec and address the long description issue by expand the description width.
Attachment #8899365 -
Flags: review+
Assignee | ||
Comment 12•7 years ago
|
||
based on https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit * add a copy of the item’s overlay navigation icon in the headline - if headline is too long it will be shown as 2 lines. Should we wrap the line under the first line of text * if description is too long, try to expand it's width to the left (bug's origin scope) * update the color and style of the button
Flags: needinfo?(rexboy)
Flags: needinfo?(gasolin)
Summary: [Onboarding] Tour Notification Description has a scroll bar on FR Locale → [Onboarding] update notification layout and style based on new spec
Assignee | ||
Comment 13•7 years ago
|
||
on https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit Aaron comment that (if headline is too long, the text should) wrap under the icon.
Assignee | ||
Comment 14•7 years ago
|
||
after discussion, I'd change the scope back and track the new spec in bug 1392552
Summary: [Onboarding] update notification layout and style based on new spec → [Onboarding] Tour Notification Description has a scroll bar on FR Locale
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
with new UI spec https://mozilla.invisionapp.com/share/XYD3ERUKE#/screens/249240594 The notification main section allows total 738px wide. We will have about 500+px left (738-64-130-18*2 = 508) minus the icon and the button width. Compare to origin 420px, that will allow enough space for longer description.
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8899365 [details] Bug 1388813 - [Onboarding] expand Tour Notification description width to allow longer descrition; https://reviewboard.mozilla.org/r/170594/#review177344
Attachment #8899365 -
Flags: review?(rexboy) → review+
Comment 19•7 years ago
|
||
Pushed by flin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f26b821af577 [Onboarding] expand Tour Notification description width to allow longer descrition;r=rexboy
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f26b821af577
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Reporter | ||
Comment 21•7 years ago
|
||
I can verify this is fixed on today's nightly based on comment 11. Attachment shows the case of too much text.
Reporter | ||
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 22•7 years ago
|
||
Comment on attachment 8899365 [details] Bug 1388813 - [Onboarding] expand Tour Notification description width to allow longer descrition; Approval Request Comment [Feature/Bug causing the regression]: bug 1357641 [User impact if declined]: notification in Franch may show the scrollbar by default [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: N [List of other uplifts needed for the feature/fix]: N [Is the change risky?]: N [Why is the change risky/not risky?]: 1 line CSS width only change [String changes made/needed]: N
Attachment #8899365 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 23•7 years ago
|
||
Comment on attachment 8899365 [details] Bug 1388813 - [Onboarding] expand Tour Notification description width to allow longer descrition; Fix a UI issue for onboarding tour notification and was verified. Beta56+.
Attachment #8899365 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 24•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4ea3381ed581
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
Comment 25•7 years ago
|
||
Bug can be reopened I guess looking at the joined screenshot took from latest Nightly. It's a slightly smaller scroll, and I believe it's back after fixing bug 1396734. What do you thin?
Flags: needinfo?(gasolin)
Updated•7 years ago
|
Flags: needinfo?(jwilliams)
Assignee | ||
Comment 26•7 years ago
|
||
Aaron we down size the height from 120px to 100px to fit the new UI spec, but description in some locale is still to long to show the scroll bar. I saw snippet currently use 120px height. Do you have plan to change that or should we align to the 120px height?
Flags: needinfo?(gasolin) → needinfo?(abenson)
Assignee | ||
Comment 27•7 years ago
|
||
Aaron, as bug 1399955 comment 7 from flod, we shouldn't go for language specific change. Flod, could you help refer franch team to see if it's possible to shorten the FR translation?
Flags: needinfo?(francesco.lodolo)
Comment 28•7 years ago
|
||
CCing Théo. With that amount of space, once again, we shouldn't be asking people to shorten their translations.
Flags: needinfo?(francesco.lodolo)
Comment 29•7 years ago
|
||
Sorry, I’ve already done my best here. Also, even if I could shorten French, it is far from being the longest translation: https://transvision.mozfr.org/string/?entity=browser/extensions/onboarding/onboarding.properties:onboarding.notification.onboarding-tour-sync.message&repo=central el, gd, id, it, ka, kk, ms and ru all seem to be about the same length or even longer. As flod pointed out, the way to fix this is with UI changes. Allowing for 3 lines for this label seem an easy way
Comment 30•7 years ago
|
||
I'm a little confused about the screenshot from Comment 25 and Theo's comment about allowing for 3 lines. There looks to be pleanty of room in that snippet to contain the text and it has 3 lines - unless I'm not seeing content that's scrolled? We could reduce the space between the headline and the paragraph if this is a matter of pinching pixels. Try giving everything the same line-height as the paragraph section. Try reducing the left/right padding of the button so there's more horizontal space. As long as the button looks equally weighted on all sides it doesn't need to be that wide. I talked to Fred about this briefly and the ideal solution would be to let the snippet text drive how tall it needed to be. We want to avoid the opposite problem here where shorter translations result in a very chunky looking snippet and is covering your stuff on new tab unnecessarily.
Flags: needinfo?(abenson)
Comment 31•7 years ago
|
||
(In reply to Aaron Benson from comment #30) > I'm a little confused about the screenshot from Comment 25 and Theo's > comment about allowing for 3 lines. There looks to be pleanty of room in > that snippet to contain the text and it has 3 lines - unless I'm not seeing > content that's scrolled? You’re seeing everything, I meant 3 full lines, without a scroll bar.
Comment 32•7 years ago
|
||
(In reply to Théo Chevalier [:tchevalier] from comment #31) > (In reply to Aaron Benson from comment #30) > > I'm a little confused about the screenshot from Comment 25 and Theo's > > comment about allowing for 3 lines. There looks to be pleanty of room in > > that snippet to contain the text and it has 3 lines - unless I'm not seeing > > content that's scrolled? > > You’re seeing everything, I meant 3 full lines, without a scroll bar. Okay, got it. I still think this is a matter of setting a min-height, then letting the snippet be taller only if it needs to be. Looks like we were close to this on Comment 7. Another thing to consider is the page needs a margin added to the bottom so they page can scroll all the way down and not have the snippet covering the content.
Assignee | ||
Comment 33•7 years ago
|
||
The issue is fixed in bug 1399955, which add max-height to allow showing 1 more line of description in same height. When the description is in normal size, the layout is identical as usual. No string change need here. That fix still have issue described in comment 8 (for more longer description the scrollbar will bleed the height of notification bar) But that should not happen if we pick string no longer than current locale strings.
Blocks: 1399955
Comment 34•7 years ago
|
||
I have reproduced this issue using Firefox 57.0a1-Fr (2017.08.09) on Win 8.1 x64. I can confirm this issue is fixed, I verified using Firefox 57.0b7, and 56.0.1 (fr build) on Win 8.1 x64, Mac OS X 10.12.6 and Ubuntu 14.04 x64.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(jwilliams)
You need to log in
before you can comment on or make changes to this bug.
Description
•