1220839 - Make 'clear' icon show up on the proper side of the filter box for rule/computed view in RTL

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

The 'x' / clear icon shows up on the right side of the searchbox used for filtering styles in the computed view and rule view.  It's using position:absolute on the element and a padding-right, so it isn't RTL friendly.

Comment 1

[Hernán Rodriguez Colmeiro (:peregrino)]

What would be the approach to make it RTL friendly? I imagine avoiding using an absolute position and somehow float it, but I never dealt with RTL issues so not completely sure. And how do you test RTL stuff?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #1)
> What would be the approach to make it RTL friendly? I imagine avoiding using
> an absolute position and somehow float it, but I never dealt with RTL issues
> so not completely sure. And how do you test RTL stuff?

You can install the Force RTL addon and then toggle RTL mode that way: https://addons.mozilla.org/en-US/firefox/addon/force-rtl/.  In this particular case probably the easiest thing to do would be something like:

.devtools-searchinput-clear:-moz-locale-dir(rtl) {
  right: auto; /* or something to reset it */
  left: 7px;
}

and then change the padding-right to padding-inline-end on .devtools-rule-searchbox[filled]

Comment 3

[Hernán Rodriguez Colmeiro (:peregrino)]

Thanks for the pointers! Will look into that :)

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #3)
> Thanks for the pointers! Will look into that :)

Great, thanks!

Comment 5

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> You can install the Force RTL addon and then toggle RTL mode that way:
> https://addons.mozilla.org/en-US/firefox/addon/force-rtl/.  In this
> particular case probably the easiest thing to do would be something like:
> 
> .devtools-searchinput-clear:-moz-locale-dir(rtl) {
>   right: auto; /* or something to reset it */
>   left: 7px;
> }

It seems -moz-locale-dir doesn't match on HTML (https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir%28rtl%29), so maybe I have to convert that search box into xul?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > You can install the Force RTL addon and then toggle RTL mode that way:
> > https://addons.mozilla.org/en-US/firefox/addon/force-rtl/.  In this
> > particular case probably the easiest thing to do would be something like:
> > 
> > .devtools-searchinput-clear:-moz-locale-dir(rtl) {
> >   right: auto; /* or something to reset it */
> >   left: 7px;
> > }
> 
> It seems -moz-locale-dir doesn't match on HTML
> (https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir%28rtl%29),
> so maybe I have to convert that search box into xul?

Ah, try using :-moz-dir instead: https://developer.mozilla.org/en-US/docs/Web/CSS/:dir.  I believe that will work based on all the hits here: https://dxr.mozilla.org/mozilla-central/search?q=%3A-moz-dir&redirect=true&case=false

Comment 7

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Make the inspector rule/computed searchbox clear icon show correctly in RTL

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8684747 [details] [diff] [review]
Make the inspector rule/computed searchbox clear icon show correctly in RTL

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

So this works for me if I set dir="rtl" on the body in cssruleview.xhtml, but the "Force RTL Direction" extension doesn't cause the selector to match.  I want to confirm that for a UA that has rtl as the default that it will match.   Based on the summary here [0] I believe it will, but I want to be sure so am asking around in #l10n before r+ing

[0]: https://developer.mozilla.org/en-US/docs/Web/CSS/:dir.

Comment 9

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> So this works for me if I set dir="rtl" on the body in cssruleview.xhtml,
> but the "Force RTL Direction" extension doesn't cause the selector to match.

Yeah, that happened to me too and the only way I could test was to set the dir property in the devtools by hand. I tried to use the Force RTL extension and Arabic locale, but it wasn't triggering either. Not sure if that was because I'm on English OSX, but switching the whole OS to Arabic can get a bit tricky to revert...

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8684747 [details] [diff] [review]
Make the inspector rule/computed searchbox clear icon show correctly in RTL

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

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > So this works for me if I set dir="rtl" on the body in cssruleview.xhtml,
> > but the "Force RTL Direction" extension doesn't cause the selector to match.
> 
> Yeah, that happened to me too and the only way I could test was to set the
> dir property in the devtools by hand. I tried to use the Force RTL extension
> and Arabic locale, but it wasn't triggering either. Not sure if that was
> because I'm on English OSX, but switching the whole OS to Arabic can get a
> bit tricky to revert...

Yeah the easiest thing to do would be to add these two things to both rule and computed view markup:

  <!ENTITY % globalDTD
    SYSTEM "chrome://global/locale/global.dtd">
  %globalDTD;

and <body dir="&locale.dir;">

as aboutNetError does: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml.

I'd suggest let's do that for now and then when we come up with a better generic solution in Bug 1223150 we can pull those bits out.  Can you update the patch with that change?

Comment 11

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Make the inspector rule/computed searchbox clear icon show correctly in RTL

Added dir to body in computed and rules views

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8685243 [details] [diff] [review]
Make the inspector rule/computed searchbox clear icon show correctly in RTL

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

The changes to computed view and rule view look good, but this looks like the wrong patch otherwise (intended for bug 1222881)

Comment 13

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Make the inspector rule/computed searchbox clear icon show correctly in RTL

Proper new patch

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8685253 [details] [diff] [review]
Make the inspector rule/computed searchbox clear icon show correctly in RTL

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

Nice, thanks

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/411e731c6a0d

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/411e731c6a0d

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8685253 [details] [diff] [review]
Make the inspector rule/computed searchbox clear icon show correctly in RTL

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

::: devtools/client/themes/toolbars.css
@@ +408,5 @@
>  
>  .devtools-rule-searchbox[filled] {
>    background-color: var(--searchbox-background-color);
>    border-color: var(--searchbox-border-color);
> +  -moz-padding-end: 23px;

padding-inline-end

@@ +436,5 @@
>  
> +.devtools-searchinput-clear:-moz-dir(rtl) {
> +  right: unset;
> +  left: 7px;
> +}

Instead of setting right and left for rtl, I would have directly set offset-inline-start/offset-inline-end on the normal rule.