Closed Bug 1308407 Opened 8 years ago Closed 8 years ago

Blurry menubar in Ubuntu/Unity with Firefox Developer Edition

Categories

(Firefox :: Theme, defect, P3)

Product:
Firefox
Component:
Theme
Platform:
Unspecified
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: nachtigall, Assigned: Gijs)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1308407 - part 1: update lwthemetextcolor attribute when devedition theme is used,
58 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1308407 - part 2: remove redundant/broken rules from devedition now we update lwtheme dark/brightness correctly,
58 bytes, text/x-review-board-request
dao
: review+
Details 
This bug is ripped out of Bug 1301670 (search "blurry", or see the screencast unity-no-menu-firefox-dev-edition.ogv in there).

Using "mozregression --good 2015-05-10 --bad 2015-05-11 --repo mozilla-aurora" I eventually settled with this Pushlog (which I cannot trace down further due to https://bugzilla.mozilla.org/show_bug.cgi?id=1263361#c6 unfortunately). So the Pushlog is pretty big:

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=8316a66bfed3&tochange=e2ce1aac996e
Hunt this down on Nightly (just learnt now that I can also enable Dev Theme on "normal" firefox). The regression was introduced here:

 8:00.70 INFO: Got as far as we can go bisecting nightlies...
 8:00.70 INFO: Last good revision: 8af276ab8636 (2015-03-31)
 8:00.70 INFO: First bad revision: 37ddc5e2eb72 (2015-04-01)
 8:00.70 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8af276ab8636&tochange=37ddc5e2eb72

 8:00.70 INFO: Switching bisection method to taskcluster
 8:00.70 INFO: Getting mozilla-central builds between 8af276ab8636 and 37ddc5e2eb72

Unfortunately, I cannot further bisect as there are no tasks that can be downloaded (too old?)

Anyway, the regression was introduced when instead of browser.devedition.theme.enabled pref (about:config) the Addon https://addons.mozilla.org/de/firefox/addon/devedition-theme-enabler/ was needed to switch Nightly to the Dark Dev Theme.

The pushlog also shows that there was work done on Bug 1148996 wrt to the devediton theme.

NIing Brian

(If you do not have Ubuntu, you can just run it in VirtualBox as outlined at https://bugzilla.mozilla.org/show_bug.cgi?id=1301670#c0 )
Flags: needinfo?(bgrinstead)
(In reply to Jens from comment #1)
> Anyway, the regression was introduced when instead of
> browser.devedition.theme.enabled pref (about:config) the Addon
> https://addons.mozilla.org/de/firefox/addon/devedition-theme-enabler/ was
> needed to switch Nightly to the Dark Dev Theme.

This doesn't make any sense. You don't need an add-on to switch Nightly to the dark devedition theme. We switched from using a pref to making the devedition theme a lightweight theme with some extra CSS. You can just use the addon manager to switch to devedition, and whether it's dark or light is controlled by the devtools theme (there's a switch for it in the devtools settings).

Does this problem reproduce without this add-on?

TBH it sounds like there's just some text shadow on the text that shouldn't be there, which might mean this is simply a Firefox theming bug. Have you tried using the browser toolbox to identify what's causing the blurriness?

(Brian is OoO right now, so clearing ni - will try to answer for him as I reviewed some of the stuff in question anyway.)
Flags: needinfo?(bgrinstead) → needinfo?(nachtigall)
Hi Gijs, 

I do not understand. In order to get the dark theme enabled on Nightly (starting with 2015-04-01) on I needed the Devedtion Theme Enabler addon. Are there other ways of doing it? (I was just pointed out the coincidence that the blurriness was introduced this moment)

> Does this problem reproduce without this add-on?

I do not know how to enabled Dark theme (not only in Devtools, but for tabs) without the addon.


Anyway, you're right. I should have just looked at the browser toolbox (I did not know it's so easy, sorry):

There is:
menubar:-moz-lwtheme > menu:not([open="true"]) {
    color: inherit;
    text-shadow: inherit;
}
which overrides the text-shadow none coming later:
menubar:-moz-lwtheme > menu {
    text-shadow: none;
}

Setting text-shadow to none fixes it. I would be glad if you could make this into a patch (or give me pointers on how to do it my own) since I do not have the full insight regarding possible side effects.

(NIing you, let me know if this is too "pushy", I do not know how strong you use NIing at mozilla)
Flags: needinfo?(nachtigall) → needinfo?(gijskruitbosch+bugs)
(In reply to Jens from comment #3)
> Hi Gijs, 
> 
> I do not understand. In order to get the dark theme enabled on Nightly
> (starting with 2015-04-01) on I needed the Devedtion Theme Enabler addon.
> Are there other ways of doing it? (I was just pointed out the coincidence
> that the blurriness was introduced this moment)
> 
> > Does this problem reproduce without this add-on?
> 
> I do not know how to enabled Dark theme (not only in Devtools, but for tabs)
> without the addon.

1) open Tools > Add-ons
2) switch to "Appearance"
3) click "Enable" next to "Developer Edition (disabled)"
ta-dah!

If it shows up with the 'light' rather than 'dark' developer edition theme, that's because the devtools are set up to use that theme. The devtools theme will be reflected in the rest of the UI when the devedition theme is in use (just like on Developer Edition / aurora itself).

> There is:
> menubar:-moz-lwtheme > menu:not([open="true"]) {
>     color: inherit;
>     text-shadow: inherit;
> }
> which overrides the text-shadow none coming later:
> menubar:-moz-lwtheme > menu {
>     text-shadow: none;
> }
> 
> Setting text-shadow to none fixes it. I would be glad if you could make this
> into a patch (or give me pointers on how to do it my own) since I do not
> have the full insight regarding possible side effects.

I'll have a look. Thanks for checking.

> (NIing you, let me know if this is too "pushy", I do not know how strong you
> use NIing at mozilla)

This is fine, thanks for pinging me. Gonna leave needinfo until this is sorted. :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Component: Widget: Gtk → Theme
Flags: needinfo?(gijskruitbosch+bugs)
Product: Core → Firefox
Comment on attachment 8800211 [details]
Bug 1308407 - fix text-shadow on dark devedition theme on Linux,

The underlying problem here is that the lwthemetextcolor attribute doesn't reflect the reality for both dev edition themes. We need to fix that.
Attachment #8800211 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8800211 [details]
> Bug 1308407 - fix text-shadow on dark devedition theme on Linux,
> 
> The underlying problem here is that the lwthemetextcolor attribute doesn't
> reflect the reality for both dev edition themes. We need to fix that.

How do you propose we do that?
Flags: needinfo?(dao+bmo)
By updating the lwthemetextcolor attribute in, I'm guessing, DevEdition._updateDevtoolsThemeAttribute or DevEdition._inferBrightness.
Flags: needinfo?(dao+bmo)
Priority: -- → P3
Attachment #8800211 - Attachment is obsolete: true
Can you summarize why observing another observer topic is needed here, in other words, why the current hooks in browser-devedition.js aren't sufficient?
(In reply to Dão Gottwald [:dao] from comment #11)
> Can you summarize why observing another observer topic is needed here, in
> other words, why the current hooks in browser-devedition.js aren't
> sufficient?

Because the existing observer, when I tested on my OS X build, gets hit before the LightweightThemeConsumer one. So we can add change the attribute in there... only to have LightweightThemeConsumer change it right back to what *it* thinks the right value is. So I decided the simplest thing was to ensure that we change it *after* LightweightThemeConsumer changes it, by using the notification it sends.

Re-reading this, I think I forgot to add a check that the subject of the notification is our own window, so I'll update the patch with that.
Can we move the code using the existing observer to the new one then?
(In reply to Dão Gottwald [:dao] from comment #15)
> Can we move the code using the existing observer to the new one then?

I don't know. It seems to me like there are dependencies there that would be problematic to move/reorder. AFAICT we add and enable stylesheets based on the observer, so this seemed the simplest thing that would address the problem in hand as you requested without risking regressing anything, e.g. triggering flashes of unstyled content while the extra stylesheet loads (which the current code tries to avoid).

Really, IMO the whole should ideally be refactored into a single JSM that has 1 or 2 observers and gets called into from browser.js to track individual windows, rather than having each window have its own (set of) observer(s). AFAICT we currently call _inferBrightness more often than necessary, for instance. Even if we did move everything to the second observer, we'd still be doing double work because I'm fairly sure something will already be calling ToolbarIconColor.inferFromText() after the lwtheme change, and we just do it again after we correct the lwtheme's text color.

But fixing all of that seems out of scope and significantly more work than just addressing this bug. Happy to deal with it in a followup, but this bug has already expanded in scope significantly and I don't think it makes sense to expand it further. Some of the simplification / refactoring would also be moot given some of the work Jared and Mike de Boer are doing with the "new style" themes, so IMO it's hard to justify doing lots of extra work on it right now.
Attachment #8806418 - Flags: review?(dao+bmo) → review+
Attachment #8806419 - Flags: review?(dao+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d0080b95fe
part 1: update lwthemetextcolor attribute when devedition theme is used, r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea142c25c76
part 2: remove redundant/broken rules from devedition now we update lwtheme dark/brightness correctly, r=dao
https://hg.mozilla.org/mozilla-central/rev/60d0080b95fe
https://hg.mozilla.org/mozilla-central/rev/eea142c25c76
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: