718015 - Widget should inherit CSS font settings from parent (Add-on bar)

Status: VERIFIED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm using a widget to display some text in the add-on bar. Per default the used font does differ from the one which is normally set for the add-on bar. This results into:

* Inconsistent font style and size dependent on the users browser settings
* Inconsistent text color (hard to read for dark Personas)
* Text not centered vertically

The Widget class should inherit all those CSS properties from the parent class and should not set its own.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Alex, I guess way to address this would be similar to thing you did in panel is not it ? Do you mind sharing links to an interesting bits, so someone could easily pick it up.

Thanks!

Comment 2

[Caspy7]

This problem makes addons with black text displayed in the addon bar useless when a dark persona is being used (AKA my setup).

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #1)
> Alex, I guess way to address this would be similar to thing you did in panel
> is not it ? Do you mind sharing links to an interesting bits, so someone
> could easily pick it up.

Alex, would you mind giving us some links or at least the one to the pull request? Thanks.

Comment 4

[Alexandre Poirot [:ochameau]]

Oh sorry, I missed the question in my bugmails...
Here is two hacks Irakli was thinking about, in panel API:
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/panel.js#L119-144
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/panel.js#L281-296

I do not think we will need the first one, but the second one is the one that fixes text color, but doesn't address font family differences.

So you should be able to do similar tricks there:
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/widget.js#L858-868

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Alex. So we should probably create an array with all the necessary computed CSS attributes we have to clone for the iframe. So far I can see a need for the color and all font related styles.

Alex, do you normally create an issue together with the bug report on Bugzilla?

Comment 6

[Alexandre Poirot [:ochameau]]

Do you mean opening a github issue?
No, we are not using github issues.

Comment 7

[David Guo [:davidg]]

I was able to extend the add-on bar's computed text color to the widgets and I find the results to be significantly better for dark personas. There are however personas which the add-on bar will have worse legibility. Some are due to position of background image that it happens to land on, and some due to background images.

For example: http://www.getpersonas.com/en-US/persona/456270 has white text but the portion of the image on the add-on bar has a mostly white background.

Thoughts?

Comment 8

[Caspy7]

Well, we can't change the image. I don't know if we should call this a fault of the persona designer.  How is this managed with text in the bookmarks or navigation bar?
I was guessing at fuzzy shadows and went to getpersonas.com to confirm and this seems to be the case. I'm guessing we look at the overall darkness/lightness of the chosen text and then choose the black or white accordingly for a shadow? 
I don't know, I'm just guessing here.

Can we not solve the problem in the same way with shadows?
If we can't inherit it, just make sure that we calculate it in the same manner to keep parity.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We can't to anything against the position of the background images. The author of that personas background is in charge of it.

Comment 10

[David Guo [:davidg]]

Inheriting the text shadows worked quite well (for OS X). Thanks for the suggestion.

Currently I think the add-on bar should extend the CSS properties color, font-family, font-size, font-weight, and text-shadow to widgets. I feel that this is a good enough subset to fix consistency and readability issues.

I purposely left out letter-spacing since I think it's ultimately the add-on author's decision. Are there any that I missed?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Probably no obvious ones. The aforementioned properties should make a big improvement. We could file follow-up bugs or update the patch if we missed something.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/352

Pointer to Github pull-request

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e9eb864bc77104617600fc9f3e5b7db6e0b42cb2
Merge pull request #352 from dglol/widgetfix

Bug 718015 - Extend toolbar's text style to widgets r=@ochameau

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, I have tested it with a couple of Personas but I was not able to get it working. The color of the widget text is not updated according to the Persona settings.

Please test on master or 1.6 with: http://www.getpersonas.com/de/persona/460010

Comment 15

[David Guo [:davidg]]

Hey Henrik, the text change doesn't take effect until the next time the widget is reloaded (via new window or browser startup). It does seem a little bit inconvenient if someone wants to test a specific persona but it was the simplest implementation at the time.

Alex pointed out to me that you can register a listener on the following observer service notification:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#275

Though, right now I'm not sure if this should be part of the feature.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to David Guo [:davidg] from comment #15)
> Hey Henrik, the text change doesn't take effect until the next time the
> widget is reloaded (via new window or browser startup). It does seem a
> little bit inconvenient if someone wants to test a specific persona but it
> was the simplest implementation at the time.

Ah, interesting. That works correctly.

> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> LightweightThemeManager.jsm#275
> 
> Though, right now I'm not sure if this should be part of the feature.

Personally I think it's kinda important to handle that observer notification because more and more users are making use of the background (Personas) feature. So I will file a follow-up bug for this specific further enhancement. Thanks again for the feedback.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed after further testing with SDK 1.6 and MemChaser.