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

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: tomer, Assigned: tomer)

Tracking

({rtl})

55 Branch
Firefox 56
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding], URL)

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
Posted image Screenshot (he)
Steps to reproduce: Open new tab

Expected result: main content should be displayed in the middle.
(Assignee)

Comment 1

2 years ago
Posted image Screenshot (en-us)
Please note that the close button may disappear if window is too narrow, as almost happened in this screenshot.
(Assignee)

Comment 2

2 years ago
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=36dfc734a106adc61c97b2d6084b1f0fcc9f000fhttps://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
(Assignee)

Comment 3

2 years ago
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
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.

Updated

2 years ago
Component: Tours → New Tab Page
Whiteboard: [photon-onboarding][triage]

Updated

2 years ago
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
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8882187 - Flags: review?(adw) → review?(dtownsend)
(Assignee)

Comment 10

2 years ago
(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?
Comment hidden (mozreview-request)
Flags: qe-verify+
QA Contact: jwilliams
Target Milestone: --- → Firefox 56
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
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 18

2 years ago
mozreview-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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
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

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b83d25b6a8cb
https://hg.mozilla.org/mozilla-central/rev/3d308947661f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Comment 21

2 years ago
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.