1267802 - Firebug theme - breadcrumbs-divider has wrong direction and position in RTL locales

Status: RESOLVED WONTFIX

(DevTools :: Inspector, defect, P2)

Description

[magicp]

Attachment: rtl-devtools-inspector-breadcrumbs-divider.png

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

Steps to reproduce:

1. Start latest Nightly with RTL locales (e.g. Arabic version)
2. Go to any sites (e.g. about:home)
3. Open DevTools > Inspector with Firebug theme
4. Confirm breadcrumbs-divider


Actual results:

breadcrumbs-divider has wrong direction and position in RTL locales. And also light-theme's breadcrumbs-divider is overlapped. Please find attached image.


Expected results:

breadcrumbs-divider has correct direction and position.  And also light-theme's breadcrumbs-divider does not display.

Comment 1

[magicp]

(In reply to magicp from comment #0)

> breadcrumbs-divider has correct direction and position.  And also
> light-theme's breadcrumbs-divider does not display.

Regarding light-theme's breadcrumbs-divider, filed to Bug 1268738 because it also reproduces in LTR locales.

Comment 2

[Patrick Brosset <:pbro>]

Bug triage (filter on CLIMBING SHOES).

Comment 3

[Tim Nguyen :ntim]

Instead of applying a simple rotation, it would be nice to consolidate the CSS by applying the same rules for both Firebug and Default (light/dark) to generate the separators.

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

Attachment: dt-fb-inspect-breadcrumb.png

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #3)
> Instead of applying a simple rotation, it would be nice to consolidate the
> CSS by applying the same rules for both Firebug and Default (light/dark) to
> generate the separators.
This seems like probably a better approach than my initial one using SVG <use>, transforms, and anchor targeting to clone and flip the image in the SVG.  It can't be exactly the same or you end up with the attached image, but I have a patch incoming that should do the job, though there are a couple possible niggles with it.

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1267802 - Make the devtools firebug theme breadcrumb divider work more like the default, so RTL support is easier.

Use the divider SVG as a background for #breadcrumb-separator-normal and that
as a background for the pseudo element, rather than the SVG as the content of
the pseudo element, since pseudo elements can't be CSS transformed

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

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/63780/#review60832

::: devtools/client/themes/widgets.css:358
(Diff revision 1)
> +  width: 5px;
> +  height: 7px;
> +}
> +
>  .theme-firebug .breadcrumbs-widget-item {
> -  margin-inline-start: 10px;
> +  margin-left: 1px;

No point using the logical properties when they get direction: ltr;ed anyway, start will always resolve to left.

::: devtools/client/themes/widgets.css:406
(Diff revision 1)
>    left: -3px;
> -  margin: 0 0 0 -5px;
> -  padding: 0;
> -  width: 5px;
> +  margin: 0 0 0 -6px;
> +}
> +
> +.theme-firebug .breadcrumbs-widget-item:-moz-locale-dir(rtl)::after {
> +  right: -7px;

Why do the margin and box offset swap in RTL?  It doesn't look right otherwise, but I don't understand why.
Why is right -7 instead of -6?  I thought it looked better.

Comment 7

[Tim Nguyen :ntim]

(In reply to Ian Moody [:Kwan] from comment #6)
> https://reviewboard.mozilla.org/r/63780/#review60832
> 
> ::: devtools/client/themes/widgets.css:358
> (Diff revision 1)
> > +  width: 5px;
> > +  height: 7px;
> > +}
> > +
> >  .theme-firebug .breadcrumbs-widget-item {
> > -  margin-inline-start: 10px;
> > +  margin-left: 1px;
> 
> No point using the logical properties when they get direction: ltr;ed
> anyway, start will always resolve to left.
Where is direction: ltr; set ?

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #7)
> (In reply to Ian Moody [:Kwan] from comment #6)
> > https://reviewboard.mozilla.org/r/63780/#review60832
> > 
> > ::: devtools/client/themes/widgets.css:358
> > (Diff revision 1)
> > > +  width: 5px;
> > > +  height: 7px;
> > > +}
> > > +
> > >  .theme-firebug .breadcrumbs-widget-item {
> > > -  margin-inline-start: 10px;
> > > +  margin-left: 1px;
> > 
> > No point using the logical properties when they get direction: ltr;ed
> > anyway, start will always resolve to left.
> Where is direction: ltr; set ?

I believe this should be the right link, though I'm not at my usual computer so doing this is harder:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/widgets.css#9
The reason it's set is so the CSS selectors stay "tag #id .class" rather than ".class id# tag" or something.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8770317 [details]
Bug 1267802 - Make the devtools firebug theme breadcrumb divider work more like the default, so RTL support is easier.

Redirecting to bgrins who has worked on the breadcrumbs widget.

Comment 10

[Tim Nguyen :ntim]

(Although the changes do look good to me, I'd like bgrins to take a second look)

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Ian Moody [:Kwan] from comment #5)
> Use the divider SVG as a background for #breadcrumb-separator-normal and that
> as a background for the pseudo element, rather than the SVG as the content of
> the pseudo element, since pseudo elements can't be CSS transformed

They can be transformed, if you apply display: inline-block to them.  Would being able to do this simplify the patch?  I'd like to avoid bringing in the -moz-element hack here if at all possible, and I'm not sure I follow all of the numerical values in the CSS - hopefully being able to do the transform will help but let me know if you want me to review it as-is

Comment 12

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Ian Moody [:Kwan] from comment #5)
> > Use the divider SVG as a background for #breadcrumb-separator-normal and that
> > as a background for the pseudo element, rather than the SVG as the content of
> > the pseudo element, since pseudo elements can't be CSS transformed
> 
> They can be transformed, if you apply display: inline-block to them.  Would
> being able to do this simplify the patch?  I'd like to avoid bringing in the
> -moz-element hack here if at all possible, and I'm not sure I follow all of
> the numerical values in the CSS - hopefully being able to do the transform
> will help but let me know if you want me to review it as-is

We already use -moz-element for the dark/light themes, any reason the firebug theme should follow a different approach ?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > (In reply to Ian Moody [:Kwan] from comment #5)
> > > Use the divider SVG as a background for #breadcrumb-separator-normal and that
> > > as a background for the pseudo element, rather than the SVG as the content of
> > > the pseudo element, since pseudo elements can't be CSS transformed
> > 
> > They can be transformed, if you apply display: inline-block to them.  Would
> > being able to do this simplify the patch?  I'd like to avoid bringing in the
> > -moz-element hack here if at all possible, and I'm not sure I follow all of
> > the numerical values in the CSS - hopefully being able to do the transform
> > will help but let me know if you want me to review it as-is
> 
> We already use -moz-element for the dark/light themes, any reason the
> firebug theme should follow a different approach ?

If it is being adapted to use the current approach then I'd expect a net removal of code - relying on the existing styling on .breadcrumbs-widget-item which uses #breadcrumb-separator-normal as a background image.  What it looks like the theme is doing instead is clearing out the background that's already there then creating ::before/::after elements for showing the separator. Anyway, we don't need -moz-element here for firebug since it just needs to render a simple background image (as opposed to light/dark themes which are more complex).

It doesn't seem right to me but I haven't dug into the details enough to say 'no' either.  If you are happy with the approach, then maybe ask Honza for additional review just so he's in the loop on the Firebug changes.

Comment 14

[Ian Moody [:Kwan] (UTC+1)]

Not currently working on this.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The Firebug theme was removed in Bug 1378108, so I am marking this report as WONTFIX.

Honza