Closed Bug 1377129 Opened 7 years ago Closed 7 years ago

[RTL] bottom onBoarding tour notification doesn't display well on RTL builds

Categories

(Firefox :: New Tab Page, defect, P1)

Product:
Firefox
Component:
New Tab Page
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: tomer, Assigned: tomer)

References

(
URL
)

Details

(Keywords: rtl, Whiteboard: [photon-onboarding])

Attachments

(4 files)

Screenshot (he)
47.80 KB, image/png
Details
Screenshot (en-us)
44.25 KB, image/png
Details
Bug 1377129 - [RTL] bottom onBoarding tour notification doesn't display well on RTL builds (fox icon)
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1377129 - [RTL] bottom onBoarding tour notification doesn't display well on RTL builds (main content)
59 bytes, text/x-review-board-request
mossop
: review+
Details
Attached image Screenshot (he) — 
Steps to reproduce: Open new tab

Expected result: main content should be displayed in the middle.
Attached image Screenshot (en-us) — 
Please note that the close button may disappear if window is too narrow, as almost happened in this screenshot.
8:31.30 INFO: Last good revision: 5554fd16af9b429902897cb1d32f1fcd7d47517b
 8:31.30 INFO: First bad revision: 36dfc734a106adc61c97b2d6084b1f0fcc9f000f
 8:31.30 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5554fd16af9b429902897cb1d32f1fcd7d47517b&tochange=36dfc734a106adc61c97b2d6084b1f0fcc9f000f


→ https://hg.mozilla.org/integration/autoland/rev/886e2e0b90a7

(The notification strings are not exposed yet on Pontoon, so there is some guesswork how it will behave on with RTL content on RTL builds)
Blocks: 1357641
Summary: [RTL] New To Nightly bottom banner doesn't display well on RTL → [RTL] bottom onBoarding tour notification doesn't display well on RTL builds
Quick fix: Remove offset-inline-start:50% from #onboarding-notification-message-section:dir(rtl) on onboarding.css, but this is an ugly workaround. I am investigating.
Assignee: nobody → tomer.moz.bugs
URL: https://dxr.mozilla.org/mozilla-centr...
Status: NEW → ASSIGNED
(In reply to Tomer Cohen :tomer from comment #1)
> Created attachment 8882171 [details]
> Screenshot (en-us)
> 
> Please note that the close button may disappear if window is too narrow, as almost happened in this screenshot.
Hi Tomer, This is fixed in Bug 1375783 for your information.
Component: Tours → New Tab Page
Whiteboard: [photon-onboarding][triage]
Priority: -- → P1
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
(In reply to Tomer Cohen :tomer from comment #3)
> Quick fix: Remove offset-inline-start:50% from
> #onboarding-notification-message-section:dir(rtl) on onboarding.css, but this is an ugly workaround. I am investigating.
Hi Tomer,

Thank you for picking this bug.
Please feel free to needinfo me if any question.
p.s For #onboarding-notification-message-section, we could simply remove `offset-block-start` and `offset-inline-start`, and then use `top: 50%; left: 50%` since it should be always centered regardless of RTL or LTR.

Thank you
(In reply to Fischer [:Fischer] from comment #5)
> Please feel free to needinfo me if any question.
> p.s For #onboarding-notification-message-section, we could simply remove
> `offset-block-start` and `offset-inline-start`, and then use `top: 50%;
> left: 50%` since it should be always centered regardless of RTL or LTR.

left:50% does mean that the element should start there, and end somewhere between 50% to 100%. It may still require replacing left with right on :dir(rtl). offset-inline-start behaves as left on LTR and right on RTL, so when it works as expected it removes the dependency for the :dir() selector.
(In reply to Tomer Cohen :tomer from comment #7)
> (In reply to Fischer [:Fischer] from comment #5)
> > Please feel free to needinfo me if any question.
> > p.s For #onboarding-notification-message-section, we could simply remove
> > `offset-block-start` and `offset-inline-start`, and then use `top: 50%;
> > left: 50%` since it should be always centered regardless of RTL or LTR.
> 
> left:50% does mean that the element should start there, and end somewhere
> between 50% to 100%. It may still require replacing left with right on
> :dir(rtl). offset-inline-start behaves as left on LTR and right on RTL, so
> when it works as expected it removes the dependency for the :dir() selector.
There is another rule: `transform: translate(-50%, -50%);`. That is going to center #onboarding-notification-message-section.
And looks like the patch hasn't address the fox icon and the message circle. Are you planning to address that in the next patch.
And you would want send the review request to :mossop, he is the reviewer of this project. Thanks.
(In reply to Fischer [:Fischer] from comment #8)
> And looks like the patch hasn't address the fox icon and the message circle.
> Are you planning to address that in the next patch.
Have not noticed that. Thanks. Will fix it right away. 
> And you would want send the review request to :mossop, he is the reviewer of
> this project. Thanks.
I was using bugzilla default component suggestions. Sorry for that, I will change it right away.
Attachment #8882187 - Flags: review?(adw) → review?(dtownsend)
(In reply to Fischer [:Fischer] from comment #8)
> There is another rule: `transform: translate(-50%, -50%);`. That is going to
> center #onboarding-notification-message-section.

`transform:translate()` can't deal well with bidirectional content, as (0,0) is always Top Left, while `offset-inline-start` does change its meaning from `left` to `right`. In other words, replacing `offset-inline-start` with `left` can fix this, but as best practice for bidirectional content, someone may think this is a leftover from the time there was no offset-inline-* support.

Also, I find this way of centering content very difficult to understand, as you set the element start to 50% of the parent width, then move it backward 25%, to have it start on 25% and end on 75%. Isn't there a better way for centering content, using flex or grids?
Flags: qe-verify+
QA Contact: jwilliams
Target Milestone: --- → Firefox 56
Comment on attachment 8882187 [details]
Bug 1377129 - [RTL] bottom onBoarding tour notification doesn't display well on RTL builds (fox icon)

https://reviewboard.mozilla.org/r/153294/#review158804
Attachment #8882187 - Flags: review?(dtownsend) → review+
Comment on attachment 8882319 [details]
Bug 1377129 - [RTL] bottom onBoarding tour notification doesn't display well on RTL builds (main content)

https://reviewboard.mozilla.org/r/153408/#review158808
Attachment #8882319 - Flags: review?(dtownsend) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b83d25b6a8cb
[RTL] bottom onBoarding tour notification doesn't display well on RTL builds (main content) r=mossop
https://hg.mozilla.org/integration/autoland/rev/3d308947661f
[RTL] bottom onBoarding tour notification doesn't display well on RTL builds (fox icon) r=mossop
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b83d25b6a8cb
https://hg.mozilla.org/mozilla-central/rev/3d308947661f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
I have reproduced this Bug on Nightly 56.0a1 (2017-06-29) on Windows 10, 64 bit!

The bug's fix is now verified on latest  Nightly 56.0a1

Build ID    :	20170706060058
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170705]
I have confirmed this fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: