Bug 919703 made the code mirror find/go to line more firefox-ey, but doesn't match the devtools theme.

In dark theme, you can notice that the toolbar height next to it isn't the same as the code mirror find bar height

(In reply to Tim Nguyen [:ntim] from comment #1) > Created attachment 8377258 [details] > Screenshot of inconsistency in dark theme > > In dark theme, you can notice that the toolbar height next to it isn't the > same as the code mirror find bar height Note that the toolbars are going to be thinned by quite a bit in Bug 942292. Such that we may actually need to shrink the height of the CM find/replace to match.

(In reply to Brian Grinstead [:bgrins] from comment #2) > (In reply to Tim Nguyen [:ntim] from comment #1) > > Created attachment 8377258 [details] > > Screenshot of inconsistency in dark theme > > > > In dark theme, you can notice that the toolbar height next to it isn't the > > same as the code mirror find bar height > > Note that the toolbars are going to be thinned by quite a bit in Bug 942292. > Such that we may actually need to shrink the height of the CM find/replace > to match. Yep, also, the light theme look should be fixed too. The CM find/replace looks dark in light theme. Maybe it should also have the .devtools-toolbar class.

This fixes the issue. Although, I'm not sure if I'm allowed to touch to CodeMirror files.

Now with r=bgrins. Also, the toolbar heights don't match, but since we're gonna change it soon, I don't think it's worth it.

Comment on attachment 8423338 [details] [diff] [review] Patch v1.1 Review of attachment 8423338 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, we want to avoid touching dialog.css and dialog.js if at all possible. I would keep any necessary .CodeMirror-dialog overrides in mozilla.css, and possibly even just add .CodeMirror-dialog into the list of selectors with devtools-toolbar in light-theme, dark-theme, and toolbars.inc.css (if needed). If there are only a couple of colors that we are wanting from the devtools-toolbar class (and don't care about padding and other props), I would probably just copy those in with theme-light and theme-dark overrides in mozilla.css.

Comment on attachment 8424098 [details] [diff] [review] Patch v2 Review of attachment 8424098 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/toolbars.inc.css @@ +10,5 @@ > %define solidSeparatorLight linear-gradient(#aaa, #aaa) > > > +.devtools-toolbar, > +.CodeMirror-dialog { I'm thinking about this one some more. I'm happy with adding CodeMirror dialog to the list in the light and dark theme files. But I don't think it needs to track exactly with all the other settings (and this could make changes on the toolbar in the future harder). I would actually just copy the padding/border rules into mozilla.css in a .CodeMirror-dialog rule

Put the padding change into a temporary CodeMirror-dialog rule. I suppose you'll remove it later when you'll thin out the toolbars.

Comment on attachment 8424192 [details] [diff] [review] Patch v2.1 Review of attachment 8424192 [details] [diff] [review]: ----------------------------------------------------------------- Nice refactor

I don't think

(In reply to Tim Nguyen [:ntim] from comment #12) > I don't think *that a try push is needed, since this is just a small CSS fix.

Comment on attachment 8424192 [details] [diff] [review] Patch v2.1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 919703 User impact if declined: Find/go to line in DevTools source editor looks bad Testing completed (on m-c, etc.): on m-c for a while Risk to taking this patch (and alternatives if risky): Very low, CSS only String or IDL/UUID changes made by this patch: None

(not required for uplifts)

Hi, I was able to reproduce it in Nightly 30.0a1 2014-03-05 in both Linux x86_64 and Windows 7 x86_64 and can confirm that this is fixed in the latest Aurora (31.0a2 2014-06-04) and latest Nightly (32.0a1 2014-06-04) on both platforms. Having verified it on two platforms out of three, I'm changing the status to Verified. Cheers, Francesca