1088305 - Font preview tooltip should allow to define a custom color

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Sebastian Zartner [:sebo]]

Firebug wants to style the font preview tooltip.
Unfortunately the code currently doesn't allow to do that[1].

It would basically already be enough, if the text color could be specified individually instead of having a fixed check for the 'light' theme[2].

Sebastian

[1] http://hg.mozilla.org/mozilla-central/file/9add1ec0251d/toolkit/devtools/server/actors/inspector.js#l598
[2] http://hg.mozilla.org/mozilla-central/file/9add1ec0251d/browser/devtools/shared/widgets/Tooltip.js#l840

Comment 1

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Inspector Bugs Triage - Filter on CLIMBING SHOES

Comment 2

[Julian Descottes [:jdescottes]]

Sebastian: the implementation of the font preview changed recently (see Bug 1279703). The fillStyle is now using the variable theme-body-color. Meaning it uses #18191a for the firebug theme. 

Did you plan to customize this further for the firebug theme or should we close as duplicate?

Comment 3

[Sebastian Zartner [:sebo]]

(In reply to Julian Descottes [:jdescottes] from comment #2)
> Sebastian: the implementation of the font preview changed recently (see Bug
> 1279703). The fillStyle is now using the variable theme-body-color. Meaning
> it uses #18191a for the firebug theme. 

Cool, that makes customization very easy!

> Did you plan to customize this further for the firebug theme or should we
> close as duplicate?

Firebug 2 uses pure black, i.e. #000000, for the tooltip and other texts in the UI, so that might still be changed.
Though the issue to allow customization is solved now, so I mark this as duplicate of the other bug.

Sebastian

Comment 4

[Sebastian Zartner [:sebo]]

After bug 1306139 being fixed I realized that bug 1279703 did *not* fix this issue.

There are still some places, which are hardcoded[1], which should be replaced by a call to getColor() and the Firebug theme isn't included in the constants[2], which causes it not to be applied.

Sebastian

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/fonts/fonts.js#165-166
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme.js#20

Comment 5

[Julian Descottes [:jdescottes]]

Good catch, the only thing bug 1279703 changed is that it now falls back to the light theme.

Sebastian, you pretty much did all the investigation and all that's left is to follow the code changes suggested in your comment #4. Would you like to take the bug?

Comment 6

[Sebastian Zartner [:sebo]]

Will be my first time to provide a patch to the Firefox sources, so I first need to get to know about the workflow, but sure, I can try.

Sebastian

Comment 7

[Julian Descottes [:jdescottes]]

Great, thanks for taking this!

The two main entry points for our contributor documentation are :
- https://wiki.mozilla.org/DevTools/Hacking (great if you plan on using Mercurial)
- https://developer.mozilla.org/en-US/docs/Tools/Contributing (also has instructions for git)

The two overlap, so just pick one and try to get your development environment setup. 
The documentation also contains instructions on our coding standards and how to submit a patch.

Once you are able to modify the source and see the changes applied in your own build of Firefox, I think you already found everything you had to change in order to fix this so you should be all set. But if you need any help you can either needinfo me here or ping me in #devtools on IRC.

On top of the fix, it would be nice to add tests for the firebug theme in devtools/client/shared/test/browser_theme.js (instructions for running tests can also be found on the documentation pages linked above).

Comment 8

[Sebastian Zartner [:sebo]]

Attachment: Bug 1088305 - Allowed to customize the text color of the font preview tooltip;

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

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8798594 [details]
Bug 1088305 - Allowed to customize the text color of the font preview tooltip;

https://reviewboard.mozilla.org/r/84010/#review82744

This is great! Nothing to say about the code change, it works perfectly.

I am clearing my review flag for now to check if you'd like to also update the test at devtools/client/shared/test/browser_theme.js to cover the firebug theme. It's mostly about copying the asserts made for the dark theme and apply them to the firebug theme. 

Let me know if you'd like to tackle this as well! Otherwise I'll r+ and log a followup.

::: devtools/client/inspector/fonts/fonts.js:166
(Diff revision 1)
>      this._lastUpdateShowedAllFonts = showAllFonts;
>  
> -    // Assume light theme colors as the default (see also bug 1118179).
> +    let fillStyle = getColor("body-color");
> -    let fillStyle = (Services.prefs.getCharPref("devtools.theme") == "dark") ?
> -        "white" : "black";
>  

nit: remove this blank line (or remove the fillStyle variable and directly do previewFillStyle: getColor("body-color")

Comment 10

[Sebastian Zartner [:sebo]]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> I am clearing my review flag for now to check if you'd like to also update
> the test at devtools/client/shared/test/browser_theme.js to cover the
> firebug theme. It's mostly about copying the asserts made for the dark theme
> and apply them to the firebug theme. 
> 
> Let me know if you'd like to tackle this as well! Otherwise I'll r+ and log
> a followup.

I'll have a look at the test. No need to create a followup (at least for now :-) ).

> ::: devtools/client/inspector/fonts/fonts.js:166
> (Diff revision 1)
> >      this._lastUpdateShowedAllFonts = showAllFonts;
> >  
> > -    // Assume light theme colors as the default (see also bug 1118179).
> > +    let fillStyle = getColor("body-color");
> > -    let fillStyle = (Services.prefs.getCharPref("devtools.theme") == "dark") ?
> > -        "white" : "black";
> >  
> 
> nit: remove this blank line (or remove the fillStyle variable and directly
> do previewFillStyle: getColor("body-color")

Ok, I'll change it.

Sebastian

Comment 11

[Sebastian Zartner [:sebo]]

Attachment: Bug 1088305 - Adjusted browser theme test to check Firebug theme;

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

Comment 12

[Sebastian Zartner [:sebo]]

Comment on attachment 8798594 [details]
Bug 1088305 - Allowed to customize the text color of the font preview tooltip;

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

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8798594 [details]
Bug 1088305 - Allowed to customize the text color of the font preview tooltip;

https://reviewboard.mozilla.org/r/84010/#review83016

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8799079 [details]
Bug 1088305 - Adjusted browser theme test to check Firebug theme;

https://reviewboard.mozilla.org/r/84388/#review83018

Great! Thanks for updating the test!

Comment 15

[Julian Descottes [:jdescottes]]

Pushed your changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d795d275c85afe5b794ad9a0bede1c27c30d6b4f

Comment 16

[Sebastian Zartner [:sebo]]

If I interpret the test results correctly, then there were a few errors but they are unrelated to my changes, right?

Sebastian

Comment 17

[Julian Descottes [:jdescottes]]

(In reply to Sebastian Zartner [:sebo] from comment #16)
> If I interpret the test results correctly, then there were a few errors but
> they are unrelated to my changes, right?
> 
> Sebastian

Exactly, they are unrelated intermittent failures. 

Adding the checkin-needed keyword to the bug so that a sheriff can land your patches on an integration branch. After this, your patches will ride the release train and should make their way to Firefox 52. Thanks a lot for fixing this Sebastian!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76efdf921d79
Allowed to customize the text color of the font preview tooltip; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3f46c236948f
Adjusted browser theme test to check Firebug theme; r=jdescottes

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4778254&repo=autoland

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3f844d18e01
Backed out changeset 3f46c236948f 
https://hg.mozilla.org/integration/autoland/rev/855d519fc717
Backed out changeset 76efdf921d79 for ES Failures

Comment 21

[Sebastian Zartner [:sebo]]

Julian, trying to push a follow up patch on this I get the following error message:

"abort: reviewboard error: One or more fields had errors (HTTP 400, API Error 105); identifier: Parent review request is submitted or discarded"

I guess this is due to the backout. Can you please tell me how I can get the patches landed now?
I read the following documents, though they don't clarify what to do in this situation:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html
https://developer.mozilla.org/en-US/docs/Tools/Contributing
http://codefirefox.com/

I assume I need to create a new branch using Mercurial and reapply the patches there. Is that correct? If yes, how to do that?

Sebastian

Comment 22

[Julian Descottes [:jdescottes]]

Arg, why didn't we get the ES lint failures on try? Weird. Sorry for missing it anyway.

Can you try to go to https://reviewboard.mozilla.org/r/84008/ and click on the "reopen for review" button in the header? 
(asking for help in #mozreview in the meantime)

Comment 23

[Sebastian Zartner [:sebo]]

Attachment: Bug 1088305 - Removed unused constant Services;

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

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8799926 [details]
Bug 1088305 - Removed unused constant Services;

https://reviewboard.mozilla.org/r/84990/#review83540

Thanks! 

(For future patches, it is usually better to fold this kind of fixes with the original changeset)

Comment 25

[Sebastian Zartner [:sebo]]

(In reply to Julian Descottes [:jdescottes] from comment #24)
> Comment on attachment 8799926 [details]
> Bug 1088305 - Removed unused constant Services;
> 
> https://reviewboard.mozilla.org/r/84990/#review83540
> 
> Thanks!

Wow, that was a fast review! :-)

> (For future patches, it is usually better to fold this kind of fixes with
> the original changeset)

I know that changesets should be merged, though I didn't see how to do that. And as the last change is actually unrelated to my changes, I thought it would be ok to keep it separate.

Sebastian

Comment 26

[Julian Descottes [:jdescottes]]

(In reply to Sebastian Zartner [:sebo] from comment #25)
> (In reply to Julian Descottes [:jdescottes] from comment #24)
> > Comment on attachment 8799926 [details]
> > Bug 1088305 - Removed unused constant Services;
> > 
> > https://reviewboard.mozilla.org/r/84990/#review83540
> > 
> > Thanks!
> 
> Wow, that was a fast review! :-)
> 

Notifications o/

> > (For future patches, it is usually better to fold this kind of fixes with
> > the original changeset)
> 
> I know that changesets should be merged, though I didn't see how to do that.

To fold changesets together I use the Mercurial histedit extension (which is similar to Git's interactive rebase) : https://www.mercurial-scm.org/wiki/HisteditExtension

> And as the last change is actually unrelated to my changes, I thought it
> would be ok to keep it separate.
> 

It's really not that important, but they are kind of related:
- the first changeset removed the last usage of "Services" in fonts.js, breaking the ESLint validation
- the last changeset fixes this by removing the import

Ideally we should make sure the build (eslint included) is ok and tests are passing for each and every changeset. But again, not a big deal.

Comment 27

[Sebastian Zartner [:sebo]]

(In reply to Julian Descottes [:jdescottes] from comment #26)
> (In reply to Sebastian Zartner [:sebo] from comment #25)
> > > (For future patches, it is usually better to fold this kind of fixes with
> > > the original changeset)
> > 
> > I know that changesets should be merged, though I didn't see how to do that.
> 
> To fold changesets together I use the Mercurial histedit extension (which is
> similar to Git's interactive rebase) :
> https://www.mercurial-scm.org/wiki/HisteditExtension

Thank you for the hint. I'll try it out.

> > And as the last change is actually unrelated to my changes, I thought it
> > would be ok to keep it separate.
> > 
> 
> It's really not that important, but they are kind of related:
> - the first changeset removed the last usage of "Services" in fonts.js,
> breaking the ESLint validation
> - the last changeset fixes this by removing the import

Oops, you're right. I totally missed that.

> Ideally we should make sure the build (eslint included) is ok and tests are
> passing for each and every changeset. But again, not a big deal.

I'll be more careful next time and do an eslint before pushing my patches. With the time I'll surely get better with that.

Thank you for the patience with me and for reviewing my changes!

Sebastian

Comment 28

[Julian Descottes [:jdescottes]]

(In reply to Sebastian Zartner [:sebo] from comment #27)
> (In reply to Julian Descottes [:jdescottes] from comment #26)
> 
> > Ideally we should make sure the build (eslint included) is ok and tests are
> > passing for each and every changeset. But again, not a big deal.
> 
> I'll be more careful next time and do an eslint before pushing my patches.
> With the time I'll surely get better with that.
> 
> Thank you for the patience with me and for reviewing my changes!
> 
> Sebastian

Let me reiterate that you did a really great job here! I really should have caught the eslint failure (again I need to see why it didn't show up on our earlier try push). You've handled everything on your own from the investigation to the implementation! Thanks again, I'll be more than happy to review your patches in the future :) 

Pushed to try your latest changesets: https://treeherder.mozilla.org/#/jobs?repo=try&revision=853d3cc4af0a5346187ac7c6323e7e79f55d937a , we'll add the checkin-needed keyword again when it's green.

Comment 29

[Julian Descottes [:jdescottes]]

Try is green except for unrelated intermittents.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/720ec39fe42b
Allowed to customize the text color of the font preview tooltip; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0eac6bd98bd0
Adjusted browser theme test to check Firebug theme; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/d064196c2214
Removed unused constant Services; r=jdescottes

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/720ec39fe42b
https://hg.mozilla.org/mozilla-central/rev/0eac6bd98bd0
https://hg.mozilla.org/mozilla-central/rev/d064196c2214

Comment 32

[Sebastian Zartner [:sebo]]

> Resolution: --- → FIXED

Yay!

Thank you again for helping me land my first patch! More will follow soon. :-)

Sebastian

Comment 33

[Nazir Ahmed Sabbir [:NaSb]]

I've managed this bug on Nightly 36.0a1 (2014-10-23) from Ubuntu 14.04.5 (64 Bit) 

This Bug is now verified as fixed on Latest Firefox Nightly 52.0a1 (2016-11-10) (64-bit) 

Build ID: 20161110030211
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
OS: Linux 4.4.0-47-generic; Ubuntu 14.04.5

Comment 34

[Sebastian Zartner [:sebo]]

Thank you for the verification!

Sebastian