1279703 - The tooltip for the font-family uses the wrong image with the Firebug theme.

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[johngraciliano]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160407164938

Steps to reproduce:


Start Firefox 48 or later.
Visit 'about:'.
Start the Developer Tools.
Select the Options and select the Firebug theme therein.
Open the Inspector and choose the root html node.
Select the Computed panel.
If necessary, scroll down to find the font-family property.
Hover over the value for the font-family.


Actual results:

The Tooltip appears to be blank, all white but slightly transparent.


Expected results:

The Tooltip should show an clear dark image showing the sample text:
The quick brown fox jumps over the lazy dog
(The sample text could be different.)
This problem is also in the Rules panel.  (Opening the font short-form property shows the font-family property.)

Comment 1

[johngraciliano]

If you manage to show the tool-tip over a dark background, you will see it has white text a semi-transparent white background.
When I examined the tools with the DOM Inspector, I found what is displayed is an image node whose src attribute value is the png image of the sample text encoded as a string.
The problem is that the image used when the Firebug theme is active appears to be the same as the image used when the Dark theme is active, so the is no contrast!
The image used should be the image used with the Light theme but that is not the case for reasons unknown to me.  
Apparently at the time of selecting the image, the process reads something different than exactly "theme-light" for the class (it is "theme-light theme-firebug") and chooses "theme-dark" as default.

Note the problem is not in the (now hidden by default) Fonts panel.  This panel also uses images for the large "Abc" text samples.  However these nodes are html|img instead of xul|image.

As temporary solution I added the rule:
.theme-firebug .devtools-tooltip > vbox > image:only-child {
  filter: invert(0.9375);
}
to inspector.css.  This works in Firefox 48 but not in Firefox 50.  
It would clearly become a bug once the right solution is applied.  
The rule may also have other side effects.

Comment 2

[roxleitan]

I can confirm that this issue is reproducible on Win 8 x64 with FF 49.0a2, FF 50.0a1 when e10s is enabled.

Comment 3

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).
Julian offered to take a look at this.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1279703 - use theme body-color for devtools font-preview;

Review commit: https://reviewboard.mozilla.org/r/63194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63194/

Comment 5

[Julian Descottes [:jdescottes]]

@pbro: this patch slightly changes the colors used for each theme since we go from black/white to #393f4c / #8fa1b2 (and now #18191a for the firebug theme). The good counterpart is that it now matches the rest of the theme, and if we ever add new themes, this will still work.

Comment 6

[johngraciliano]

@jdescottes: Thank you so much!

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8769231 [details]
Bug 1279703 - use theme body-color for devtools font-preview;

https://reviewboard.mozilla.org/r/63194/#review60254

::: devtools/client/inspector/shared/style-inspector-overlays.js:474
(Diff revision 1)
>  
>      font = font.replace(/"/g, "'");
>      font = font.replace("!important", "");
>      font = font.trim();
>  
> -    let fillStyle = getTheme() === "light" ? "black" : "white";
> +    let fillStyle = getColor("body-color");

Cool, much better!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5b110e44f7a6
font-preview should use white fillstyle only for dark theme;r=pbro

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_styleinspector_tooltip-longhand-fontfamily.js:

https://hg.mozilla.org/integration/fx-team/rev/8f80e6e64a01

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=5b110e44f7a649a7b98d202ae36750a37efd95e1
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=10413607&repo=fx-team

10:54:04     INFO -  29 INFO Testing font-family tooltips in the rule view
10:54:04     INFO -  30 INFO TEST-PASS | devtools/client/inspector/shared/test/browser_styleinspector_tooltip-longhand-fontfamily.js | Tooltip instance exists -
10:54:04     INFO -  31 INFO TEST-PASS | devtools/client/inspector/shared/test/browser_styleinspector_tooltip-longhand-fontfamily.js | XUL panel exists -
10:54:04     INFO -  32 INFO TEST-PASS | devtools/client/inspector/shared/test/browser_styleinspector_tooltip-longhand-fontfamily.js | A tooltip is defined on hover of the given element -
10:54:04     INFO -  33 INFO TEST-PASS | devtools/client/inspector/shared/test/browser_styleinspector_tooltip-longhand-fontfamily.js | Tooltip contains an image -
10:54:04     INFO -  34 INFO TEST-PASS | devtools/client/inspector/shared/test/browser_styleinspector_tooltip-longhand-fontfamily.js | Tooltip contains a data-uri image as expected -
10:54:04     INFO -  35 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/shared/test/browser_styleinspector_tooltip-longhand-fontfamily.js | Tooltip contains the correct data-uri image - Got data:image/png;base64,...

Comment 10

[Julian Descottes [:jdescottes]]

Sorry about that, I didn't think we had tests checking the actual image used for the font preview.

Updated the tests and submitted to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dcc6e1d7a68

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1279703 - update tests after changing fillStyle for devtools font preview;

Review commit: https://reviewboard.mozilla.org/r/63484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63484/

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8769231 [details]
Bug 1279703 - use theme body-color for devtools font-preview;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63194/diff/1-2/

Comment 13

[Julian Descottes [:jdescottes]]

Review request for the test update I missed the first time. I can fold the changesets together before landing if you'd prefer.

Green try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a362d159e31d92974e6657e5b0118948e10fe402

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8769753 [details]
Bug 1279703 - update tests after changing fillStyle for devtools font preview;

https://reviewboard.mozilla.org/r/63484/#review60558

Looks good to me, thanks. Yes, please do fold the 2 commits together.

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8769231 [details]
Bug 1279703 - use theme body-color for devtools font-preview;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63194/diff/2-3/

Comment 16

[Julian Descottes [:jdescottes]]

Rebased & folded. Started https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c6513b225d8 for reference but there was no code change / merge since the previous try run, landing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0d13d82daefd
use theme body-color for devtools font-preview;r=pbro

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[johngraciliano]

Will this make it into Firefox 48?!
Or 49!!!

Comment 20

[roxleitan]

Verified as fixed on Windows 10 x64 and Windows 8 x64 on the latest Nightly 50.0a1 ( Build ID: 	20160718081125)

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8769231 [details]
Bug 1279703 - use theme body-color for devtools font-preview;

Approval Request Comment
[Feature/regressing bug #]: Firebug theme implementation: Bug 1244054
[User impact if declined]: The "font preview" tooltip will be unreadable for devtools users with the Firebug theme. Those users are most likely current firebug users trying out devtools and we should make sure the experience is as good as possible.
[Describe test coverage new/current, TreeHerder]: already tested in browser_styleinspector_tooltip-longhand-fontfamily.js, https://treeherder.mozilla.org/#/jobs?repo=try&revision=a362d159e31d92974e6657e5b0118948e10fe402
[Risks and why]: low risk, simple color change. 
[String/UUID change made/needed]: N/A

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8769231 [details]
Bug 1279703 - use theme body-color for devtools font-preview;

This patch polishes a UI and it's also verified. Let's take it in aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2241d19980e6