1318259 - Firebug theme - Adjust UI of markup view in Inspector panel

Status: VERIFIED FIXED

(DevTools :: General, defect, P2)

Description

[Sebastian Zartner [:sebo]]

Attachment: Firebug's HTML in comparison with DevTools' Inspector panel

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

[Erik Krause]

The color differences impact readability. Otherwise this wouldn't be important. But the DevTools version is far harder to read.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Honza and Helen, any thoughts on priority here?

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 4

[Ruturaj Vartak]

Attachment: fix-1318259-1.patch

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)

Comment 5

[Sebastian Zartner [:sebo]]

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

Comment 6

[Ruturaj Vartak]

Attachment: fix-1318259-2.patch

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

Comment 7

[Ruturaj Vartak]

Attachment: 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 :) )
- The DOCTYPE italics should be enabled for other light and dark theme as well, looks better.

Comment 8

[Sebastian Zartner [:sebo]]

(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

Comment 9

[Patrick Brosset <:pbro>]

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.

Comment 10

[Patrick Brosset <:pbro>]

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.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Gabriel, please see comment #11

Honza

Comment 13

[Ruturaj Vartak]

Attachment: fix-1318259-3.patch

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)

Comment 14

[Tim Nguyen :ntim]

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 }

Comment 15

[Ruturaj Vartak]

Attachment: fix-1318259-4.patch

(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

Comment 16

[Tim Nguyen :ntim]

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% */

Comment 17

[Ruturaj Vartak]

Attachment: fix-1318259-5.patch

- fixed Comments' whitespace nit.

Comment 18

[Ruturaj Vartak]

should I apply the checkin-needed ?

Comment 19

[Ruturaj Vartak]

Thanks a lot Tim.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/285af284df45
Firebug theme - Adjust UI of markup view in Inspector panel. r=ntim

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/285af284df45

Comment 22

[Sebastian Zartner [:sebo]]

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

Comment 23

[SriSailesh]

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

Comment 24

[66makilan66]

Looks good in Nightly Version