Closed Bug 1246514 Opened 8 years ago Closed 8 years ago

Switch toolbox-options.xul to HTML

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ntim, Assigned: nchevobbe, Mentored)

References

Details

(Whiteboard: [lang=js][lang=html][btpp-backlog])

Attachments

(3 files, 1 obsolete file)

      No description provided.
The goal of this bug is to convert toolbox-options.xul to a standard HTML file.
Mentor: ntim.bugs
Whiteboard: [lang=js][lang=html]
I want to work on this bug.
(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.
Assignee: nobody → chevobbe.nicolas
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 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
Flags: needinfo?(jsantell)
Attachment #8727155 - Flags: review?(jsantell)
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.
Flags: needinfo?(jsantell)
Attachment #8727155 - Flags: review?(jsantell)
Mentor: ntim.bugs → bgrinstead
Priority: -- → P3
Whiteboard: [lang=js][lang=html] → [lang=js][lang=html][btpp-backlog]
Attachment #8727155 - Flags: review?(bgrinstead)
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
Attachment #8727155 - Flags: review?(bgrinstead)
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
>> 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
Flags: needinfo?(bgrinstead)
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.
(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?
Flags: needinfo?(bgrinstead)
Attachment #8727155 - Attachment is obsolete: true
> 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 ?
(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
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/
Attachment #8729697 - Flags: review?(bgrinstead)
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
Attachment #8729697 - Flags: review?(bgrinstead)
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
Attachment #8729698 - Flags: review?(bgrinstead) → review+
https://reviewboard.mozilla.org/r/39545/#review36721

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

yeah, not needed here
Attachment #8729697 - Flags: review?(bgrinstead)
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 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/
(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.
Attachment #8729697 - Flags: review?(bgrinstead) → review+
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
Keywords: checkin-needed
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
Flags: needinfo?(chevobbe.nicolas)
Keywords: checkin-needed
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 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/
(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 ?
Flags: needinfo?(chevobbe.nicolas) → needinfo?(cbook)
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 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/
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
Keywords: checkin-needed
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/2ddb3191c7f0
https://hg.mozilla.org/mozilla-central/rev/5c5cf183a608
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blocks: 1264436
Depends on: 1271063
No longer depends on: 1271063
See Also: → 1276126
Depends on: 1266142
Product: Firefox → DevTools
Depends on: 1519726
Blocks: 1607948
You need to log in before you can comment on or make changes to this bug.