1017202 - browser.customizemode.tip0 text decreasingly legible as screen density increases

Status: NEW

(Firefox :: Theme, defect)

Description

[Felix Miata]

Attachment: ff30b8 096 DPI screenshot (passable; could be better)

Screenshots demonstrate tooltip text has no apparent relationship to size of CSS menu or other DE UI text when display density is >96. The higher the screen density, the closer to useless the tooltip text is, apparently sized in screen pixels instead of a competent function of CSS menu or other DE UI text size.

Comment 1

[Felix Miata]

Attachment: ff30b7 120 DPI screenshot (text size marginally useful)

Comment 2

[Felix Miata]

Attachment: ff30b8 144 DPI screenshot (text size much too small, bordering on totally illegible)

Comment 3

[:Gijs (he/him)]

This is because of this block of CSS, which should be updated to use ems instead, or just entirely removed (depending on what is feasible).

http://hg.mozilla.org/mozilla-central/annotate/1e712b724d17/browser/themes/shared/customizableui/customizeTip.inc.css#l28

Comment 4

[Shashank VRSN Sabniveesu]

Everything in the file is using px. Can we modify everything to ems?

Comment 5

[:Gijs (he/him)]

(In reply to Shashank VRSN Sabniveesu from comment #4)
> Everything in the file is using px. Can we modify everything to ems?

Sorry for the delay in response. No, not everything. The text is sized in em, and the background image in px. We need to make sure that the text and the image fit irrespective of the font size in use. You may need to use calc( NNem + MMpx) for some sizes in order to ensure that both text and image fit properly.

Does this seem like something you'll be able to do, and is that more clear?

Comment 6

[Shashank VRSN Sabniveesu]

(In reply to :Gijs Kruitbosch from comment #5)
> We need to make sure that the text and
> the image fit irrespective of the font size in use. You may need to use
> calc( NNem + MMpx) for some sizes in order to ensure that both text and
> image fit properly.
> 
Could you explain what you mean by 'NNem', 'MMpx' and that calculation?

Also, how do I test the changes I am making?

Comment 7

[:Gijs (he/him)]

(In reply to Shashank VRSN Sabniveesu from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > We need to make sure that the text and
> > the image fit irrespective of the font size in use. You may need to use
> > calc( NNem + MMpx) for some sizes in order to ensure that both text and
> > image fit properly.
> > 
> Could you explain what you mean by 'NNem', 'MMpx' and that calculation?

I meant "some number of 'em's" and "some number of 'px's" - so e.g. 5em + 70px or something of the sort.

The actual values of NN and MM would depend on the size of the image and the size of the text. It should be relatively easy to deduce them from the current size of the tip.


> Also, how do I test the changes I am making?

Assuming you've built Firefox, you can rebuild just the relevant CSS bits with

./mach build browser/themes

You can test what happens with different font sizes and/or DPI settings by adjusting those within the Windows settings. Here's some instructions on how to do that for Windows 7: http://windows.microsoft.com/en-us/windows7/Make-the-text-on-your-screen-larger-or-smaller?v=t ( I believe similar instructions should be easy to find for Windows 8)

Comment 8

[Felix Miata]

(In reply to :Gijs Kruitbosch from comment #7)
> You can test what happens with different font sizes and/or DPI settings by
> adjusting those within the Windows settings

Platform: x86 Linux is wrong then?

Comment 9

[:Gijs (he/him)]

(In reply to Felix Miata from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > You can test what happens with different font sizes and/or DPI settings by
> > adjusting those within the Windows settings
> 
> Platform: x86 Linux is wrong then?

Sorry, I made a wrong assumption that this was about Windows - but it should be reproducible on both Windows and Linux as both let you adjust DPI/font settings and they share CSS for this panel. OS X doesn't easily let you change DPI/font settings, so it probably isn't affected. Still, setting all/all.

Shashank, does my explanation in comment #7 and here help? :-)

Comment 10

[Shashank VRSN Sabniveesu]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Felix Miata from comment #8)
> > (In reply to :Gijs Kruitbosch from comment #7)
> > > You can test what happens with different font sizes and/or DPI settings by
> > > adjusting those within the Windows settings
> > 
> > Platform: x86 Linux is wrong then?
> 
> Sorry, I made a wrong assumption that this was about Windows - but it should
> be reproducible on both Windows and Linux as both let you adjust DPI/font
I might need too much information here as I am not well-versed with dpi settings and such display-related stuff for now.
I am working on 2 more bugs and a slightly bigger project and hence would leave this opportunity for others.

I'm sorry for getting into this but leaving without doing anything. It is just that I can't find a time slot for learning this stuff and experiment.

> settings and they share CSS for this panel. OS X doesn't easily let you
> change DPI/font settings, so it probably isn't affected. Still, setting
> all/all.
> 
> Shashank, does my explanation in comment #7 and here help? :-)
Thanks for your patience, Gijs. I believe I can ask for your help in future on different works.

-- Shashank

Comment 11

[Luke]

Attachment: Replaces px values with em for text

This patch replaces px values with ems where sensible. (I am assuming we still want images to be their native size.) Based on the advice here, am I incorrect in my assumption that firefox css land pixels are the same as w3c pixels, scaling with dpi and intended viewing distance? In any case, it's likely a px value is being inherited as an em from somewhere else (look at the rest of the customisation ui in the background of the screenshots), and back to the same problem... or it's really true that I just can't test this with Win8.1 fancy scaling.

Comment 12

[Felix Miata]

1-Don't add CC for any bug reporter's bug. They automatically get bugmail.

2-(In reply to Luke from comment #11)
> I am assuming we still want images to be their native size.

Bitmap images rendered smaller on a higher DPI device as a practical matter are usually neither better nor worse than when scaled up from intrinsic size in same proportion as text. The difference between discernable detail when intrinsically too small for the apparent screen density and the absence of detail when scaled up is only a matter of taste, not functionally available detail. IOW, I don't think, in general principle, maintaining intrinsic size is a good idea if scaling factor is much more than nominal. OTOH, preserving the relationship between text size and image size makes layout relationships easier to maintain. 

> Based on the advice here, am I incorrect in my assumption that firefox css land 
> pixels are the same as w3c pixels

I'm fuzzy on this, but I think for UI, the answer is they are not the same when density is more than nominally above 96 DPI, at least in Linux environments I'm familiar with (which excludes Gnome).

> scaling with dpi and intended viewing distance? ... or it's really true that I just
> can't test this with Win8.1 fancy scaling.

These parts of comment either I don't understand, or am unequipped to respond to, with no available post-XP Windows licenses.

Comment 13

[:Gijs (he/him)]

(In reply to Felix Miata from comment #12)
> 1-Don't add CC for any bug reporter's bug. They automatically get bugmail.

That's not anybody's fault but bugzilla, which does that automatically when you needinfo someone.

> 2-(In reply to Luke from comment #11)
> > I am assuming we still want images to be their native size.
> 
> Bitmap images rendered smaller on a higher DPI device as a practical matter
> are usually neither better nor worse than when scaled up from intrinsic size
> in same proportion as text. The difference between discernable detail when
> intrinsically too small for the apparent screen density and the absence of
> detail when scaled up is only a matter of taste, not functionally available
> detail. IOW, I don't think, in general principle, maintaining intrinsic size
> is a good idea if scaling factor is much more than nominal. OTOH, preserving
> the relationship between text size and image size makes layout relationships
> easier to maintain. 
> 
> > Based on the advice here, am I incorrect in my assumption that firefox css land 
> > pixels are the same as w3c pixels
> 
> I'm fuzzy on this, but I think for UI, the answer is they are not the same
> when density is more than nominally above 96 DPI, at least in Linux
> environments I'm familiar with (which excludes Gnome).
> 
> > scaling with dpi and intended viewing distance? ... or it's really true that I just
> > can't test this with Win8.1 fancy scaling.
> 
> These parts of comment either I don't understand, or am unequipped to
> respond to, with no available post-XP Windows licenses.

I'll test the patch either tomorrow or Monday and report back. Thanks Luke and Felix!

Comment 14

[Luke]

Attachment: Replaces all px values with em plus hidpi images

(In reply to Felix Miata from comment #12)
> 1-Don't add CC for any bug reporter's bug. They automatically get bugmail.

Sorry! I'll try to remember never to flag reporters.

> Bitmap images rendered smaller on a higher DPI device as a practical matter
> are usually neither better nor worse than when scaled up from intrinsic size
> in same proportion as text. The difference between discernable detail when
> intrinsically too small for the apparent screen density and the absence of
> detail when scaled up is only a matter of taste, not functionally available
> detail. IOW, I don't think, in general principle, maintaining intrinsic size
> is a good idea if scaling factor is much more than nominal. OTOH, preserving
> the relationship between text size and image size makes layout relationships
> easier to maintain. 

Fair enough -- that seems perfectly logical now I'm more awake... /facepalm There's actually 2x images in the folder; I think I found the right way to include them, referencing the patch in Bug 781327.

> > Based on the advice here, am I incorrect in my assumption that firefox css land 
> > pixels are the same as w3c pixels
> 
> I'm fuzzy on this, but I think for UI, the answer is they are not the same
> when density is more than nominally above 96 DPI, at least in Linux
> environments I'm familiar with (which excludes Gnome).
> 
> > scaling with dpi and intended viewing distance? ... or it's really true that I just
> > can't test this with Win8.1 fancy scaling.
> 
> These parts of comment either I don't understand, or am unequipped to
> respond to, with no available post-XP Windows licenses.

I looked it up earlier today, and "responsive pixel" was the term I was after. To explain myself more clearly: if themes use responsive pixels like regular css, then won't the suggested solution in c3 of converting to em units do nothing, since responsive pixels already scale to appear the same size at any given dpi? The only way I understand this could work is if themes override this behaviour so pixels mean physical screen pixels, not responsive pixels, which afaik they do not. (But I am about as far from an expert as you can get, so hopefully it'll result in me learning something and not a failure :D)

Comment 15

[Luke]

Attachment: Replaces all px values with em plus hidpi images

Some wrong line endings slipped in, meh...

Comment 16

[:Gijs (he/him)]

Comment on attachment 8440325 [details] [diff] [review]
Replaces all px values with em plus hidpi images

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

So, some feedback from me:

- the border-radius should stay in px
- the sizing of images (or elements with just images as backgrounds) should stay in px
- the sizing of containers of both images and backgrounds should be in calc( em + px), where the px value depends on the size of the contained image, and the em value on the size of the text...

And actually, I think the real issue here might be that this code hardcodes the text size to a px value. I don't know why it does that, but it's wrong. Mike?

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #16)
> - the sizing of containers of both images and backgrounds should be in calc(
> em + px), where the px value depends on the size of the contained image, and
> the em value on the size of the text...

Egh. I meant *both images and text*. Sorry.

Comment 18

[Luke]

I didn't notice it before, but the root element has a rule set in a block I presumed would be cascaded over: "-x-system-font: message-box". There's no documentation for it, but (after much headbanging) it seems that corresponds to a value of: "font: message-box", which is listed on MDN. Apparently, Windows (and Linux by the sounds of it) treats that as locking the font-size to 12px, amongst other things. We can't change that without affecting the entire window.

Presuming we want to keep the same font-style, overwriting the local font-size and allowing it to inherit seems to work, like so: "font-size: -moz-use-system-font". It's then just a matter of some small tweaks, like removing other pixel references within the stylesheet. Does this sound like the way to go?

Comment 19

[Luke]

Attachment: Override font-size to system-font plus hidpi images

Here's an example without adjusting the container dimensions, so the text remains bound by the image width. Afaik, best way to fix that is tinker with the XUL a little, add a spacer to the right of the image so the text can expand further.

Comment 20

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #16)
> And actually, I think the real issue here might be that this code hardcodes
> the text size to a px value. I don't know why it does that, but it's wrong.
> Mike?

That's by design; the spec in bug 870593 specified this.

Comment 21

[Mike de Boer [:mikedeboer]]

Hmm, except that in the spec there is a line 'use platform specific fonts'... but I applied that only to font-family... So I think it's preferred to use an 'em' value here (as we usually do) or omit setting the font-size entirely if it's unnecessary.

Comment 22

[bilal]

Hi would like join in helping to fix this bug so what do I need?

Comment 23

[:Gijs (he/him)]

Comment on attachment 8442091 [details] [diff] [review]
Override font-size to system-font plus hidpi images

Renewing feedback request per comment 21.

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8442091 [details] [diff] [review]
Override font-size to system-font plus hidpi images

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

Thanks for the patch, Luke! Great to see this work progressing.

This generally looks very good to me, but an important thing is that the rules for the OSX theme at http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#4665 will not be necessary anymore. Can you remove the lines that apply to the Customize Tip?

::: browser/themes/shared/customizableui/customizeTip.inc.css
@@ +26,5 @@
>    background-repeat: no-repeat;
>  }
>  
> +@media (min-resolution: 2dppx) {
> +

nit: no extra newline necessary at the start and end of a media query block.

@@ +28,5 @@
>  
> +@media (min-resolution: 2dppx) {
> +
> +  .customization-tipPanel-infoBox {
> +    background-image: url(chrome://browser/skin/customizableui/[e-mail]);

Did you test this? The background-size is not defined for the info icon yet, so it'll probably blow up like this...