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

VERIFIED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Computed Styles Inspector
P2
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: johngraciliano, Assigned: jdescottes)

Tracking

(Blocks: 1 bug)

50 Branch
Firefox 50
x86_64
Windows 8
Points:
---

Firefox Tracking Flags

(e10s-, firefox49 fixed, firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.)
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 8
Hardware: Unspecified → x86_64
I can confirm that this issue is reproducible on Win 8 x64 with FF 49.0a2, FF 50.0a1 when e10s is enabled.
Status: UNCONFIRMED → NEW
status-firefox49: --- → affected
status-firefox50: --- → affected
tracking-e10s: --- → ?
Component: Untriaged → Add-ons Manager
Ever confirmed: true
Product: Firefox → Toolkit
Version: 48 Branch → 50 Branch

Updated

a year ago
tracking-e10s: ? → -

Updated

a year ago
Component: Add-ons Manager → Developer Tools
Product: Toolkit → Firefox

Updated

a year ago
Blocks: 1263889
Component: Developer Tools → Developer Tools: Computed Styles Inspector
Inspector bug triage (filter on CLIMBING SHOES).
Julian offered to take a look at this.
Flags: needinfo?(jdescottes)
Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Created attachment 8769231 [details]
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/
Attachment #8769231 - Flags: review?(pbrosset)
@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.
(Reporter)

Comment 6

a year ago
@jdescottes: Thank you so much!
Attachment #8769231 - Flags: review?(pbrosset) → review+
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

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5b110e44f7a6
font-preview should use white fillstyle only for dark theme;r=pbro
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,...
Flags: needinfo?(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
Flags: needinfo?(jdescottes)
Created attachment 8769753 [details]
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/
Attachment #8769753 - Flags: review?(pbrosset)
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/
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 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.
Attachment #8769753 - Flags: review?(pbrosset) → review+
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/
Attachment #8769231 - Attachment description: Bug 1279703 - font-preview should use white fillstyle only for dark theme; → Bug 1279703 - use theme body-color for devtools font-preview;
Attachment #8769753 - Attachment is obsolete: true
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

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/0d13d82daefd
use theme body-color for devtools font-preview;r=pbro

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d13d82daefd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Reporter)

Comment 19

a year ago
Will this make it into Firefox 48?!
Or 49!!!
Verified as fixed on Windows 10 x64 and Windows 8 x64 on the latest Nightly 50.0a1 ( Build ID: 	20160718081125)
Status: RESOLVED → VERIFIED
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
Attachment #8769231 - Flags: approval-mozilla-aurora?
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.
Attachment #8769231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2241d19980e6
status-firefox49: affected → fixed
Duplicate of this bug: 1088305
You need to log in before you can comment on or make changes to this bug.