1473932 - Load "findBar.css" as a document stylesheet

Status: RESOLVED DUPLICATE of bug 1411707

(Toolkit :: Find Toolbar, enhancement, P3)

Description

[:Paolo Amadini]

This is part of the work tracked in bug 1470830.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1473932 - Load "findBar.css" as a document stylesheet.

Review commit: https://reviewboard.mozilla.org/r/255416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255416/

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8990356 [details]
Bug 1473932 - Load "findBar.css" as a document stylesheet.

https://reviewboard.mozilla.org/r/255416/#review262240

Comment 3

[:Paolo Amadini]

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

Comment 4

[:Paolo Amadini]

Comment on attachment 8990356 [details]
Bug 1473932 - Load "findBar.css" as a document stylesheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255416/diff/1-2/

Comment 5

[:Paolo Amadini]

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.

Comment 6

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f70355d98524b823ea3985d3c20756f47fa77d45

Comment 7

[Dão Gottwald [:dao]]

(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?

Comment 8

[Brian Grinstead [:bgrins]]

(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 9

[Dão Gottwald [:dao]]

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...

Comment 10

[:Paolo Amadini]

(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.

Comment 11

[:Paolo Amadini]

Comment on attachment 8990356 [details]
Bug 1473932 - Load "findBar.css" as a document stylesheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255416/diff/2-3/

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

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 13

[Dão Gottwald [:dao]]

Attachment: patch

Comment 14

[:Paolo Amadini]

Comment on attachment 8998764 [details] [diff] [review]
patch

Thank you Dão! I've tested on Mac and this looks good to me there.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eacbf25c0f
Load findBar.css as a document stylesheet. r=paolo

Comment 16

[Eliza Balazs [:ebalazs_]]

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

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch with browser_findbar.js fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a61a79c58a55d8df8686aadbe21d69d639dda307

Comment 18

[Dão Gottwald [:dao]]

(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.

Comment 19

[Dão Gottwald [:dao]]

(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.

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch with browser_findbar.js fix v2

(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

Comment 21

[Dão Gottwald [:dao]]

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

Comment 22

[:Paolo Amadini]

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.

Comment 23

[Tim Nguyen :ntim]

Maybe this:

<resources>
  <stylesheet src="data:text/css,"/>
</resources>


could be a possible workaround, similar to what's been done for popup.css.

Comment 24

[Brian Grinstead [:bgrins]]

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