1015627 - Inspector markup preview doesn't match devtools theme

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tim Nguyen :ntim]

STR :
- Enable the pref : devtools.inspector.markupPreview
- Open inspector on a page where HTML source is long enough for markup preview

The inspector markup preview is currently fully dark whatever the theme.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8430717 [details] [diff] [review]
Patch

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

::: browser/devtools/markupview/markup-view.css
@@ +140,5 @@
>    border-bottom: 1px solid #333;
>    overflow: hidden;
>  }
>  
> +.theme-light #previewbar {

I know most of these are already issues, but I'm going to suggest some improvements:

1) Please move all of the #previewbar and #preview stuff into themes/shared/devtools/markup-view.css.
2) Use colors from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors (probably Tab Toolbar for background and Splitters for border)
3) Move the colors from #previewbar { .. } into .theme-dark #previewbar {} so that it we have similar definitions as in .theme-light #previewbar {}.

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch v2

Addressed review comments

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8430795 [details] [diff] [review]
Patch v2

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

Looks good to me, please update with the minor notes then you can reupload with r+

::: browser/themes/shared/devtools/markup-view.css
@@ +40,5 @@
> +  position: fixed;
> +  top: 0;
> +  right: 0;
> +  width: 90px;
> +  background: black;

You can remove background:black here

@@ +47,5 @@
> +  overflow: hidden;
> +}
> +
> +.theme-dark #previewbar {
> +  background: #252c33;

Please add comments that say /* Tab Toolbar */ and /* Splitters */ after each color used in these two rules

Comment 5

[Tim Nguyen :ntim]

Attachment: Patch v2.1 (r=bgrins)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0a7752f9805a

Landed without a Try run since it was a css-only change. That said, please try to use commit messages that explain what the patch is doing instead of restating the problem it's solving. Thanks :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 7

[Tim Nguyen :ntim]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> https://hg.mozilla.org/integration/fx-team/rev/0a7752f9805a
> 
> Landed without a Try run since it was a css-only change. That said, please
> try to use commit messages that explain what the patch is doing instead of
> restating the problem it's solving. Thanks :)
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Committing_Rules_and_Responsibilities#Checkin_comment

Sorry, I'll keep that in mind next time :/

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a7752f9805a