1067999 - Images in background-image tooltips aren't always easy to see

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

STR:

- Open https://www.mozilla.org/en-US/
- Open the inspector
- Make sure the light theme is selected (go to the options panel to change it if not)
- Using the inspector search field, find node '.btn-prev' and select it
- In the Rules panel, hover over the URL in the property 'background-image: url("/media/img/home/control-arrows-lt.png");'

==> the white arrow icon in the tooltip is just invisible in the light theme. We need a better tooltip background to help see it.

If you switch to the dark theme (in the options panel), you will see the image fine.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Inspector Bugs Triage - Filter on CLIMBING SHOES

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: bug.png

STR from Comment 0 isn't available anymore, but this screenshot shows the issue for the light theme.

Comment 3

[Brad Aytes [:eights]]

Nicolas, I'll take a shot at this one. What is the best way to get started? What is the particular directory in the source that holds the css for the inspector?

Comment 4

[Brad Aytes [:eights]]

Cannot replicate from comment 0, there isn't a node '.btn-prev' and no 'background-image: url("/media/img/home/control-arrows-lt.png");' Has this been fixed as well?

Comment 5

[Patrick Brosset <:pbro>]

As commented by Nicolas on IRC just now:

> 4:15 PM <nchevobbe> eights: you should be able to see it if you inspect the "Eurosport" logo in this page "http://www.eurosport.fr/"
> 4:16 PM <nchevobbe> search ".nav__logo"
> 4:17 PM <nchevobbe> eights: it's a data-uri, but we do show a preview for this, and you can see that if you have the light theme, you can't really see the logo

Comment 6

[Patrick Brosset <:pbro>]

Also, as discussed with Helen on IRC: a checkered background would help.
Brad, once you have something that works, it would be great if you could ping :helenvholmes on the bug so she can verify the visual aspect of it. Also because we have a new popup design coming up, so we should make sure this works there too.

Comment 7

[Brad Aytes [:eights]]

Patrick, Ok great. Thanks for the help, I'll see what I can work up. 

Nicolas, Thank you for the point in the right direction.

Comment 8

[Brad Aytes [:eights]]

Attachment: blackLogo.png - black logo over top of checkered background for devtools tooltip

Screenshot of black logo on checkered background for devtools tooltip

Comment 9

[Brad Aytes [:eights]]

Attachment: whiteLogo.png - white logo over top of checkered background for devtools tooltip

Screenshot of white logo on checkered background for devtools tooltip

Comment 10

[Brad Aytes [:eights]]

Attachment: coloredLogo.png - colored logo over top of checkered background for devtools tooltip

Screenshot of colored logo on checkered background for devtools tooltip

Comment 11

[Brad Aytes [:eights]]

Screenshots added for ui-reivew by :helenvholmes showing checkered background on the devtools tooltip(s) have been added.

Comment 12

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8754467 [details]
coloredLogo.png - colored logo over top of checkered background for devtools tooltip

I'm noticing some odd overlap in this screenshot: http://cl.ly/3A11380e2R0z is that because of the background image not lining up perfectly?

I also think that the background is a little dark. Could we lighten it up to match the background colors of say Sketch? http://cl.ly/1K1s223Y3a13

Comment 13

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8754465 [details]
whiteLogo.png - white logo over top of checkered background for devtools tooltip

I'm thinking that we might want to keep the pixel values over top of a white background for readability. Can we have the checkered background not span the entire height of the tooltip, but leave say, 15px of white background at the bottom with the pixel label vertically centered within it?

Comment 14

[Brad Aytes [:eights]]

Attachment: tooltipBG-LogoSS.zip - blackLogo.png - Logos over top of checkered background for devtools tooltip

Comment 15

[Brad Aytes [:eights]]

Attachment: 1067999.patch

Attached '1067999.patch'

User: Brad Aytes
Email: [e-mail] 

Bug 1067999 - Fixed "Images in background-image tooltips aren't always easy to see" by creating a new css class and adding a repeating checkered background image for the tooltip widget.

Files Modified:

~/devtools/client/shared/widgets/Tooltip.js 
	Line 584: vbox.className = "devtools-tooltip-simple-bg-container";

~/devtools/client/themes/common.css
	Line 212-216: /* Tooltip: Background Image */
			 .devtools-tooltip-simple-bg-container {
			    background-image: url("chrome://devtools/skin/images/repeating-bg.png");
			    background-repeat: repeat;
			  }

~/devtools/client/jar.mn
	Line 189: skin/images/repeating-bg.png (themes/images/repeating-bg.png)


The 'repeating-bg.png' image has been added to: ~/devtools/client/themes/images/repeating-bg.png

Comment 16

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8754867 [details]
tooltipBG-LogoSS.zip -  blackLogo.png - Logos over top of checkered background for devtools tooltip

Hey Brad,

Looks like some of the review from my last pass isn't in these images—was that intentional or just a uploading error?

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8755067 [details] [diff] [review]
1067999.patch

Hey Brad, can you make sure this is rebased with today's fx-team? I'm having issues applying this cleanly.

Comment 18

[Brad Aytes [:eights]]

Helen, 

Yes that was an uploading errormy last comment was the updated one & yes as soon as I get back to the house I'll pull down the latest and apply the updates re-create the patch and upload it for you. Let me know if you need anything else in the mean time.

Thanks,

Brad

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8755067 [details] [diff] [review]
1067999.patch

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

::: devtools/client/shared/widgets/Tooltip.js
@@ +580,5 @@
>  
>      // Main container
>      let vbox = this.doc.createElement("vbox");
>      vbox.setAttribute("align", "center");
> +    vbox.className = "devtools-tooltip-simple-bg-container";

Instead of introducing this new classname, can you use the existing .devtools-tooltip-tiles class instead ?
That also means the image you've introduced can be removed.
See https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#236

Comment 20

[Brad Aytes [:eights]]

I apologize for the delay on this, I'm working this up now and will submit a new patch once completed. 

Thanks,

Brad

Comment 21

[Brad Aytes [:eights]]

Attachment: 1067999-06012016.patch

*** EDITED *** Per 'Tim Nguyen :ntim' instructions.

Attached '1067999-06012016.patch'

User: Brad Aytes
Email: [e-mail] 

*** EDITED *** Bug 1067999 - Fixed "Images in background-image tooltips aren't always easy to see" by creating a new css class and adding a repeating checkered background image for the tooltip widget.

Files Modified:

~/devtools/client/shared/widgets/Tooltip.js 
	Line 584: vbox.className = "devtools-tooltip-tiles";

			  }
REMOVED: 

~/devtools/client/themes/common.css
	Line 212-216: /* Tooltip: Background Image */
			 .devtools-tooltip-simple-bg-container {
			    background-image: url("chrome://devtools/skin/images/repeating-bg.png");
			    background-repeat: repeat;

~/devtools/client/jar.mn
	Line 189: skin/images/repeating-bg.png (themes/images/repeating-bg.png)

REMOVED: 

The 'repeating-bg.png' image has been removed to: ~/devtools/client/themes/images/repeating-bg.png

Comment 22

[Tim Nguyen :ntim]

Comment on attachment 8758997 [details] [diff] [review]
1067999-06012016.patch

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

It seems like this patch applies on top of a previous patch I don't have.
Can you post a folded patch ?

::: devtools/client/jar.mn
@@ +184,5 @@
>      skin/images/command-console.svg (themes/images/command-console.svg)
>      skin/images/command-eyedropper.svg (themes/images/command-eyedropper.svg)
>      skin/images/command-rulers.svg (themes/images/command-rulers.svg)
>      skin/images/command-measure.svg (themes/images/command-measure.svg)
> +    skin/images/command-noautohide.svg (themes/images/command-noautohide.svg)            

nit: trailing whitespace

::: devtools/client/shared/widgets/Tooltip.js
@@ +575,5 @@
>     */
>    setImageContent: function(imageUrl, options={}) {
>      if (!imageUrl) {
>        return;
>      }    

nit: trailing whitespace

@@ +579,5 @@
>      }    
>  
>      // Main container
>      let vbox = this.doc.createElement("vbox");
> +    vbox.setAttribute("align", "center");    

nit: trailing whitespace

Comment 23

[Brad Aytes [:eights]]

Sure, how do I create the 'folded patch'?

hg qpush --move my-first-patch     ?

Thanks,

Brad

Comment 24

[Patrick Brosset <:pbro>]

If this bug requires 2 patches to be fixed, then you should post both patches here. Unless it depends on an other bug that will land first? In which case you should mark this bug as depending on that other bug.
In any case, if you want to "fold" 2 patches into 1 and you are using MQ (which appears to be the case from comment 23), then you should use the the qfold command: http://hgbook.red-bean.com/read/mercurial-queues-reference.html#id444909

Comment 25

[Brad Aytes [:eights]]

Patrick, It doesn't require 2 patches, that way my mistake in creating a patch initially (for the first stab at fixing this bug, however there were a few changes that needed to be made after i created the initial patch) and once I reverted back the initial changes and made the 'final' changes, I created another patch and uploaded it here. This is why it is 'seeming' like there are multiple patches. How can I get rid of ALL of the changes and start fresh with the default mozilla-central repo and make the 'correct' changes? Any of that make sense? LOL

Comment 26

[Patrick Brosset <:pbro>]

(In reply to Brad Aytes [:eights] from comment #25)
> How can I get rid of ALL of the changes and
> start fresh with the default mozilla-central repo and make the 'correct'
> changes?
Using MQ, make sure you unapply all your patches first:
hg qpop -a [1]
Then pull the latest changes made to the repository:
hg pull -u [2]
Then, 2 choices:
- either you have one of your patches that is clean and you want to re-apply just this one: hg qpush --move N (where N is the index of the patch in your queue)
- or you really want tol start fresh as you said and in which case you can just start working and create a new patch with hg qnew

[1] https://www.mercurial-scm.org/wiki/MqExtension#Command_Examples
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build#How_to_update

Comment 27

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8758997 [details] [diff] [review]
1067999-06012016.patch

If it's all right, I'm going to clear my review flag until the new patch gets attached :)

Comment 28

[Brad Aytes [:eights]]

Absolutly, Sorry! I'm a Few Days Behind.

Comment 29

[Brad Aytes [:eights]]

Attachment: 1067999-reBuilt.patch

This should be a clean patch now with no dependencies. Let me know if it is not.

Thanks,

Brad

Comment 30

[Tim Nguyen :ntim]

Comment on attachment 8760558 [details] [diff] [review]
1067999-reBuilt.patch

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

The patch now applies fine, but I can't seem to see the checkered background anywhere when running the build.
Maybe the class needs to be applied on a different element ?

Comment 31

[Tim Nguyen :ntim]

It seems to be applied on the netmonitor image tooltip, which is a good thing, but not in the inspector tooltip.

The inspector relies on the HTML tooltip which is here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js#50

So you'll have to add it there as well.

Comment 32

[Tim Nguyen :ntim]

Also, instead of adding the class on the container, I think it would be better to apply it on the image itself: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/Tooltip.js#563 . This way, the labels still remain visible.

Comment 33

[Tim Nguyen :ntim]

Note that you can use `hg qref` to fold the changes in your top-most patch without creating a new patch for each change.

Comment 34

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen :ntim from comment #31)
> It seems to be applied on the netmonitor image tooltip, which is a good
> thing, but not in the inspector tooltip.
> 
> The inspector relies on the HTML tooltip which is here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> widgets/tooltip/ImageTooltipHelper.js#50
> 
> So you'll have to add it there as well.

Sorry about that, I didn't see this bug! I should have warned you about the HTML Tooltip migration. We still have one panel using XUL Tooltips for images: the netmonitor, so if you intend to fix this now you will indeed have to do it both for the XUL Tooltip and for the HTML Tooltip. 

(FWIW, the netmonitor tooltips will be migrated in Bug 1277264, but since we are focusing on the inspector for now, it might not happen before a while)

Sorry again for the double maintenance, ping me if you have any question about the HTMLTooltip implementation.

Comment 35

[Brad Aytes [:eights]]

I dont seem to have a /devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js in my source. Am I missing something?

Comment 36

[Julian Descottes [:jdescottes]]

(In reply to Brad Aytes [:eights] from comment #35)
> I dont seem to have a
> /devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js in my source.
> Am I missing something?

This is a recent addition, you probably need to pull and update your repository. This file landed just 13 days ago : https://hg.mozilla.org/integration/fx-team/rev/a41f8a55229c

Comment 37

[Brad Aytes [:eights]]

(In reply to Julian Descottes [:jdescottes] from comment #36)
> (In reply to Brad Aytes [:eights] from comment #35)
> > I dont seem to have a
> > /devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js in my source.
> > Am I missing something?
> 
> This is a recent addition, you probably need to pull and update your
> repository. This file landed just 13 days ago :
> https://hg.mozilla.org/integration/fx-team/rev/a41f8a55229c

I went and did a 'hg pull' and then a 'hg update' and it says: 

eights@dev:~/Code/mozilla/mozilla-central$ hg pull
pulling from https://hg.mozilla.org/mozilla-central
searching for changes
adding changesets
adding manifests
adding file changes
added 896 changesets with 3392 changes to 2135 files                                                                     
(run 'hg update' to get a working copy)

eights@dev:~/Code/mozilla/mozilla-central$ hg update
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
1 other heads for branch "default"

Comment 38

[Julian Descottes [:jdescottes]]

You need to explicitly update to the latest changeset here: "hg update tip"

I am not using mq, so I don't know how you can reapply your patch on top of the new tip.
According to Patrick in comment 26, looks like you can do something like:

> hg qpush --move N (where N is the index of the patch in your queue)

Comment 39

[Brad Aytes [:eights]]

ok, thanks. I should obviously 'hg update tip' before reapplying the patch on top correct?

Comment 40

[Tim Nguyen :ntim]

(In reply to Brad Aytes [:eights] from comment #39)
> ok, thanks. I should obviously 'hg update tip' before reapplying the patch
> on top correct?

I never needed to do that with MQ.

Comment 41

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 42

[Nicolas Chevobbe [:nchevobbe]]

Hey Brad, do you need help to finish this bug ? Since you have a patch it would be nice to have it in the tools :)

Comment 43

[Julian Descottes [:jdescottes]]

Just a heads up, the image tooltip has since moved to a HTML tooltip. 

The file that should be modified here is devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js
The markup for the image tooltip is being created in setImageTooltip() [1]

(looking at the current implementation, feel free to take the opportunity of moving some of the inline styles to a class :) ) 

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js#82

Comment 44

[Stefan Yohansson]

Attachment: adding checkered background to preview background-image tooltip.

Hi,

I have found this issue on my devtools and fix it. This issue is idle since 2016-10, so I submitted my patch, sorry if I need to talk something (it's my first mozilla patch). Let me know if I need to change or send something.

Comment 45

[Stefan Yohansson]

Attachment: Captura de tela de 2017-03-10 11-58-54.png

This is a preview from my patch. I have a newbie question, I added an image background for that commit, should I send checkered-background.png file here with the patch?

Comment 46

[Stefan Yohansson]

Attachment: bug1067999.patch

When put background on wrapper like I did, it show a little of the checkered background even when image isn't transparent.

This fix this problem: added checkered background to img.

Comment 47

[Stefan Yohansson]

Attachment: Captura de tela de 2017-03-10 12-17-46.png

Preview from my last patch.

Comment 48

[Tim Nguyen :ntim]

Comment on attachment 8845934 [details] [diff] [review]
bug1067999.patch

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

Looks great thanks!

::: devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js
@@ +86,5 @@
>                  justify-content: center;
>                  min-height: 1px;">
> +      <img style="height: ${imgHeight}px;
> +                  background-image: url('chrome://devtools/skin/images/checkered-bg.png');
> +                  background-repeat: round repeat;

The devtools-tooltip-tiles class does exactly this without the need of the image or the background properties. Can you use it ?

Comment 49

[Stefan Yohansson]

Attachment: bug1067999.patch

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40da036eabf6
Fixed 'Images in background-image tooltips aren't always easy to see' by adding a repeating image for the tooltip widget. r=ntim

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40da036eabf6

Comment 52

[Surentharan]

[bugday-20170315]

Comment 53

[Madhuri]

[bugday-20170322]

status-ff55: verified and fixed

BuildID : 20170321030211

Comment 54

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 35.0a1 (build id:20140916030204)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox nightly 55.0a1(build id:20170502030211)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170503]