700639 - Make the script debugger UI look right on RTL

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Setting the configuration property intl.uidirection.en to rtl has no effect on the script debugger. However, the rest of the browser chrome is instantly altered and so is the style inspector, highlighter, HTML panel, etc. The debugger should behave appropriately in RTL locales.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

This will probably require a conversion of debugger.xhtml to xul, as we had to do for the style inspector in bug 700036.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

RTL issues are fixed, but I broke everything else. CSS tweaking is coming next.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP 2

Fixed some brokenness, more to come. Also incorporated a couple of simplifications Mihai suggested in bug 697762.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch

This version fixes all outstanding issues, besides some windows-related styling, which I am fixing in bug 702939, because splitting the css files is a prerequisite.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 577972 [details] [diff] [review]
Working patch

We think this should be included when landing the debugger to m-c, so one of you guys (or both) should review it. This does not touch toolkit/ code.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 577972 [details] [diff] [review]
Working patch

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

Patch looks good, only one concern noted below.

::: browser/devtools/debugger/debugger-view.js
@@ +731,1 @@
>      arrow.style.visibility = "hidden";

Why not use element.hidden to toggle the element visibility ?

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Thank you Mihai. Rob will you review this, too, or should I land it in remote-debug?(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 577972 [details] [diff] [review]
> Working patch
> 
> Review of attachment 577972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good, only one concern noted below.
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +731,1 @@
> >      arrow.style.visibility = "hidden";
> 
> Why not use element.hidden to toggle the element visibility ?

Victor could comment about the why, but I'll just note that changing it now requires some more CSS changes, because the arrow is not rendered at all in that case and the line moves to the left. Also, I'm always ambivalent about using the hidden property based on the note here:

https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden

Do you feel strongly about this?

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #8)
> Thank you Mihai. Rob will you review this, too, or should I land it in
> remote-debug?(In reply to Mihai Sucan [:msucan] from comment #7)
> > Comment on attachment 577972 [details] [diff] [review]
> > Working patch
> > 
> > Review of attachment 577972 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch looks good, only one concern noted below.
> > 
> > ::: browser/devtools/debugger/debugger-view.js
> > @@ +731,1 @@
> > >      arrow.style.visibility = "hidden";
> > 
> > Why not use element.hidden to toggle the element visibility ?
> 
> Victor could comment about the why, but I'll just note that changing it now
> requires some more CSS changes, because the arrow is not rendered at all in
> that case and the line moves to the left. Also, I'm always ambivalent about
> using the hidden property based on the note here:
> 
> https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden
> 
> Do you feel strongly about this?

I don't feel strongly about this. I just noted it because it would move style code out of the JS. Currently this patch had to go from display = "block" to display = "-moz-box" because there were some style changes. With .hidden (or some other attribute), the code wouldn't have to change - only the styles.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #9)
> (In reply to Panos Astithas [:past] from comment #8)
> > Thank you Mihai. Rob will you review this, too, or should I land it in
> > remote-debug?(In reply to Mihai Sucan [:msucan] from comment #7)

Doh, copy and paste fail, disregard this.

> > > Comment on attachment 577972 [details] [diff] [review]
> > > Working patch
> > > 
> > > Review of attachment 577972 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Patch looks good, only one concern noted below.
> > > 
> > > ::: browser/devtools/debugger/debugger-view.js
> > > @@ +731,1 @@
> > > >      arrow.style.visibility = "hidden";
> > > 
> > > Why not use element.hidden to toggle the element visibility ?
> > 
> > Victor could comment about the why, but I'll just note that changing it now
> > requires some more CSS changes, because the arrow is not rendered at all in
> > that case and the line moves to the left. Also, I'm always ambivalent about
> > using the hidden property based on the note here:
> > 
> > https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden
> > 
> > Do you feel strongly about this?
> 
> I don't feel strongly about this. I just noted it because it would move
> style code out of the JS. Currently this patch had to go from display =

Agreed, that's a good thing.

> "block" to display = "-moz-box" because there were some style changes. With
> .hidden (or some other attribute), the code wouldn't have to change - only
> the styles.

The reason for -moz-box is the change from an HTML document to a XUL document. Everything was broken and the easiest way I found to fix it was to use -moz-box.

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #8)
> Thank you Mihai. Rob will you review this, too, or should I land it in
> remote-debug?(In reply to Mihai Sucan [:msucan] from comment #7)
> > Comment on attachment 577972 [details] [diff] [review]
> > Working patch
> > 
> > Review of attachment 577972 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch looks good, only one concern noted below.
> > 
> > ::: browser/devtools/debugger/debugger-view.js
> > @@ +731,1 @@
> > >      arrow.style.visibility = "hidden";
> > 
> > Why not use element.hidden to toggle the element visibility ?
> 
> Victor could comment about the why, but I'll just note that changing it now
> requires some more CSS changes, because the arrow is not rendered at all in
> that case and the line moves to the left. Also, I'm always ambivalent about
> using the hidden property based on the note here:
> 
> https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden

I have no serious objections with element.hidden instead of visibility="hidden", but I usually prefer changing the visibility. We could create two classes (.visible, .hidden?) to deal with the js vs. css code changing.

Comment 12

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #10)
> > "block" to display = "-moz-box" because there were some style changes. With
> > .hidden (or some other attribute), the code wouldn't have to change - only
> > the styles.
> 
> The reason for -moz-box is the change from an HTML document to a XUL
> document. Everything was broken and the easiest way I found to fix it was to
> use -moz-box.

Yep, the change makes sense.


(In reply to Victor Porof from comment #11)
> > Victor could comment about the why, but I'll just note that changing it now
> > requires some more CSS changes, because the arrow is not rendered at all in
> > that case and the line moves to the left. Also, I'm always ambivalent about
> > using the hidden property based on the note here:
> > 
> > https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden
> 
> I have no serious objections with element.hidden instead of
> visibility="hidden", but I usually prefer changing the visibility. We could
> create two classes (.visible, .hidden?) to deal with the js vs. css code
> changing.

element.hidden is not the same as element.style.visibility=hidden. element.hidden = true is similar to element.style.display = none.

(anyway, this is going way off-topic ;) )

Comment 13

[Victor Porof [:vporof][:vp]]

(In reply to Mihai Sucan [:msucan] from comment #12)
> (In reply to Panos Astithas [:past] from comment #10)
> > > "block" to display = "-moz-box" because there were some style changes. With
> > > .hidden (or some other attribute), the code wouldn't have to change - only
> > > the styles.
> > 
> > The reason for -moz-box is the change from an HTML document to a XUL
> > document. Everything was broken and the easiest way I found to fix it was to
> > use -moz-box.
> 
> Yep, the change makes sense.
> 
> 
> (In reply to Victor Porof from comment #11)
> > > Victor could comment about the why, but I'll just note that changing it now
> > > requires some more CSS changes, because the arrow is not rendered at all in
> > > that case and the line moves to the left. Also, I'm always ambivalent about
> > > using the hidden property based on the note here:
> > > 
> > > https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden
> > 
> > I have no serious objections with element.hidden instead of
> > visibility="hidden", but I usually prefer changing the visibility. We could
> > create two classes (.visible, .hidden?) to deal with the js vs. css code
> > changing.
> 
> element.hidden is not the same as element.style.visibility=hidden.
> element.hidden = true is similar to element.style.display = none.
> 
> (anyway, this is going way off-topic ;) )

Whoops, i meant "none" obviously. Sorry for the typo.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 577972 [details] [diff] [review]
Working patch

   get height() {
-    return Services.prefs.getIntPref("devtools.debugger.ui.height");
+    if (this._height < 0) {
+      this._height = Services.prefs.getIntPref("devtools.debugger.ui.height");
+    }
+    return this._height;

is this bit necessary for RTL? Not that I'm complaining...

     /**
      * Shows the element, setting the display style to "block".
      * @return object
      *         The same element.
      */
     element.show = function DVP_element_show() {
-      element.style.display = "block";
+      element.style.display = "-moz-box";
...
     element.expand = function DVP_element_expand() {
-      arrow.classList.remove("collapsed");
-      arrow.classList.add("expanded");
-      details.style.display = "block";
+      arrow.setAttribute("open", "");
+      details.style.display = "-moz-box";

is there a good reason for not doing this stuff in CSS directly?

You can have an attribute rule for [open="true"].

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (robcee) from comment #14)
> Comment on attachment 577972 [details] [diff] [review]
> Working patch
> 
>    get height() {
> -    return Services.prefs.getIntPref("devtools.debugger.ui.height");
> +    if (this._height < 0) {
> +      this._height =
> Services.prefs.getIntPref("devtools.debugger.ui.height");
> +    }
> +    return this._height;
> 
> is this bit necessary for RTL? Not that I'm complaining...

Nope, it's one of the simplifications I mention in comment 3.

>      /**
>       * Shows the element, setting the display style to "block".
>       * @return object
>       *         The same element.
>       */
>      element.show = function DVP_element_show() {
> -      element.style.display = "block";
> +      element.style.display = "-moz-box";
> ...
>      element.expand = function DVP_element_expand() {
> -      arrow.classList.remove("collapsed");
> -      arrow.classList.add("expanded");
> -      details.style.display = "block";
> +      arrow.setAttribute("open", "");
> +      details.style.display = "-moz-box";
> 
> is there a good reason for not doing this stuff in CSS directly?
> 
> You can have an attribute rule for [open="true"].

I'm not sure I understand this bit. We already do that, see the .arrow[open] rules. This is the code that toggles the attribute. What should be done in CSS directly?

Comment 16

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #15)
> (In reply to Rob Campbell [:rc] (robcee) from comment #14)
> >      element.expand = function DVP_element_expand() {
> > -      arrow.classList.remove("collapsed");
> > -      arrow.classList.add("expanded");
> > -      details.style.display = "block";
> > +      arrow.setAttribute("open", "");
> > +      details.style.display = "-moz-box";
> > 
> > is there a good reason for not doing this stuff in CSS directly?
> > 
> > You can have an attribute rule for [open="true"].
> 
> I'm not sure I understand this bit. We already do that, see the .arrow[open]
> rules. This is the code that toggles the attribute. What should be done in
> CSS directly?

Ah, ok.

What I meant was, in your arrow[open] rules:

+.arrow[open] {
+    -moz-appearance: treetwistyopen;
 }

you can add the display: -moz-box; there and take it out of js.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (robcee) from comment #16)
> (In reply to Panos Astithas [:past] from comment #15)
> > (In reply to Rob Campbell [:rc] (robcee) from comment #14)
> > >      element.expand = function DVP_element_expand() {
> > > -      arrow.classList.remove("collapsed");
> > > -      arrow.classList.add("expanded");
> > > -      details.style.display = "block";
> > > +      arrow.setAttribute("open", "");
> > > +      details.style.display = "-moz-box";
> > > 
> > > is there a good reason for not doing this stuff in CSS directly?
> > > 
> > > You can have an attribute rule for [open="true"].
> > 
> > I'm not sure I understand this bit. We already do that, see the .arrow[open]
> > rules. This is the code that toggles the attribute. What should be done in
> > CSS directly?
> 
> Ah, ok.
> 
> What I meant was, in your arrow[open] rules:
> 
> +.arrow[open] {
> +    -moz-appearance: treetwistyopen;
>  }
> 
> you can add the display: -moz-box; there and take it out of js.

Ah yes, that makes perfect sense.

Unfortunately, this patch is in the bottom of my patch queue, and there is the patch from bug 702939 that moves the CSS files around on top of it, among others. How about I change it along with your other review items in the updated patch in bug 697762, in order to avoid lots of rebasing?

Comment 18

[Rob Campbell [:rc] (:robcee)]

yeah, of course. :)

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Made the change in the patch you have reviewed (Part 2) in bug 697762.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 577972 [details] [diff] [review]
Working patch

looks like this has already been reviewed. Re-request if you need a third.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/75377889d954