Tim Nguyen :ntim Assignee
	Description • 7 years ago

The text shadow is only used to make sure text is readable on image backgrounds.

In other situations, the text shadow looks quite odd. See attached screenshot.

	Mike Conley (:mconley) (:⚙️)
	Comment 2 • 6 years ago

Hey shorlander,

I think we need some clarification on this one from UX on how this API should work. Do we want to apply the text-shadow only when there's a background-image? Or do we want to have the API make it so that we can remove the text shadow from every single tab, regardless of whether there's a background-image? Do we want to make it possible to apply a lighter/less visible text-shadow?

I guess the question here is: what function does the text-shadow need to serve, and what do we think Theme API users should be able to override?

	Tom S [:evilpie]
	Comment 3 • 6 years ago

Just saw this on reddit: https://www.reddit.com/r/firefox/comments/7t6hmx/extensions_in_firefox_59/dtafuci/. Any update on this?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 4 • 6 years ago

Some notes from talking to Stephen,
- It's really only useful for really busy background images
- Don't need it on flat colored backgrounds
- Not sure we should make it something that developers need to worry about controlling

Based on those notes, we can move forward with only applying the text-shadow when a background-image is supplied.

	Tim Nguyen :ntim Assignee
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

https://reviewboard.mozilla.org/r/219374/#review225078

::: browser/themes/windows/compacttheme.css
(Diff revision 1)
> -  /* Make the menubar text readable on aero glass (copied from browser-aero.css). */
> -  #toolbar-menubar {
> -    text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
> -  }
> -

I removed it because it never gets applied (due to the text-shadow: none !important, which I've removed as well)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

https://reviewboard.mozilla.org/r/219374/#review225872

	Pulsebot
	Comment 9 • 6 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/85e30806ade1
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws

	Dorel Luca [:dluca]
	Comment 10 • 6 years ago

Backed out changeset 85e30806ade1 (bug 1404688) for Browser chrome failure on browser/base/content/test/general/browser_compacttheme.js. CLOSED TREE

Log of the fail:
https://treeherder.mozilla.org/logviewer.html#?job_id=162207133&repo=autoland&lineNumber=2410

Backout changeset:
https://hg.mozilla.org/integration/autoland/rev/0b7ae7a92e2be896a74f2b728c7fb8362dac8b98

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=85e30806ade11c6b7b7ba8aba07693266f326a85

	Cristina Coroiu [:ccoroiu]
	Comment 11 • 6 years ago

Also failed the screenshots jobs.

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162213826&repo=autoland&lineNumber=2390

	Pulsebot
	Comment 13 • 6 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/067ee834b07b
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws

	Cristina Coroiu [:ccoroiu]
	Comment 14 • 6 years ago

Backed out changeset 067ee834b07b (bug 1404688) for xpcshell failures at toolkit/mozapps/extensions/test/xpcshell/test_LightweightThemeManager.js

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=067ee834b07b32bc0fd3023ef510cbea6dad1aa2&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162322931&repo=autoland&lineNumber=2747

Backout: https://hg.mozilla.org/integration/autoland/rev/8c6dc94629fc116689e5499424319403307d6e3a

	Pulsebot
	Comment 18 • 6 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/58d315ce1f1b
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws

	Noemi Erli[:noemi_erli]
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/58d315ce1f1b

	Dão Gottwald [:dao]
	Comment 20 • 6 years ago

(In reply to Tim Nguyen :ntim from comment #6)
> Comment on attachment 8950095 [details]
> Bug 1404688 - Make headerURL optional and remove text-shadow when there is
> no headerURL.
> 
> https://reviewboard.mozilla.org/r/219374/#review225078
> 
> ::: browser/themes/windows/compacttheme.css
> (Diff revision 1)
> > -  /* Make the menubar text readable on aero glass (copied from browser-aero.css). */
> > -  #toolbar-menubar {
> > -    text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
> > -  }
> > -
> 
> I removed it because it never gets applied (due to the text-shadow: none
> !important, which I've removed as well)

Which was a bug, right? We do want that text shadow on glass.

Btw, I really don't like big browser/theme/ and toolkit/theme/ patches "sneaking in" via "WebExtensions: Themes". Please stop doing that or at least CC me (not ideal because others may be interested as well).

	Tim Nguyen :ntim Assignee
	Comment 21 • 6 years ago

(In reply to Dão Gottwald [::dao] from comment #20)
> Which was a bug, right? We do want that text shadow on glass.

I guess so, filed bug 1439789 for it.

> Btw, I really don't like big browser/theme/ and toolkit/theme/ patches
> "sneaking in" via "WebExtensions: Themes".

"WebExtensions: Themes" doesn't seem like an unreasonable choice for this bug to me, the patch touches the WE theme code (LWTConsumer.jsm + ext-theme.js + WE theme tests + extension loading code). 


Some stuff makes sense in "WebExtensions: Themes", I'll CC you if that's the case but you might still want to CC to the relevant meta bugs (themingapi-polish, themingapi-more-ui, ...), so you get notified about newly created child bugs too.

	Dão Gottwald [:dao]
	Comment 22 • 6 years ago

(In reply to Tim Nguyen :ntim from comment #21)
> (In reply to Dão Gottwald [::dao] from comment #20)
> > Which was a bug, right? We do want that text shadow on glass.
> 
> I guess so, filed bug 1439789 for it.
> 
> > Btw, I really don't like big browser/theme/ and toolkit/theme/ patches
> > "sneaking in" via "WebExtensions: Themes".
> 
> "WebExtensions: Themes" doesn't seem like an unreasonable choice for this
> bug to me, the patch touches the WE theme code (LWTConsumer.jsm +
> ext-theme.js + WE theme tests + extension loading code). 

Sure it does, but the theme/ parts aren't exactly trivial either. This could have been just two bugs each in their right component: 1. make headerURL optional, 2. remove text-shadow when there is no headerURL. Turns out there was already bug 1413144 for the first part with some relevant discussion.

Like I said this isn't just about me. Components matter because different people watch them. Please make an effort to use them accordingly.

	Chris Mills [:cmills]
	Comment 23 • 6 years ago

I've had a go at documenting this. I added a note to the relevant manifest ref page, and the Firefox 60 rel notes:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#images
https://developer.mozilla.org/en-US/Firefox/Releases/60#WebExtensions

Does this look OK?

	Tim Nguyen :ntim Assignee
	Comment 24 • 6 years ago

Looks fine, thanks!