1246514 - Switch toolbox-options.xul to HTML

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P3)

Comment 1

[Tim Nguyen :ntim]

The goal of this bug is to convert toolbox-options.xul to a standard HTML file.

Comment 2

[Rahul Bhattacharjee]

I want to work on this bug.

Comment 3

[Tim Nguyen :ntim]

(In reply to Rahul Bhattacharjee from comment #2)
> I want to work on this bug.

Thanks for your interest Rahul, keep in mind this is not an easy bug.

Feel free to work on it, and needinfo me if you need any information.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r?

I changed the XUL label + vbox to fieldset + legend
as I think it's the most relevant for describing form
section. Moreover, we don't have to look for a previous
label when we hide a fieldset.
I had do edit a couple of tests to make them work.
On all the files I edited, I fixed eslint warning anors when necessary.
I ran the whole test suite with
```./mach test devtools/client/framework/test/*```
and they all pass.

Please tell me if something is wrong or missing here.

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8727155 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r?

jsantell: I'm requesting a review to you because you were in suggested reviewers with no pending reviews and ntim is currently busy and not accepting review requests. If you don't have the time to look after this, please tell me and I'll ask someone else.

Thank you

Comment 6

[Tim Nguyen :ntim]

Thanks for the patch! Sorry I won't have time to review this :(. :bgrins should be most appropriate reviewer, he'll be back on Monday, so you'll be able to send him a review request tomorrow.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8727155 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r?

https://reviewboard.mozilla.org/r/38363/#review35997

Thanks for taking this on.  A few general notes:

* Weird bug - when I have the dark theme applied and then switch to the options panel it switches to the light theme.

* Also, when I switch between light and dark theme the inputs in the right column shift some pixels horizontally (probably due to floating scrollbars being used in the dark theme).  This should be visible on Windows, or on OSX if you change 'Show scroll bars' in the settings to 'Always'.

* The current behavior on the xul doc is that text can't be selected for labels.  I guess let's match that with -moz-user-select: none on the root element in css

* For some reason some of the headers have a text cursor when I hover them.  If the user select change doesn't change that then we'll need to make sure the default cursor shows up on those section headers

* You don't need to split it up now, but in the future can you push the eslint cleanup stuff as a separate patch to the bug since it's easier to review them separately?

::: devtools/client/framework/options-panel.css:41
(Diff revision 1)
> +  margin-left: -15px;

Should be margin-inline-start instead of margin-left

::: devtools/client/framework/options-panel.css:62
(Diff revision 1)
> +  padding-left: 5px;

padding-inline-start instead of padding-left

::: devtools/client/framework/options-panel.css:74
(Diff revision 1)
> +  margin-left: 4px;

margin-inline-start instead of margin-left

::: devtools/client/framework/options-panel.css:90
(Diff revision 1)
> -  padding: 4px 0 0; /* To align it with the checkbox */
> +  padding: 4px 0 0 4px; /* To align it with the checkbox */

padding-inline-end instead of shorthand for left

::: devtools/client/framework/test/browser_toolbox_options.js:14
(Diff revision 1)
> -  const URL = "data:text/html;charset=utf8,test for dynamically registering and unregistering tools";
> +  const URL = "data:text/html;charset=utf8,test for dynamically registering " +

Nite: even if it doesn't pass eslint I'm not a fan of splitting hardcoded strings up like this.  I think there's been discussions about allowing this sort of pattern to exceed 80/100 char in the rule but don't think that's done.  So if you do split it, something like this would be a little better IMO:

    const URL = "data:text/html;charset=utf8," +
                "test for dynamically registering and unregistering tools";
              
Same comment goes for other tests below that do this

::: devtools/client/framework/toolbox-options.js:108
(Diff revision 1)
> +    yield this.populatePreferences();

Why'd this line need to move?

::: devtools/client/framework/toolbox-options.js:155
(Diff revision 1)
> +    enabledToolbarButtonsBox.innerHTML = enabledToolbarButtonsBox.querySelector(

I'm confused about this (even the previous code is confusing).  Why is the html from the legend being copied into this box?  It looks like this will destroy tools-not-supported-label

::: devtools/client/framework/toolbox-options.js:164
(Diff revision 1)
> -      Services.prefs.setBoolPref(toolDefinition.visibilityswitch, checkbox.checked);
> +        tool => tool.id === checkbox.getAttribute("id"))[0];

Changing from checkbox.id to checkbox.getAttribute("id") seems like it shouldn't be needed

::: devtools/client/framework/toolbox-options.xhtml:1
(Diff revision 1)
> +<?xml version="1.0" encoding="utf-8"?>

Technically, it would be nice if this was done as a 'move' command in hg / git so history is preserved (even though the entire file is basically changing).  If you've already done this and mozreview isn't showing it this way, then please ignore this comment.

To do so, you could revert the removal of toolbox-options.xul, then copy the contents of this file onto your clipboard, then do something like `hg|git mv devtools/client/framework/toolbox-options.xul devtools/client/framework/toolbox-options.xhtml` and paste the contents back into the moved file.


if there's anything at all that isn't changing  and then this content was pasted in

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/38363/#review35997

> Weird bug - when I have the dark theme applied and then switch to the options panel it switches to the light theme.

Must be because light-theme is the default and I'm probably doing something wrong.

> Also, when I switch between light and dark theme the inputs in the right column shift some pixels horizontally (probably due to floating scrollbars being used in the dark theme).  This should be visible on Windows, or on OSX if you change 'Show scroll bars' in the settings to 'Always'.

Good to know. I'm on OSX and have floating toolbars, so I don't see this

> You don't need to split it up now, but in the future can you push the eslint cleanup stuff as a separate patch to the bug since it's easier to review them separately?

I will. Sorry about that.

> Why'd this line need to move?

Originally, I moved it because of the `populatePreferences`function was adding event listeners to all the radio inputs, and as I now handle the creation of the themes input radio to `setupThemeList`, I did not wanted to have the listeners added twice.
But after, I did change the querySelectorAll query string in `populatePreferences` to ignore those radios, and forgot to revert this line move.

> I'm confused about this (even the previous code is confusing).  Why is the html from the legend being copied into this box?  It looks like this will destroy tools-not-supported-label

Yet, as you pointed out, it will indeed destroy the label. I may move it outside the fieldset or take care of it during the clear the process.
Since the <legend> lives in <fieldset>, when we want to clear the <fieldset> content, we should make sure we're keeping the legend.
Does it bother you ? Does creating a `function clearFieldset(fieldset)` would make it less confusing ?

> Technically, it would be nice if this was done as a 'move' command in hg / git so history is preserved (even though the entire file is basically changing).  If you've already done this and mozreview isn't showing it this way, then please ignore this comment.
> 
> To do so, you could revert the removal of toolbox-options.xul, then copy the contents of this file onto your clipboard, then do something like `hg|git mv devtools/client/framework/toolbox-options.xul devtools/client/framework/toolbox-options.xhtml` and paste the contents back into the moved file.
> 
> 
> if there's anything at all that isn't changing  and then this content was pasted in

I'll try that

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

>> I'm confused about this (even the previous code is confusing).  Why is the html from the legend being copied into this box?  It looks like this will destroy tools-not-supported-label

> Yet, as you pointed out, it will indeed destroy the label. I may move it outside the fieldset or take care >of it during the clear the process.
> Since the <legend> lives in <fieldset>, when we want to clear the <fieldset> content, we should make sure > we're keeping the legend.
> Does it bother you ? Does creating a `function clearFieldset(fieldset)` would make it less confusing ?

needinfo on this

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: vertical-align-options.png

Screenshot of slight vertical alignment issue on Win7.  All the labels look slightly below the input fields.  Also, I've confirmed the 'shifting' issue happens on Windows when switching themes.  Guessing there's some flex property that can be set to fix it.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe from comment #9)
> >> I'm confused about this (even the previous code is confusing).  Why is the html from the legend being copied into this box?  It looks like this will destroy tools-not-supported-label
> 
> > Yet, as you pointed out, it will indeed destroy the label. I may move it outside the fieldset or take care >of it during the clear the process.
> > Since the <legend> lives in <fieldset>, when we want to clear the <fieldset> content, we should make sure > we're keeping the legend.
> > Does it bother you ? Does creating a `function clearFieldset(fieldset)` would make it less confusing ?
> 
> needinfo on this

If I'm understanding properly, we are wanting to clear out the existing controls in the fieldset.  However since this is called on initialization, I don't think there would be anything that needs to be cleared out.  So, if you remove the innerHTML setting does everything still work?  If not, could we querySelectorAll for 'label' or whatever the top-level input controls are and remove each one?

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

> Also, when I switch between light and dark theme the inputs in the right column shift some pixels horizontally (probably due to floating scrollbars being used in the dark theme).  This should be visible on Windows, or on OSX if you change 'Show scroll bars' in the settings to 'Always'.

This one was already existing before my patch. Because the panel are set relative to the width ( 100%/3 - 10px ), the floating scrollbar from the dark theme causes the panel to be slightly wider, hence this bug.
Except using floating bar in the light theme, I can't see an easy fix for this. Maybe we could move it to a proper bug ?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe from comment #12)
> > Also, when I switch between light and dark theme the inputs in the right column shift some pixels horizontally (probably due to floating scrollbars being used in the dark theme).  This should be visible on Windows, or on OSX if you change 'Show scroll bars' in the settings to 'Always'.
> 
> This one was already existing before my patch. Because the panel are set
> relative to the width ( 100%/3 - 10px ), the floating scrollbar from the
> dark theme causes the panel to be slightly wider, hence this bug.
> Except using floating bar in the light theme, I can't see an easy fix for
> this. Maybe we could move it to a proper bug ?

OK I guess I just hadn't seen this before.  Let's move that work into another bug

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins

Change the XUL label + vbox to fieldset + legend as it's the most relevant
element for describing form sections.
Moreover, we don't have to look for a previous label when we hide a fieldset.

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

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1246514 - Fix eslint errors in files touched in this bug. r=bgrins

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

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8729697 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins

https://reviewboard.mozilla.org/r/39545/#review36721

This is looking great.. Let's see how Try likes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24b944b956a

::: devtools/client/framework/toolbox-options.xhtml:13
(Diff revision 1)
>  ]>
> -<?xml-stylesheet rel="stylesheet" href="chrome://global/skin/" type="text/css"?>
> -<?xml-stylesheet rel="stylesheet" href="chrome://devtools/content/framework/options-panel.css" type="text/css"?>
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +  <head>
> +    <title>Toolbox option</title>
> +    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
> +    <link rel="stylesheet" href="chrome://global/skin/" type="text/css"/>

I don't think we need this global/skin link anymore

::: devtools/client/framework/toolbox-options.xhtml:17
(Diff revision 1)
> +    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
> +    <link rel="stylesheet" href="chrome://global/skin/" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://devtools/content/framework/options-panel.css" type="text/css"/>
> +    <script type="application/javascript;version=1.8" src="chrome://devtools/content/shared/theme-switching.js"/>
> +  </head>
> +  <body role="application" empty="true">

What's up with the 'empty=true' attribute?

::: devtools/client/framework/toolbox-options.xhtml:18
(Diff revision 1)
> +    <link rel="stylesheet" href="chrome://global/skin/" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://devtools/content/framework/options-panel.css" type="text/css"/>
> +    <script type="application/javascript;version=1.8" src="chrome://devtools/content/shared/theme-switching.js"/>
> +  </head>
> +  <body role="application" empty="true">
> +  <form id="options-panel" class="theme-body">

Please put theme-body on the <body> tag

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8729698 [details]
MozReview Request: Bug 1246514 - Fix eslint errors in files touched in this bug. r=bgrins

https://reviewboard.mozilla.org/r/39547/#review36727

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/39545/#review36721

> What's up with the 'empty=true' attribute?

yeah, not needed here

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8729697 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39545/diff/1-2/

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8729698 [details]
MozReview Request: Bug 1246514 - Fix eslint errors in files touched in this bug. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39547/diff/1-2/

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8729697 [details]
> MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins
> 
> https://reviewboard.mozilla.org/r/39545/#review36721
> 
> This is looking great.. Let's see how Try likes it:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24b944b956a
> 

The jobs are over. There are dt* errors but they don't seem related to my patch.

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8729697 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins

https://reviewboard.mozilla.org/r/39545/#review37051

Let's ship it

Comment 23

[Carsten Book [:Tomcat]]

failed to apply:

patching file devtools/client/jar.mn
Hunk #1 FAILED at 104
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/jar.mn.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8729697 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39545/diff/2-3/

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8729698 [details]
MozReview Request: Bug 1246514 - Fix eslint errors in files touched in this bug. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39547/diff/2-3/

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Carsten Book [:Tomcat] from comment #23)
> failed to apply:
> 
> patching file devtools/client/jar.mn
> Hunk #1 FAILED at 104
> 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/jar.mn.rej
> patch failed to apply
> abort: fix up the working directory and run hg transplant --continue

I've pulled, rebased and pushed again : https://reviewboard.mozilla.org/r/39545/diff/2-3/
Is it okay or do I need to do something more ?

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8729697 [details]
MozReview Request: Bug 1246514 - Switch toolbox-options.xul to HTML. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39545/diff/3-4/

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8729698 [details]
MozReview Request: Bug 1246514 - Fix eslint errors in files touched in this bug. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39547/diff/3-4/

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Other patches landed in the meantime , pulled and rebased again.
I applied the 2 patches on the fresh repository and this time they both applied successfully.

Let me know if something is wrong

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2ddb3191c7f0
https://hg.mozilla.org/integration/fx-team/rev/5c5cf183a608

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ddb3191c7f0
https://hg.mozilla.org/mozilla-central/rev/5c5cf183a608