Closed Bug 1318259 Opened 6 years ago Closed 6 years ago

Firebug theme - Adjust UI of markup view in Inspector panel

Categories

(DevTools :: General, defect, P2)

53 Branch
defect

Tracking

(firefox53 fixed)

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebo, Assigned: ruturaj, Mentored)

References

()

Details

(Keywords: good-first-bug)

Attachments

(4 files, 3 obsolete files)

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
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: nobody → ruturaj
Attached 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)
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
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)
- 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)
(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)
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)
Attached 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 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+
Attached 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 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+
- fixed Comments' whitespace nit.
Attachment #8817570 - Attachment is obsolete: true
Attachment #8817609 - Flags: review?(ntim.bugs)
Attachment #8817609 - Flags: review?(ntim.bugs) → review+
should I apply the checkin-needed ?
Flags: needinfo?(gl)
Keywords: checkin-needed
Thanks a lot Tim.
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
https://hg.mozilla.org/mozilla-central/rev/285af284df45
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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
It looks, as it must be in UI looks in Mozilla Nighty version..
Looks good in Nightly Version
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.