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)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: jwilliams, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding][photon-onboarding-newui])

Attachments

(5 files)

Attached image Scroll Bar.png
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.
Whiteboard: [photon-onboarding] → [photon-onboarding][triage]
Verdi, how do you think?
Flags: needinfo?(mverdi)
(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)
Fisher, could you put your idea here?
Flags: needinfo?(fliu)
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)
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P3 → P1
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
fischer's suggestion fits well
Attachment #8899365 - Flags: review?(rexboy) → review+
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
ni? for the comment 8.
Flags: needinfo?(rexboy)
Flags: needinfo?(gasolin)
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+
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
Blocks: 1392467
Blocks: 1392468
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.
Blocks: 1392552
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
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.
No longer blocks: 1392468
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+
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
https://hg.mozilla.org/mozilla-central/rev/f26b821af577
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Attached image FR Locale.png
I can verify this is fixed on today's nightly based on comment 11.

Attachment shows the case of too much text.
Status: RESOLVED → VERIFIED
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?
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+
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
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)
Flags: needinfo?(jwilliams)
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)
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)
CCing Théo. With that amount of space, once again, we shouldn't be asking people to shorten their translations.
Flags: needinfo?(francesco.lodolo)
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
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)
(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.
(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.
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
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.
Flags: needinfo?(jwilliams)
You need to log in before you can comment on or make changes to this bug.