Firebug theme - Adjust UI of markup view in Inspector panel

VERIFIED FIXED in Firefox 53

Status

defect
P2
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: sebo, Assigned: ruturaj, Mentored)

Tracking

({good-first-bug})

53 Branch
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Using the Firebug theme the UI of the Inspector panel still has some differences in comparison to the Firebug extension. See the attached screenshot for comparison.
Parts of the differences were noted in the Firebug discussion group[1]. There was also a link posted to another comparing screenshot[2].

Here's a list of differences, which should be adjusted:

- Background color is slightly darker
- Tag names are bold
- Tag brackets and attributes are pale and use different colors
- Comments use a lighter green

Sebastian

[1] https://groups.google.com/d/msg/firebug/8ICvaq6gFhs/EQSzF2MlAAAJ
[2] http://ge.tt/9X6xr8h2

Comment 1

2 years ago
The color differences impact readability. Otherwise this wouldn't be important. But the DevTools version is far harder to read.
Honza and Helen, any thoughts on priority here?
Flags: needinfo?(odvarko)
Flags: needinfo?(hholmes)
Thanks for the report Sebastian.

I am setting P3 for this. Helen feel free to change it.

But, this might be fairly simple to fix. It looks like a few CSS changes so, I am marking this as good-first-bug.

If anyone is interested in working on this, here is the CSS file with styles for the markup view & Firebug theme:
https://dxr.mozilla.org/mozilla-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/devtools/client/themes/markup.css#291

Honza
Mentor: odvarko
Flags: needinfo?(odvarko)
Keywords: good-first-bug
Priority: -- → P3
Summary: Firebug theme - Adjust UI of node view in Inspector panel → Firebug theme - Adjust UI of markup view in Inspector panel
(Assignee)

Updated

2 years ago
Assignee: nobody → ruturaj
(Assignee)

Comment 4

2 years ago
Posted patch fix-1318259-1.patch (obsolete) — Splinter Review
Following are the changes:

- markup font synced to 11px for Linux which was at 80% of 11px (11px is hardcoded size)
- tag's font-weight to normal
Firebug theme
- background color synced to white (#fff)
- theme comment color synced to DarkGreen (from orignal firebug extension)
- --theme-highlight-purple changed to navy (#000080)
- --theme-highlight-red changed to red (#f00)
Attachment #8814608 - Flags: review?(odvarko)
(Reporter)

Comment 5

2 years ago
I didn't try out the patch, though the changes seem to be correct. Looking at the panel again, I just realized two more differences regarding the Firebug theme:

- Doctype should be gray (#787878) and italic
- Hidden elements' opacity should be lower (0.5)

Ruturaj, could you please change that, too?

Sebastian
(Assignee)

Comment 6

2 years ago
Changes:
- markup font synced to 11px for Linux which was at 80% of 11px (11px is hardcoded size)
- tag's font-weight to normal
Firebug theme
- background color synced to white (#fff)
- theme comment color synced to DarkGreen (from orignal firebug extension)
- --theme-highlight-purple changed to navy (#000080)
- --theme-highlight-red changed to red (#f00)

New Modifications as per :sebo suggestions
- Doctype color changed to gray (#787878) and italic
- Hidden elements' opacity lowered to 0.5
Attachment #8814608 - Attachment is obsolete: true
Attachment #8814608 - Flags: review?(odvarko)
Attachment #8814642 - Flags: review?(odvarko)
(Assignee)

Comment 7

2 years ago
- The opacity of 0.5 looks too difficult to read as opposed to 0.7 (Typically for cheaper monitors also read non-retina or non-mac :) )
- The DOCTYPE italics should be enabled for other light and dark theme as well, looks better.
Flags: needinfo?(sebastianzartner)
(Reporter)

Comment 8

2 years ago
(In reply to Ruturaj Vartak from comment #7)
> Created attachment 8814644 [details]
> non-displayed-opacity.png
> 
> - The opacity of 0.5 looks too difficult to read as opposed to 0.7
> (Typically for cheaper monitors also read non-retina or non-mac :) )

I am not a visually impaired person, but I guess it might be illegible for some people.
Though I mentioned it because of two reasons:
1. The difference between opacity: 1; and opacity: 0.7; may not be visible enough.
2. opacity: 0.5; is nearer to Firebug's display (and I can't remember any complaints that the opacity was too low in Firebug.)

> - The DOCTYPE italics should be enabled for other light and dark theme as
> well, looks better.

I agree.

Helen, it would be great to hear your opinion regarding the above two points.

Sebastian
Flags: needinfo?(sebastianzartner)
(Assignee)

Updated

2 years ago
Attachment #8814642 - Flags: review?(pbrosset)
Comment on attachment 8814642 [details] [diff] [review]
fix-1318259-2.patch

Honza is the mentor on this bug, I'll let him review this change. Doesn't seem like 2 reviewers are needed.
Attachment #8814642 - Flags: review?(pbrosset)
I would like to change the priority to P2 here. I'm planning to dedicate some more time in Q1 on Firebug "gaps", and this work fits nicely into this.
Flags: needinfo?(hholmes)
Priority: P3 → P2
Comment on attachment 8814642 [details] [diff] [review]
fix-1318259-2.patch

Review of attachment 8814642 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, just one inline comment.

Thanks for working on this!

Honza

::: devtools/client/themes/markup.css
@@ +336,5 @@
>    color: var(--theme-selection-color);
>  }
>  
> +/* Applicable to the DOCTYPE */
> +.theme-firebug .comment .tag {

This CSS selector seems to be a bit risky. Could we introduce a new 'doctype' class  on the element?

It looks like we could append 'doctype' class here:
https://dxr.mozilla.org/mozilla-central/rev/6f39c69810f258b4108f8ee88048c5b690a503a2/devtools/client/inspector/markup/views/read-only-editor.js#23

Honza
Attachment #8814642 - Flags: review?(odvarko)
@Gabriel, please see comment #11

Honza
Flags: needinfo?(gl)
(Assignee)

Comment 13

2 years ago
Posted patch fix-1318259-3.patch (obsolete) — Splinter Review
Based on :Honza's suggestion

- added doctype class
- also taken liberty to set doctypes to italics for all themes (let me know if I should do it in other bug)
Attachment #8816649 - Flags: review?(odvarko)

Comment 14

2 years ago
Comment on attachment 8816649 [details] [diff] [review]
fix-1318259-3.patch

Review of attachment 8816649 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/common.css
@@ +35,5 @@
>    font-size: 80%;
>  }
>  
> +:root[platform="linux"].theme-firebug .devtools-monospace {
> +  font-size: 11px; /*Font becomes too small for linux with 80%*/

Does the firebug theme use a different monospace font than the light/dark theme ? If that's not the case, can you change 80% above to 11px?

::: devtools/client/themes/markup.css
@@ +338,5 @@
>  
> +/* Applicable to the DOCTYPE */
> +.doctype {
> +  font-style: italic;
> +}

nit: add a new line after }
Attachment #8816649 - Flags: feedback+
(Assignee)

Comment 15

2 years ago
Posted patch fix-1318259-4.patch (obsolete) — Splinter Review
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> ::: devtools/client/themes/common.css
> @@ +35,5 @@
> >    font-size: 80%;
> >  }
> >  
> > +:root[platform="linux"].theme-firebug .devtools-monospace {
> > +  font-size: 11px; /*Font becomes too small for linux with 80%*/
> 
> Does the firebug theme use a different monospace font than the light/dark
> theme ? If that's not the case, can you change 80% above to 11px?

Problem is the firebug theme's :root starts at font-size:11px [1] while for other themes it starts with font: message-box [2]. Which leads to devtools' monospace font being even smaller.

[1] - https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/firebug-theme.css#11
[2] - https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#10
Attachment #8816649 - Attachment is obsolete: true
Attachment #8816649 - Flags: review?(odvarko)
Attachment #8817570 - Flags: review?(ntim.bugs)

Comment 16

2 years ago
Comment on attachment 8817570 [details] [diff] [review]
fix-1318259-4.patch

Review of attachment 8817570 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/common.css
@@ +35,5 @@
>    font-size: 80%;
>  }
>  
> +:root[platform="linux"].theme-firebug .devtools-monospace {
> +  font-size: 11px; /*Font becomes too small for linux with 80%*/

nit: Can you add spaces between /* and */ ?
eg. /* Font becomes too small for linux with 80% */
Attachment #8817570 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 17

2 years ago
- fixed Comments' whitespace nit.
Attachment #8817570 - Attachment is obsolete: true
Attachment #8817609 - Flags: review?(ntim.bugs)

Updated

2 years ago
Attachment #8817609 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 18

2 years ago
should I apply the checkin-needed ?
Flags: needinfo?(gl)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

2 years ago
Thanks a lot Tim.

Comment 20

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/285af284df45
Firebug theme - Adjust UI of markup view in Inspector panel. r=ntim
Keywords: checkin-needed
See Also: → 1319079

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/285af284df45
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Reporter)

Comment 22

2 years ago
Looks very good. Thank you for the changes! I can confirm that the UI looks as expected in Nightly 53.0a1 2016-12-17.

Sebastian
Status: RESOLVED → VERIFIED

Comment 23

2 years ago
It looks, as it must be in UI looks in Mozilla Nighty version..

Comment 24

2 years ago
Looks good in Nightly Version

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.