Make the script debugger UI look right on RTL

RESOLVED FIXED

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
This will probably require a conversion of debugger.xhtml to xul, as we had to do for the style inspector in bug 700036.
Created attachment 576941 [details] [diff] [review]
WIP

RTL issues are fixed, but I broke everything else. CSS tweaking is coming next.
Assignee: nobody → past
Status: NEW → ASSIGNED
Created attachment 577342 [details] [diff] [review]
WIP 2

Fixed some brokenness, more to come. Also incorporated a couple of simplifications Mihai suggested in bug 697762.
Attachment #576941 - Attachment is obsolete: true
Duplicate of this bug: 700624
Created attachment 577972 [details] [diff] [review]
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.
Attachment #577342 - Attachment is obsolete: true
Attachment #577972 - Flags: review?(dcamp)
Component: Developer Tools → Developer Tools: Debugger
QA Contact: developer.tools → developer.tools.debugger
Blocks: 706506

Updated

6 years ago
Attachment #577972 - Flags: review?(dcamp) → review+
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.
Attachment #577972 - Flags: review?(rcampbell)
Attachment #577972 - Flags: review?(mihai.sucan)
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 ?
Attachment #577972 - Flags: review?(mihai.sucan) → review+
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?
(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.
(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.
(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.
(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 ;) )
(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 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"].
(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?
(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.
(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?
yeah, of course. :)
Made the change in the patch you have reviewed (Part 2) in bug 697762.
Blocks: 697762
Blocks: 702939
Comment on attachment 577972 [details] [diff] [review]
Working patch

looks like this has already been reviewed. Re-request if you need a third.
Attachment #577972 - Flags: review?(rcampbell)
https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/75377889d954
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.