Closed Bug 1220839 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: bgrins, Assigned: peregrino)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 1216569
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?
(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]
Thanks for the pointers! Will look into that :)
(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #3)
> Thanks for the pointers! Will look into that :)

Great, thanks!
(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?
(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
Assignee: nobody → colmeiro
Status: NEW → ASSIGNED
Attachment #8684747 - Flags: review?(bgrinstead)
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.
Depends on: 1223150
(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 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?
Attachment #8684747 - Flags: review?(bgrinstead)
Added dir to body in computed and rules views
Attachment #8684747 - Attachment is obsolete: true
Attachment #8685243 - Flags: review?(bgrinstead)
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)
Attachment #8685243 - Flags: review?(bgrinstead) → review-
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
Attachment #8685253 - Flags: review?(bgrinstead) → review+
Blocks: 1206338
Blocks: 1206341
Attachment #8685243 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/411e731c6a0d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.