Switch toolbox-options.xul to HTML

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: Framework
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ntim, Assigned: nchevobbe, Mentored)

Tracking

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Comment 1

2 years ago
The goal of this bug is to convert toolbox-options.xul to a standard HTML file.
Mentor: ntim.bugs@gmail.com
Whiteboard: [lang=js][lang=html]

Comment 2

2 years ago
I want to work on this bug.
(Reporter)

Comment 3

2 years ago
(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)

Updated

2 years ago
Assignee: nobody → chevobbe.nicolas
(Assignee)

Comment 4

2 years ago
Created attachment 8727155 [details]
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/
(Assignee)

Comment 5

2 years ago
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)
(Reporter)

Comment 6

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8727155 - Flags: review?(jsantell)
Mentor: ntim.bugs@gmail.com → bgrinstead@mozilla.com
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)
(Assignee)

Comment 8

2 years ago
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
(Assignee)

Comment 9

2 years ago
>> 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)
Created attachment 8729254 [details]
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.
(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)
(Assignee)

Updated

2 years ago
Attachment #8727155 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
> 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
(Assignee)

Comment 14

2 years ago
Created attachment 8729697 [details]
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/
Attachment #8729697 - Flags: review?(bgrinstead)
(Assignee)

Comment 15

2 years ago
Created attachment 8729698 [details]
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/
Attachment #8729698 - 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+
(Assignee)

Comment 18

2 years ago
https://reviewboard.mozilla.org/r/39545/#review36721

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

yeah, not needed here
(Assignee)

Updated

2 years ago
Attachment #8729697 - Flags: review?(bgrinstead)
(Assignee)

Comment 19

2 years ago
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/
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
(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
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
(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)
(Assignee)

Comment 27

2 years ago
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/
(Assignee)

Comment 28

2 years ago
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/
(Assignee)

Comment 29

2 years ago
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

Comment 30

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/2ddb3191c7f0
https://hg.mozilla.org/integration/fx-team/rev/5c5cf183a608
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Flags: needinfo?(cbook)

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ddb3191c7f0
https://hg.mozilla.org/mozilla-central/rev/5c5cf183a608
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

2 years ago
Blocks: 1264436

Updated

2 years ago
Depends on: 1271063

Updated

2 years ago
No longer depends on: 1271063

Updated

2 years ago
See Also: → bug 1276126

Updated

a year ago
Duplicate of this bug: 1197627

Updated

a year ago
Depends on: 1266142
You need to log in before you can comment on or make changes to this bug.