1205563 - [RTL] devtools-sidebar-alltabs (overflow dropmarker) is placed wrong in RTL locales

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[magicp]

Attachment: rtl-devtools-sidebar-alltabs.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150916030203

Steps to reproduce:

1. Run any Firefox version in Arabic.
2. Open Inspector in Developer Tools
3. Display devtools-sidebar-alltabs


Actual results:

devtools-sidebar-alltabs does not have a left border.


Expected results:

devtools-sidebar-alltabs should have a left border, and also does not need a right border.

Comment 1

[arni2033]

[right="0"] attribute of .devtools-sidebar-alltabs dropmarker should be replaced with [end="0"]
I haven't read that code, but probably some script creates that dropmarker when you open inspector
So, the attribute in that script should be replaced.

Comment 2

[arni2033]

If somebody's going to work on this bug, take into account that the same happens in Netmonitor for .html files. So it's better to file a new report about that/fix it in this bug (I haven't found a dupe)

Comment 3

[Tim Nguyen :ntim]

(In reply to arni2033 from comment #2)
> If somebody's going to work on this bug, take into account that the same
> happens in Netmonitor for .html files. So it's better to file a new report
> about that/fix it in this bug (I haven't found a dupe)

The same code is behind the Inspector and the Netmonitor: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/sidebar.js#132
So you can just replace right with end on that file, and both issues are fixed.

Comment 4

[magicp]

> Expected results:
>
> devtools-sidebar-alltabs should have a left border, and also does not need a right border.

I'd like to change expected result.

- Keep right border
- devtools-sidebar-alltabs position change from right side (inside) to left side (outside)

Comment 5

[Patrick]

Hi I would like to work on this. Please confirm it's OK to proceed.

Comment 6

[Tim Nguyen :ntim]

(In reply to Patrick from comment #5)
> Hi I would like to work on this. Please confirm it's OK to proceed.
Please do. Just post a patch here and we'll assign this bug to you.

Comment 7

[Victor Olasupo]

Hi I want to have a go at this bug if it's okay, please look at my patch below

Comment 8

[Victor Olasupo]

Attachment: Bug1205563_sidebar.diff

Please have a look at my patch and assign me where applicable please, thanks

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8696572 [details] [diff] [review]
Bug1205563_sidebar.diff

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

Thanks for the patch ! Looks fine, but why did you set the left attribute to 0 ?

Comment 10

[Victor Olasupo]

Attachment: Bug1205563_sidebar.diff

Hi Tim, I've changed the attribute value to "11" instead? I hope this suffices.

Comment 11

[Tim Nguyen :ntim]

(In reply to Victor Olasupo from comment #10)
> Created attachment 8696819 [details] [diff] [review]
> Bug1205563_sidebar.diff
> 
> Hi Tim, I've changed the attribute value to "11" instead? I hope this
> suffices.

I don't understand why we need to set the left attribute at all. Replacing right with end should suffice for this bug.

Comment 12

[Victor Olasupo]

Attachment: Bug1205563_sidebar.diff

Patch has been updated.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8696836 [details] [diff] [review]
Bug1205563_sidebar.diff

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

Looks good ! This just needs peer review. Also, can you change the commit message to:
Bug 1205563 - Correctly place overflow dropmarker for RTL locales. r=pbro, ntim

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8696836 [details] [diff] [review]
Bug1205563_sidebar.diff

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

Looks good to me.

Comment 15

[Victor Olasupo]

Attachment: Bug1205563_sidebar.diff

Patch has been updated.

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8697703 [details] [diff] [review]
Bug1205563_sidebar.diff

You don't need to re-request review since :pbro and I already gave you r+

Comment 17

[Victor Olasupo]

ohh sorry about that, thanks

Comment 18

[Carsten Book [:Tomcat]]

Hi, this patch failed to apply:

renamed 1205563 -> Bug1205563_sidebar.diff
applying Bug1205563_sidebar.diff
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug1205563_sidebar.diff

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: bug_1205563_correctly_place_overflow_drop_marker_for_rtl_locales_r_pbro_nti.patch

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Filter on CLIMBING SHOES

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/e0fa395e4982a5225e73908a4a0816932043440d
Bug 1205563 - Correctly place overflow drop marker for RTL Locales. r=pbro, ntim

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Thanks for the patch Victor. It has been landed!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0fa395e4982