Closed Bug 1417577 Opened 8 years ago Closed 7 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: 7 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: