GNOME Bugzilla – Bug 646921
Notifications are not right-aligned on RTL layout
Last modified: 2011-04-15 18:21:54 UTC
Notifications are not right-aligned on RTL layout. Currently, the notifications are left aligned instead. text-align: right; in the CSS will fix this issue.
Screenshot (not displaying any confidential data) attachment welcome.
Created attachment 185323 [details] Screenshot A sample notification created with notify-send (no actual meaning, it says "Hello World" and "content"). This is how it looks when the notification appears. On mouse-over, the text is aligned correctly, but the title is still aligned to the left.
Created attachment 185324 [details] Screenshot on mouse over
Created attachment 185328 [details] [review] notifications: Fix alignment/ordering in RTL locales Currently, text in notifications is left-aligned, and the banner text is located at the right of the title - we need to swap those around and right-align text for RTL locales.
Created attachment 185333 [details] [review] notifications: Fix order of title/banner for RTL locales Previous patch was overzealous and changed the text alignment - the result looked right when testing with LTR input, but not when actually testing with RTL (thanks elad) ...
Comment on attachment 185333 [details] [review] notifications: Fix order of title/banner for RTL locales Well... it definitely gives the right results when sending RTL notifications in an RTL locale, or LTR notifications in an LTR locale. But I'm not sure about the behavior when sending, eg, English notifications under LANG=he_IL.UTF-8. I guess that's not really supposed to happen, but... Anyway, if elad is happy with this patch, then I have no complaints.
(In reply to comment #6) > (From update of attachment 185333 [details] [review]) > Well... it definitely gives the right results when sending RTL notifications in > an RTL locale, or LTR notifications in an LTR locale. But I'm not sure about > the behavior when sending, eg, English notifications under LANG=he_IL.UTF-8. I > guess that's not really supposed to happen, but... > > Anyway, if elad is happy with this patch, then I have no complaints. I think you do have a point here, perhaps we should set the align based on the content. (Like gtk input areas does)
(In reply to comment #6) > (From update of attachment 185333 [details] [review]) > Well... it definitely gives the right results when sending RTL notifications in > an RTL locale, or LTR notifications in an LTR locale. But I'm not sure about > the behavior when sending, eg, English notifications under LANG=he_IL.UTF-8. I > guess that's not really supposed to happen, but... Mmmh, yeah ... it's at the very least weird that the banner mode will right-align latin text, while the body ends up left-aligned when expanded. I guess we could use something like pango_find_base_dir() instead of st_widget_get_direction() (or use st_widget_set_direction() in st_label_set_text()). If we do that, we should probably force the direction on the notification as well, so the icon ends up correctly next to the title. Then there's still the problem that title and banner may use different text directions - what would be the expected behavior in that case?
(In reply to comment #8) > Then there's still the problem that title and banner may use different text > directions - what would be the expected behavior in that case? I would expect content that it is LTR to be left aligned, content that it is RTL to be right aligned, and the same for the title. I don't think it would look weird if the title is aligned to one direction and the content to another.
Created attachment 185805 [details] [review] notifications: Fix order of title/banner for RTL text Updated patch which should cope well with using different scripts for title/summary.
Comment on attachment 185805 [details] [review] notifications: Fix order of title/banner for RTL text >+ this._titleDirection = St.Widget.get_default_direction(); no real point in initializing this like that, since we'll always end up overwriting it later, right? >+ if (pangoDir == Pango.Direction.RTL || >+ pangoDir == Pango.Direction.WEAK_RTL || >+ pangoDir == Pango.Direction.TTB_RTL) http://developer.gnome.org/pango/stable/pango-Bidirectional-Text.html#PangoDirection says that TTB_RTL is deprecated and unused, and WEAK_RTL is never returned from Pango.find_base_dir(). >- titleBox.x1 = titleBox.y1 = 0; >- titleBox.x2 = Math.min(titleNatW, availWidth); >+ let titleBoxW = Math.min(titleNatW, availWidth); I didn't look at the math; I'm assuming it's the same as before.
(In reply to comment #11) > (From update of attachment 185805 [details] [review]) > >+ this._titleDirection = St.Widget.get_default_direction(); > > no real point in initializing this like that, since we'll always end up > overwriting it later, right? Yeah, I just like to reference all properties in _init(), but it is pointless here ... should I kill that line or just initialize to 0? > http://developer.gnome.org/pango/stable/pango-Bidirectional-Text.html#PangoDirection > says that TTB_RTL is deprecated and unused, and WEAK_RTL is never returned from > Pango.find_base_dir(). Nice. > >- titleBox.x1 = titleBox.y1 = 0; > >- titleBox.x2 = Math.min(titleNatW, availWidth); > >+ let titleBoxW = Math.min(titleNatW, availWidth); > > I didn't look at the math; I'm assuming it's the same as before. Mostly, except for the banner label, which was allocated at full width if possible or the available width otherwise. I changed that to always allocate it at the available width, so that the left/right alignment is done automatically by the label rather than the code here.
(In reply to comment #12) > Yeah, I just like to reference all properties in _init(), but it is pointless > here ... should I kill that line or just initialize to 0? St.TextDirection.NONE? (aka 0) > > I didn't look at the math; I'm assuming it's the same as before. > > Mostly, except for the banner label that looks right
Created attachment 185907 [details] [review] notifications: Fix order of title/banner for RTL text (In reply to comment #13) > St.TextDirection.NONE? (aka 0) Sure. > > > I didn't look at the math; I'm assuming it's the same as before. > > > > Mostly, except for the banner label > > that looks right OK. (Obsoleting both patches assuming we'll go with the 2nd one)
Comment on attachment 185907 [details] [review] notifications: Fix order of title/banner for RTL text looks good. (Would be nice if Elad could test it too.)
(In reply to comment #15) > (From update of attachment 185907 [details] [review]) > (Would be nice if Elad could test it too.) Yeah, makes sense.
Works well.
(In reply to comment #17) > Works well. Thanks for testing!
Review of attachment 185907 [details] [review]: Think think this is fine, code looks good as far as I can tell, and testing works right in a bunch of cases. ::: js/ui/messageTray.js @@ +524,3 @@ + // is done correctly automatically. + if (this._table.get_direction() != this._titleDirection) + this._table.set_direction(this._titleDirection); Not in anyway a correctness issue, but this._table.set_direction() already optimizes out work if the effective direction doesn't change, so I don't see any reason to do the pre-check.
Attachment 185907 [details] pushed as 1d2eadb - notifications: Fix order of title/banner for RTL text (In reply to comment #19) > Not in anyway a correctness issue, but this._table.set_direction() already > optimizes out work if the effective direction doesn't change, so I don't see > any reason to do the pre-check. Good point - removed the check before pushing.