1285528 - [RTL] Collapse/Expand pane button is not properly displayed

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P1)

Description

[Ada [:adalucinet]]

[Affected versions]:
*reproducible only with RTL builds
- Firefox 47.0.1 (Build ID: 20160623154057)
- Firefox 48 beta 6 (Build ID: 20160706215822)
- latest Aurora 49.0a1
- latest Nightly 50.0a1

[Affected platforms]:
- Windows 10 64-bit
- Mac OS X 10.9.5
- Ubuntu 14.04 x86

[Steps to reproduce]:
1. Launch l10n Firefox build.
2. Open Inspector: Ctrl + Shift + C (for Windows & Ubuntu) or Cmd + Opt + C (for Mac OS X)

[Expected result]: Collapse/Expand pane button is properly displayed.

[Actual result]: Collapse/Expand pane button is not properly displayed.

[Regression range]: 
- Also reproducible with the oldest available l10n Nightly build on the ftp - 45.0a1 (from 2015-11-02); I guess this is a long standing issue.

[Additional notes]:
- Screenshot with both normal and RTL builds → https://i.imgur.com/46FrV18.png

Comment 1

[Marco Mucci [:MarcoM]]

Hi Alexandra, which bug is this related to?

Comment 2

[Marco Mucci [:MarcoM]]

Is there a specific Bug ID I can make this bug block?

Comment 3

[Marco Mucci [:MarcoM]]

(In reply to Marco Mucci [:MarcoM] from comment #2)
> Is there a specific Bug ID I can make this bug block?

Comment 4

[Ada [:adalucinet]]

(In reply to Marco Mucci [:MarcoM] from comment #3)
> (In reply to Marco Mucci [:MarcoM] from comment #2)
> > Is there a specific Bug ID I can make this bug block?

Hey Marco! This issue was found while verifying bug 1266420; but, as far as I can see, it's a long standing bug, definitely not a recent regression.

Comment 5

[Marco Mucci [:MarcoM]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #4)
> (In reply to Marco Mucci [:MarcoM] from comment #3)
> > (In reply to Marco Mucci [:MarcoM] from comment #2)
> > > Is there a specific Bug ID I can make this bug block?
> 
> Hey Marco! This issue was found while verifying bug 1266420; but, as far as
> I can see, it's a long standing bug, definitely not a recent regression.

Thanks very much Alexandra.  Track #2 team will triage this bug.

Comment 6

[Marco Mucci [:MarcoM]]

Waiting on team to triage tomorrow.

Comment 7

[[e-mail]]

seems pseudo-class(::-moz-dir) with pseudo-element(:before) does not work

> - .sidebar-toggle::before {
> + .sidebar-toggle::before,
> + .sidebar-toggle.pane-collapsed:-moz-dir(rtl)::before {
>   background-image: var(--theme-pane-collapse-image);
> }

Does it a bug of pseudo selector, or should I assign image in code instead of in CSS?

Comment 8

[Tim Nguyen :ntim (please use needinfo?)]

(In reply to Fred Lin [:gasolin] from comment #7)
> seems pseudo-class(::-moz-dir) with pseudo-element(:before) does not work
> 
> > - .sidebar-toggle::before {
> > + .sidebar-toggle::before,
> > + .sidebar-toggle.pane-collapsed:-moz-dir(rtl)::before {
> >   background-image: var(--theme-pane-collapse-image);
> > }
> 
> Does it a bug of pseudo selector, or should I assign image in code instead
> of in CSS?
Your approach should work, it's just a matter of choosing the right selector :)

::-moz-dir has been replaced by ::dir. ::dir only works on HTML documents, while ::-moz-locale-dir works for XUL documents.

Comment 9

[[e-mail]]

-moz-locale-dir works, thanks! I was thinking the div button is a normal html element :/

Comment 10

[[e-mail]]

Attachment: Bug 1285528 - [RTL] Collapse/Expand pane button is not properly displayed;

Review commit: https://reviewboard.mozilla.org/r/67096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67096/

Comment 11

[Tim Nguyen :ntim (please use needinfo?)]

(In reply to Fred Lin [:gasolin] from comment #9)
> -moz-locale-dir works, thanks! I was thinking the div button is a normal
> html element :/

It is, except inspector.xul is a XUL doc.

Comment 12

[Tim Nguyen :ntim (please use needinfo?)]

Comment on attachment 8774638 [details]
Bug 1285528 - [RTL] Collapse/Expand pane button is not properly displayed;

https://reviewboard.mozilla.org/r/67096/#review63948

::: devtools/client/shared/components/sidebar-toggle.css:10
(Diff revision 1)
>  
>  .sidebar-toggle {
>    display: block;
>  }
>  
> -.sidebar-toggle::before {
> +.sidebar-toggle::before,

Can you change this to .sidebar-toggle:-moz-locale-dir(ltr) ?

::: devtools/client/shared/components/sidebar-toggle.css:15
(Diff revision 1)
> -.sidebar-toggle::before {
> +.sidebar-toggle::before,
> +.sidebar-toggle.pane-collapsed:-moz-locale-dir(rtl)::before {
>    background-image: var(--theme-pane-collapse-image);
>  }
>  
> -.sidebar-toggle.pane-collapsed::before {
> +.sidebar-toggle.pane-collapsed::before,

Same thing here

::: devtools/client/shared/components/sidebar-toggle.css:27
(Diff revision 1)
> +  .sidebar-toggle:-moz-locale-dir(rtl)::before {
> +    transform: rotate(-90deg);
> +  }

Does the sidebar have a different location (top instead of bottom) on RTL for widths < 700px ? If not, can you remove this ?

Comment 13

[[e-mail]]

https://reviewboard.mozilla.org/r/67096/#review63948

> Can you change this to .sidebar-toggle:-moz-locale-dir(ltr) ?

done

> Does the sidebar have a different location (top instead of bottom) on RTL for widths < 700px ? If not, can you remove this ?

In RTL mode the icon is changed to the conter-part, so the icon has to rotate in counter direction as well.

Comment 14

[[e-mail]]

Comment on attachment 8774638 [details]
Bug 1285528 - [RTL] Collapse/Expand pane button is not properly displayed;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67096/diff/1-2/

Comment 15

[Tim Nguyen :ntim (please use needinfo?)]

Comment on attachment 8774638 [details]
Bug 1285528 - [RTL] Collapse/Expand pane button is not properly displayed;

https://reviewboard.mozilla.org/r/67096/#review64414

::: devtools/client/shared/components/sidebar-toggle.css:27
(Diff revision 2)
>  @media (max-width: 700px) {
> -  .sidebar-toggle::before {
> +  .sidebar-toggle:-moz-locale-dir(ltr)::before {
>      transform: rotate(90deg);
>    }
> +
> +  .sidebar-toggle:-moz-locale-dir(rtl)::before {

I think this is worth adding a comment:
/* Since RTL swaps the used images, we need to flip them the other way round */

Comment 16

[[e-mail]]

Comment on attachment 8774638 [details]
Bug 1285528 - [RTL] Collapse/Expand pane button is not properly displayed;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67096/diff/2-3/

Comment 17

[[e-mail]]

comment added, thanks for review!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f5875c1a1fbd
[RTL] Collapse/Expand pane button is not properly displayed. r=ntim

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Ada [:adalucinet]]

Verified fixed with latest 50.0a1 RTL, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.

Comment 22

[Itiel]

Fixed, but only for the Inspector tab. I guess the same patch should be applied to other tabs (such as Debugger, Network Monitor and possibly more).
Can it be addressed here or should I open a new bug for it?

Comment 23

[Marco Mucci [:MarcoM]]

(In reply to ItielMaN from comment #22)
> Fixed, but only for the Inspector tab. I guess the same patch should be
> applied to other tabs (such as Debugger, Network Monitor and possibly more).
> Can it be addressed here or should I open a new bug for it?

Please open a new bug.  Thanks.

Comment 24

[Itiel]

bug 1295161 was opened.
Thanks.