Closed Bug 1417577 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, enhancement, P3)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: daleharvey, Assigned: cng)

Details

Attachments

(1 file)

Bug 1417577 - Support locale direction changes in sync sidebar
59 bytes, text/x-review-board-request
eoger
: review+
zbraniecki
: review+
Details 
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
Assignee: nobody → cng
Status: NEW → ASSIGNED
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+
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 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 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.
Flags: needinfo?(cng)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/25432177de8f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: