Sync sidebar doesnt support the intl.uidirection preference to override the layout direction

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: daleharvey, Assigned: cng)

Tracking

Trunk
Firefox 62
Points:
---

Firefox Tracking Flags

(firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
In https://bugzilla.mozilla.org/show_bug.cgi?id=1411230 I tested rtl by setting intl.uidirection = 1, the sync sidebar did not seem to respect that setting and I had to set <body dir="rtl" to check for rtl
Since bug 1244481 the sidebar should support rtl for locales where that's the default. A quick look for that pref implies it's only implemented for XUL and this sidebar is HTML - so this bug is really just about adding support for that pref.
Summary: Sync sidebar doesnt support rtl → Sync sidebar doesnt support the intl.uidirection preference to override the layout direction
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Assignee: nobody → cng
Status: NEW → ASSIGNED

Comment 3

10 months ago
mozreview-review
Comment on attachment 8980714 [details]
Bug 1417577 - Support locale direction changes in sync sidebar

https://reviewboard.mozilla.org/r/246880/#review253668

r+ with fix. This is great looking, I left a few comments there and there :)

::: browser/components/syncedtabs/SyncedTabsDeckComponent.js:83
(Diff revision 1)
>      // ui-refreshing notifications! We listen to :ready in order to check again
>      // if this engine is disabled and refresh the UI one last time.
>      Services.obs.addObserver(this, "weave:service:ready");
>  
> +    // Add intl.uidirection support for HTML sidebar
> +    Services.prefs.addObserver("intl.uidirection", this);

Nit: We could have also used https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/xpconnect/loader/XPCOMUtils.jsm#377-381 instead

::: browser/components/syncedtabs/test/xpcshell/test_SyncedTabsDeckComponent.js:134
(Diff revision 1)
>  
>    Assert.ok(component.observe.calledWith(null, "weave:service:login:change"),
>      "component is notified of login change");
>    Assert.equal(component.updatePanel.callCount, 4, "triggers panel update again");
> +
> +  Services.prefs.setIntPref("intl.uidirection", 100);

Let's use 1 instead, see https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#2253
Attachment #8980714 - Flags: review?(eoger) → review+
Comment hidden (mozreview-request)

Comment 5

10 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3cb69bac2eac
Support intl.uidirection pref in sync sidebar r=eoger
Hi all, sorry for late input but please, move this to use https://searchfox.org/mozilla-central/source/intl/locale/mozILocaleService.idl#207 instead of intl.uidirection.

With the patch as you land it you're adding support for RTL that only covers pseudolocale, and doesn't cover real RTL locales.

For event - please use `intl:app-locales-changed`.
I'm going to ask for backout, can you amend the patch Carol?
Flags: needinfo?(cng)
Comment hidden (mozreview-request)

Comment 10

10 months ago
mozreview-review
Comment on attachment 8980714 [details]
Bug 1417577 - Support locale direction changes in sync sidebar

https://reviewboard.mozilla.org/r/246880/#review254158

::: browser/components/syncedtabs/SyncedTabsDeckComponent.js:83
(Diff revision 3)
>      // ui-refreshing notifications! We listen to :ready in order to check again
>      // if this engine is disabled and refresh the UI one last time.
>      Services.obs.addObserver(this, "weave:service:ready");
>  
> +    // Add app locale change support for HTML sidebar
> +    Services.obs.addObserver(this, "intl:app-locales-changed");

I think you also need to observe for the pref `intl.uidirection` since we're not going to fire the `intl:app-locales-changed` when this pref flips.
Attachment #8980714 - Flags: review?(gandalf) → review+
The rest looks great, thank you!
Comment hidden (mozreview-request)
(Assignee)

Comment 13

10 months ago
mozreview-review-reply
Comment on attachment 8980714 [details]
Bug 1417577 - Support locale direction changes in sync sidebar

https://reviewboard.mozilla.org/r/246880/#review253668

> Nit: We could have also used https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/xpconnect/loader/XPCOMUtils.jsm#377-381 instead

I've opted for prefs.addObserver now in the newest version, since the pref's value isn't used directly and it makes the series of additional observers slightly nicer.
(Assignee)

Updated

10 months ago
Flags: needinfo?(cng)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 14

10 months ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25432177de8f
Support locale direction changes in sync sidebar r=eoger,gandalf
Keywords: checkin-needed

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25432177de8f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
status-firefox59: affected → wontfix
status-firefox60: --- → wontfix
status-firefox61: --- → wontfix
You need to log in before you can comment on or make changes to this bug.