This is part of the work tracked in bug 1470830.

Comment on attachment 8990356 [details] Bug 1473932 - Load "findBar.css" as a document stylesheet. https://reviewboard.mozilla.org/r/255416/#review262240

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99f8dee09073fac09e37492b7f2938cdc477fdc Screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c06d6493b8b1a421e8f40adb512d3f4f1c38e8a4 Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a0f9ed2fce946043b3e86ce85f9f5086b78bef2

The "findbar" element is only used in the browser window, so instead of increasing the specificity of the browser rules I applied the overridden properties to the Toolkit styles directly. I haven't moved everything from "toolbarbuttons.inc.css" to "findBar.css" because there are some browser-specific CSS variables referenced there. Ideally we should move the entire component to the "browser" folder, but it's probably simpler to do this during or after the conversion to Custom Element.

(In reply to :Paolo Amadini from comment #5) > The "findbar" element is only used in the browser window, This is true only as of recently, since support for view source in a separate window has been removed. It doesn't seem certain to me that we'll never again want a find bar outside of the browser window. E.g. I can imagine devtools using this somewhere once we've migrated from XUL to HTML. There's also other applications such as Thunderbird and SeaMonkey that we shouldn't completely forget about. That said, it's not clear to me why browser/themes/osx/browser.css overrides findBar.css in the first place. Is there anything browser.xul-specific to these overrides that wouldn't work in a different context?

(In reply to Dão Gottwald [::dao] from comment #7) > (In reply to :Paolo Amadini from comment #5) > > The "findbar" element is only used in the browser window, > > This is true only as of recently, since support for view source in a > separate window has been removed. It doesn't seem certain to me that we'll > never again want a find bar outside of the browser window. E.g. I can > imagine devtools using this somewhere once we've migrated from XUL to HTML. > There's also other applications such as Thunderbird and SeaMonkey that we > shouldn't completely forget about. I'd be OK with not ultimately moving the styles (and implementation) to the browser/ folder - don't have a strong preference about it. > That said, it's not clear to me why browser/themes/osx/browser.css overrides > findBar.css in the first place. Is there anything browser.xul-specific to > these overrides that wouldn't work in a different context? I don't see any real problem with moving the .findbar-button style into toolkit/ as the patch does. If TB/SM want to then override the style to restore previous toolkit look that could be done in app-specific sheets.

Comment on attachment 8990356 [details] Bug 1473932 - Load "findBar.css" as a document stylesheet. https://reviewboard.mozilla.org/r/255416/#review266840 ::: browser/themes/osx/browser.css (Diff revision 2) > -.findbar-button { > - background: none; > - box-shadow: none; > - border: none; > - color: inherit; > -} So, how do these buttons look with this moved to findBar.css *without* the toolbarbuttons.inc.css overrides? Judging by https://hg.mozilla.org/mozilla-central/rev/45e2731926803cf9020bbc46ddec11e3ce103b6b it looks like this should either stay here or move to toolbarbuttons.inc.css along with the other overrides...

(In reply to Dão Gottwald [::dao] from comment #9) > So, how do these buttons look with this moved to findBar.css *without* the > toolbarbuttons.inc.css overrides? They don't get the padding. > Judging by > https://hg.mozilla.org/mozilla-central/rev/ > 45e2731926803cf9020bbc46ddec11e3ce103b6b it looks like this should either > stay here or move to toolbarbuttons.inc.css along with the other overrides... I'm not sure what the original intent of this old changeset is, but I suppose you're noting that the styling was supposed to match other toolbar buttons, and thus should be defined in the same place? This may have regressed, since at least on Mac there's no visible border radius around the findbar buttons, while it is present for other buttons. I'm not sure if this is a bug and if we want to fix it while we're touching the styles here. Anyways, I'm doing a lot of guesswork and it's also not clear to me what kind of toolkit/browser separation is appropriate here, as I don't have the historical context to determine the right solution. Dão, would you have some time to go through the findbar styles and help us with fixing them properly for the conversion to a document stylesheet? While there are only a few changed lines, every one of them matters, and it may be faster overall if you show the solution directly with code.

I think this should do it. I haven't actually tested this on Mac, but I could do so using a loaner device if you can't.

Comment on attachment 8998764 [details] [diff] [review] patch Thank you Dão! I've tested on Mac and this looks good to me there.

Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eacbf25c0f Load findBar.css as a document stylesheet. r=paolo

Backed out changeset a0eacbf25c0f (Bug 1473932) for bc failures in toolkit/content/tests/browser/browser_findbar.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a0eacbf25c0ffcb0f60631c27391c19230d7f962&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=193022651 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193022651&repo=mozilla-inbound&lineNumber=9396 Backout push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=09fbeb118af3db3c5092603339f025164a01b337&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

(In reply to Dão Gottwald [::dao] from comment #17) > Created attachment 8999142 [details] [diff] [review] > patch with browser_findbar.js fix > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a61a79c58a55d8df8686aadbe21d69d639dda307 FWIW, I'm not sure this will help, as I'm not sure this is really a test issue. Apparently we're now faster loading the find bar and I'm afraid this might have uncovered a real race condition in production code.

(In reply to Dão Gottwald [::dao] from comment #17) > Created attachment 8999142 [details] [diff] [review] > patch with browser_findbar.js fix > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a61a79c58a55d8df8686aadbe21d69d639dda307 Still failing on Linux x64 opt, but instead of timing out, we get: TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_findbar.js | abc fully entered as find query - Got ab, expected abc Still not quite sure what's going on there.

(In reply to Dão Gottwald [::dao] from comment #19) > (In reply to Dão Gottwald [::dao] from comment #17) > > Created attachment 8999142 [details] [diff] [review] > > patch with browser_findbar.js fix > > > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=a61a79c58a55d8df8686aadbe21d69d639dda307 > > Still failing on Linux x64 opt, but instead of timing out, we get: > > TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_findbar.js | > abc fully entered as find query - Got ab, expected abc > > Still not quite sure what's going on there. Here's a version that explicitly waits for three keypress events, and it times out again on Linux x64 opt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d6d2bc5d5f04890717f34eaa845a4bdd05f1d88&selectedJob=193261629

I don't have time to dig deeper right now.

Thanks for looking into this! I'm wondering if folding the patch with the migration to Custom Element in bug 1411707 makes a difference? We may investigate it when we resume work on landing the other bug, which is unblocked now that we have tested and approved the CSS changes.

Maybe this: <resources> <stylesheet src="data:text/css,"/> </resources> could be a possible workaround, similar to what's been done for popup.css.

I'm going to try and handle this in Bug 1411707