Update layout of extensions in about:debugging

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: about:debugging
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

8 months ago
There is a new design for the about:debugging add-ons tab since we are adding more options to this page it will get cluttered quickly.

The current options that are available now should be updated to the new layout so that more options can be added.

Mocks: https://mozilla.invisionapp.com/share/RUA0FEJSW#/screens/220703851

Updated

8 months ago
Component: Developer Tools: Debugger → Developer Tools: about:debugging
(Assignee)

Comment 1

7 months ago
Created attachment 8846825 [details]
reload-button-states.mov

Emanuela, a few questions (unfortunately the invision doc doesn't seem to let me zoom or show colours):

1. What are the colours for the buttons and divider?
2. What should the hover/click interaction be? I made it the same as the link up top in the attached video (colours too).
3. Is there a shadow on the box?
Flags: needinfo?(emanuela)

Comment 2

7 months ago
Hi Mark,

excellent questions! I mix the answer a bit, but it should answer all your doubts. 


#1 Colours of different links/buttons states

normal, :visited {color: #0087ff} 
:hover, :focus {color: #0052cc, text-decoration: underline}
:active { color: #003399}
:disabled {color: #6e6e6e }

It could be nice to add some transition between the different states.


#2 Divider colour

rgba(0, 0, 0, 0.2)


#3 Box Shadow

Yes, there is!

box-shadow: 0 0 1px rgba(0,0,0,0.12);


That's because I was thinking it could be nice to add a smooth hover effect that transform that box shadow in something just a bit heavier [box-shadow: 0 1px 2px rgba(0,0,0,0.24);] but I'm not too sure now. 


----

Btw, it looks already quite ok! Great job, Mark!
Flags: needinfo?(emanuela)
(Assignee)

Comment 3

7 months ago
Created attachment 8847222 [details]
about-debugging-styles.mov

Here's how it looks with the updated styles. I'll have the patch up soon, let me know how it looks, Emanuela.
Attachment #8846825 - Attachment is obsolete: true
Flags: needinfo?(emanuela)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

7 months ago
mozreview-review
Comment on attachment 8847259 [details]
Bug 1344016 - Use variables for updated styles

https://reviewboard.mozilla.org/r/120278/#review122186

::: commit-message-7bf94:1
(Diff revision 1)
> +Bug 1344016 - Use variables for updated styles r?jdescottes

This commit updates the styles to use CSS variables. I saw they were used in other parts of this page and I would have used variables in SCSS so I thought I'd give it a shot.

It probably doesn't improve readability since they're so verbose which is too bad but it would prevent tweaking one value and having the alignment go off.
(Assignee)

Comment 7

7 months ago
mozreview-review
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review122188

::: devtools/client/aboutdebugging/aboutdebugging.css:200
(Diff revision 1)
>  
>  .error-page .error-page-details {
>    color: gray;
>  }
> +
> +.AddonTarget {

I'm not sure what sort of class name convention is used here (I see they're all lowercase right now though) so I went with what we used on addons-frontend where you include the component name as a prefix.

Happy to change it to something else though if that's more consistent.

Comment 8

7 months ago
mozreview-review
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review123036

Thanks for the patch Mark!

Tests are failing due to classname and markup changes, so this needs to be fixed.
Let's try to stick to classes already used in the other categories as much as possible.

In the long run we can't really have one category using this new style while the worker & tabs categories are using another one.

::: devtools/client/aboutdebugging/aboutdebugging.css:200
(Diff revision 1)
>  
>  .error-page .error-page-details {
>    color: gray;
>  }
> +
> +.AddonTarget {

Let's stick to lowercase classnames AddonTarget-* -> addon-target-*

::: devtools/client/aboutdebugging/aboutdebugging.css:202
(Diff revision 1)
>    color: gray;
>  }
> +
> +.AddonTarget {
> +  background: #fff;
> +  box-shadow: 0 0 1px rgba(0,0,0,0.12);

nit: consistency: add spaces after commas rgba(0, 0 ,0, 0.12)

::: devtools/client/aboutdebugging/aboutdebugging.css:210
(Diff revision 1)
> +  padding: 4px 16px;
> +  transition: box-shadow 150ms;
> +}
> +
> +.AddonTarget:hover {
> +  box-shadow: 0 1px 2px rgba(0,0,0,0.24);

nit: consistency: add spaces after commas rgba(0, 0 ,0, 0.24)

::: devtools/client/aboutdebugging/components/addons/target.js:65
(Diff revision 1)
>  
>      return dom.li(
> -      { className: "target-container", "data-addon-id": target.addonID },
> +      { className: "AddonTarget", "data-addon-id": target.addonID },
> +      dom.div({ className: "AddonTarget-header" },
> -      dom.img({
> +        dom.img({
> -        className: "target-icon",
> +          className: "AddonTarget-icon",

Why not keep the target-icon class here? 

The only property that needs to be updated is the margin-inline-end. We could simply have a rule :

.addon-target .target-icon {
  margin-inline-end: var(--AddonTarget-padding);
}

instead of duplicating all the rules for .target-icon

::: devtools/client/aboutdebugging/components/addons/target.js:69
(Diff revision 1)
> -      dom.img({
> +        dom.img({
> -        className: "target-icon",
> +          className: "AddonTarget-icon",
> -        role: "presentation",
> +          role: "presentation",
> -        src: target.icon
> +          src: target.icon
> -      }),
> +        }),
> -      dom.div({ className: "target" },
> +        dom.span({ className: "AddonTarget-name", title: target.name }, target.name)

our tests rely on the .target-name selector. I think you could just reuse the styles from our existing .target-name class and add your additional styles with .addon-target .target-name

You can run our tests by doing ./mach test devtools/client/aboutdebugging/test

::: devtools/client/aboutdebugging/components/addons/target.js:72
(Diff revision 1)
> -      }),
> +        }),
> -      dom.div({ className: "target" },
> +        dom.span({ className: "AddonTarget-name", title: target.name }, target.name)
> -        dom.div({ className: "target-name", title: target.name }, target.name)
>        ),
>        dom.button({
> -        className: "debug-button",
> +        className: "AddonTarget-debug-button AddonTarget-button",

our tests rely on the .debug-button, .reload-button selectors. Can we keep non addon specific classnames here?

You can run our tests by doing ./mach test devtools/client/aboutdebugging/test
Attachment #8847258 - Flags: review?(jdescottes)
(Assignee)

Comment 9

7 months ago
mozreview-review-reply
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review123036

Would you like me to update the worker and tabs categories in this patch? Looks like they would fit the mocks [1] fairly well.

[1] https://mozilla.invisionapp.com/share/RUA0FEJSW#/screens/220706957
Comment hidden (mozreview-request)

Comment 11

7 months ago
(In reply to Mark Striemer [:mstriemer] from comment #3)
> Created attachment 8847222 [details]
> about-debugging-styles.mov
> 
> Here's how it looks with the updated styles. I'll have the patch up soon,
> let me know how it looks, Emanuela.

Hi Mark, it looks great to me! 
One minor: the divider seems a bit too dark from the video, but I don't mind :)

Awesome :)
Flags: needinfo?(emanuela)

Comment 12

7 months ago
mozreview-review
Comment on attachment 8848640 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/121528/#review124864

Tests are now passing, thanks :)

Can you please fold this with the other patches.
Attachment #8848640 - Flags: review?(jdescottes) → review+

Comment 13

7 months ago
mozreview-review
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review124866

I'll r+ this one once it's folded with your last patch.

Comment 14

7 months ago
mozreview-review
Comment on attachment 8847259 [details]
Bug 1344016 - Use variables for updated styles

https://reviewboard.mozilla.org/r/120278/#review123052

In this particular case I'd prefer to avoid variables. As you commented, it's too verbose to be easily understood. 
Since this stylesheet is only used in one screen at the moment I think testing for UI regressions should be straightforward.
Attachment #8847259 - Flags: review?(jdescottes) → review-
Created attachment 8851962 [details]
about_debugging_disabled_links.png

I was originally very confused about this screen. 

When all the links are disabled, the gray color used for disabled links feels too dark to really tell the user they are disabled. Probably because there are no enabled links next to it to give a point of comparison. I was hovering and clicking for a few seconds before I remembered that I had to click on the checkbox above the list to enable addon debugging.

What do you think about this?
Attachment #8851962 - Flags: feedback?(emanuela)

Comment 16

7 months ago
Hey Julian,

thank you for the screenshot. I think you're right here.

Let's use #999 as a color for the disabled link.

Comment 17

7 months ago
Comment on attachment 8851962 [details]
about_debugging_disabled_links.png

Change color from #6e6e6e to #999999
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8847258 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8847259 - Attachment is obsolete: true
(Assignee)

Comment 19

7 months ago
Created attachment 8855065 [details]
about-debugging.png

I lightened the divider from rgba(0,0,0,0.2) to rgba(0,0,0,0.1) and it looks much closer to the mocks. Also lightened the disabled button colour.

CSS variables have been removed and commits were folded into one.

Attached a screenshot of how it looks now.
Attachment #8847222 - Attachment is obsolete: true
Attachment #8851962 - Attachment is obsolete: true
Attachment #8851962 - Flags: feedback?(emanuela)

Comment 20

7 months ago
Hi Mark,

I like how it looks, just afraid it may be too light for not HD screens :)

The name of the extensions seems a bit uncentered. Do we have a stanging or something? Where I can see it in action? It could be something simple like setting a different line-height.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

6 months ago
Created attachment 8855349 [details]
about-debugging.png

I centred the text and made the divider rgba(0,0,0,0.2) again.
Attachment #8855065 - Attachment is obsolete: true
(Assignee)

Comment 23

6 months ago
We're you wanting to look this over again, Julian? I see that reviewboard is still showing r+ even though the commit updated.
Flags: needinfo?(jdescottes)
Reviewboard will keep forwarding the r+ as soon as it's been given once for a changeset. 
If the changes are significant enough to require a new review, you have to reset the flag yourself in bugzilla :)

In this case I think it's ok to proceed. 
Do you need me to change your last changeset to try or can you do it?
Flags: needinfo?(jdescottes)
Flags: needinfo?(mstriemer)
> Do you need me to change your last changeset to try or can you do it?

I meant "push your last changeset", sorry.
(Assignee)

Comment 26

6 months ago
I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7e9efeef45f0dbb65f41ccba5270c7ca68b41a to try.
Flags: needinfo?(mstriemer)
(Assignee)

Comment 27

6 months ago
Try build looks fine to me so I'm marking this checkin-needed.
Keywords: checkin-needed

Comment 28

6 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a911151c924
Update styling of about:debugging for add-ons r=jdescottes
Keywords: checkin-needed

Comment 29

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a911151c924
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 30

6 months ago
I have reproduced this issue with Nightly 54.0a1 (2017-03-02) on Windows 10, 64 bit!

The fix is now verified on Latest Nightly 55.0a1

Build ID 	20170412030252
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170412]
You need to log in before you can comment on or make changes to this bug.