Closed Bug 1244481 Opened 8 years ago Closed 8 years ago

[RTL][Synced Tabs] The synced tabs sidebar has wrong direction in RTL locales

Categories

(Firefox :: Sync, defect, P2)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- verified

People

(Reporter: magicp.jp, Assigned: markh)

References

Details

Attachments

(1 file, 2 obsolete files)

0011-Bug-1244481-ensure-Synced-Tabs-sidebar-and-its-twist.patch
17.28 KB, patch
Gijs
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160130030240

Steps to reproduce:

1. Start latest Nightly in RTL locales with synced environment
2. View the Synced Tabs sidebar


Actual results:

The Synced Tabs sidebar is displayed as LTR


Expected results:

The Synced Tabs sidebar should be displayed as RTL in RTL locales
Has STR: --- → yes
status-firefox47: --- → affected
Component: Untriaged → Sync
OS: Unspecified → All
Hardware: Unspecified → All
Blocks: 1201331, 996638
Blocks: 1246869
(In reply to magicp from comment #0)
> Expected results:
> 
> The Synced Tabs sidebar should be displayed as RTL in RTL locales

Do the History and Bookmarks sidebars support RTL?
Flags: needinfo?(magicp.jp)
(In reply to Ryan Feeley [:rfeeley] from comment #1)
> Do the History and Bookmarks sidebars support RTL?

Yes, they (and PanelUI-remotetabs) support RTL.
Flags: needinfo?(magicp.jp)
Blocks: 1239084
Flags: firefox-backlog+
Priority: -- → P2
(In reply to magicp from comment #2)
> (In reply to Ryan Feeley [:rfeeley] from comment #1)
> > Do the History and Bookmarks sidebars support RTL?
> 
> Yes, they (and PanelUI-remotetabs) support RTL.

Sigh. Would this have been a freebie if we had implemented this in XUL instead rebuilding it in HTML?
(In reply to Ryan Feeley [:rfeeley] from comment #3)

> Sigh. Would this have been a freebie if we had implemented this in XUL
> instead rebuilding it in HTML?

What you mean? How are you thinking about to support RTL?
From what I understand the bookmarks and history sidebars are implemented in XUL, but our developer was required to redo it in HTML. I simply did the UX component.
(In reply to Ryan Feeley [:rfeeley] from comment #5)
> From what I understand the bookmarks and history sidebars are implemented in
> XUL, but our developer was required to redo it in HTML. I simply did the UX
> component.

Is it official direction? If it's not, you should do this!
This is simple to fix and I have a WIP (it just needs |dir="&locale.dir;"| on <body>). However, another complication is that we also need a twisty that goes in the other direction in rtl.
(In reply to Mark Hammond [:markh] from comment #8)
> However, another complication is that we also need a twisty that goes in the other direction in rtl.

There are already available "chrome://global/skin/tree/twisty.svg". If [+]and[-] twisty are not have to think about direction.
(In reply to magicp from comment #9)
> There are already available "chrome://global/skin/tree/twisty.svg". If
> [+]and[-] twisty are not have to think about direction.

We don't want to use [+] or [-] - we want to be consistent with the other sidebars. We want to use similar to what OSX uses for the sidebar - "arrow" style twisties, and these do need a different direction for RTL.
(In reply to Mark Hammond [:markh] from comment #10)

> We don't want to use [+] or [-] - we want to be consistent with the other sidebars.

Sounds good. Because, the following twisties are used now in the default theme on Windows.
 [-] chrome://browser/skin/syncedtabs/twisty-open.svg
 [+] chrome://browser/skin/syncedtabs/twisty-closed.svg
This patch also fixes bug 1245728 (wrong twisties used on Windows and Linux) as the twisties also have rtl implications.

Note that this is very much a WIP and needs lots of cleanup - I'm asking for feedback on the general idea, notably using a single .svg with *all* twisties and referencing the specific ones we need using a fragment ID.

Of particular note, the name of the .svg file is yet to be renamed - it's still called arrow-closed.svg, but I'll probably rename it to arrow-twisties.svg and delete arrow-open.svg.

It looks fairly good on Windows, but I'm yet to test this on OSX and it looks like I'm not going to get to that today, so I thought I'd ask for feedback before I work further on this approach.
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8718222 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8718222 [details] [diff] [review]
0010-wip-for-rtl-and-twisties.patch

Somehow my review queue was a train wreck when I got to it this morning, despite leaving it empty last night. From purely a very quick look... why aren't we just using the same files as the bookmarks/history sidebar (which I assume are the toolkit ones) ? I would prefer us not reinventing the wheel. That way we should be getting platform-specific things for free (see e.g. bug 1191230)
Attachment #8718222 - Flags: feedback?(gijskruitbosch+bugs)
No longer blocks: 996638
This got confusing :)

For the builtin tree control:
* On Windows, a SVG is used for twisties - there's no "-moz-appearance: treetwisty*" style set.
* On Linux there's *both* "-moz-appearance: treetwisty;" and "list-style-image: ..." - however, my testing shows the list-style-image is not being uses - only treetwisty seems relevant.
* On OSX there's only -moz-appearance: treetwisty.

So this patch takes the same approach (and fortunately "treetwisty*" is working in the html) - although it does *not* bother with the (apparently redundant) list-style-image on Linux. Thus, this patch removes all svgs added by the synced tabs sidebar and moves some of the CSS around so the shared stuff is truly shared and the platform specific rules are in their platform's css.

With this patch, everything seems to work correctly in LTR and RTL, with the twisties flipping on RTL as expected - except on Linux where the twisties are pointing in the wrong direction with RTL - but they also are for the bookmarks and history sidebars, so that problem isn't syncedtabs specific. It's possible that's an issue with ForceRTL, but either way, I'm convinced this sidebar behaves like the other sidebars.
Attachment #8718222 - Attachment is obsolete: true
Attachment #8719626 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8719626 [details] [diff] [review]
0011-Bug-1244481-ensure-Synced-Tabs-sidebar-and-its-twist.patch

Review of attachment 8719626 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but if you land it like this please file a followup to tweak some of the padding/margin.

::: browser/themes/linux/syncedtabs/sidebar.css
@@ +60,5 @@
> +.item.client .item-twisty-container {
> +  -moz-appearance: treetwistyopen;
> +  padding-end: 4px;
> +  margin-top: 3px;
> +  width: 9px; /* The image's width is 9 pixels */

Is it necessary to set this? I would have expected the moz-appearance to take care of this...

::: browser/themes/osx/syncedtabs/sidebar.css
@@ +38,3 @@
>  .item.client .item-twisty-container {
> +  margin-top: -2px;
> +  padding-right: 5px;

FWIW, it seems to me that this leads to more padding than in the other sidebars (visually, when testing this patch on OS X). I'm also confused about how this works when the document is in RTL mode. It seems OS X's tree.css sets -moz-padding-end to 2px. Though, testing this a bit, it seems the padding is also getting ignored by virtue of the -moz-appearance... Did you notice the same?

Maybe we need to tweak the margins for OS X? Can be part of a follow up because it seems like this is a big enough patch that getting this out of the way and then dealing with the minor tweaks in a followup would be more productive...

::: browser/themes/windows/syncedtabs/sidebar.css
@@ +115,5 @@
> +.item.client .item-twisty-container:hover {
> +  background-image: url("chrome://global/skin/tree/twisty.svg#open-hover");
> +}
> +
> +.item.client.closed .item-twisty-container:-moz-dir(rtl) {

Nit: can you order the RTL ones the same way as the non-RTL ones?
Attachment #8719626 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #15)

> Is it necessary to set this? I would have expected the moz-appearance to
> take care of this...

Nope - removed.

> ::: browser/themes/osx/syncedtabs/sidebar.css
> @@ +38,3 @@
> >  .item.client .item-twisty-container {
> > +  margin-top: -2px;
> > +  padding-right: 5px;
> 
> FWIW, it seems to me that this leads to more padding than in the other
> sidebars (visually, when testing this patch on OS X). I'm also confused
> about how this works when the document is in RTL mode. It seems OS X's
> tree.css sets -moz-padding-end to 2px. Though, testing this a bit, it seems
> the padding is also getting ignored by virtue of the -moz-appearance... Did
> you notice the same?

I do see the padding have no effect (so I removed it), but I don't see any significant padding differences - maybe a pixel or so, but using a "ruler" app there is no significant change for me.

> Maybe we need to tweak the margins for OS X? Can be part of a follow up
> because it seems like this is a big enough patch that getting this out of
> the way and then dealing with the minor tweaks in a followup would be more
> productive...

Given the above I haven't filed a followup yet, but I'm sure UX will be all over it if they see a problem.

> Nit: can you order the RTL ones the same way as the non-RTL ones?

Done.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8456a64466c1 - seems to be perma-orange in a webgl test in dom/canvas, and I can't see how this could possibly cause that, so landing...

Thanks!
https://hg.mozilla.org/mozilla-central/rev/b68b4f667bfc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
I've reproduced the initial issue on Windows 7 64bit using Nightly 47.0a1 (2016-01-30), ar build. 

I've tested on Windows 7 64bit, Mac OSX 10.9.5 and Ubuntu 13.10 32bit using latest DevEdition 47.0a2 (buildID: 20160322004033), ar, fa and he builds and the synced tabs sidebar has the right direction.
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
Attachment #8980699 - Attachment is obsolete: true
Attachment #8980699 - Flags: review?(eoger)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: