1364099 - Cleanup autocomplete css

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement)

Description

[Tim Nguyen :ntim]

See bug 1354504 comment 41. We may want to remove code that adds the theme classes as well.

Comment 1

[Ruturaj Vartak]

Hey Tim,
Referring to bug 1354504 comment 38, The CSS isn't present perhaps got changed in some other commit.
Can I know what the fix for this bug now changes to?
Thanks

Comment 2

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #1)
> Hey Tim,
> Referring to bug 1354504 comment 38, The CSS isn't present perhaps got
> changed in some other commit.
FYI, the last commit which changed the CSS significantly was bug 1354504: https://hg.mozilla.org/mozilla-central/filelog/58c5151bfd62de934b2286dbd664e69886270e28/devtools/client/themes/common.css

> Can I know what the fix for this bug now changes to?

- Removing the code that adds the theme classes to the popup
- Simplifying box-shadow and cursor rules

You might want to ask Julian about this.

Comment 3

[Ruturaj Vartak]

Attachment: fix-1364099-1.patch

Hey Julian from your comments below

> 1 - We already have a box-shadow defined for .devtools-autocomplete-popup on line 45 (with a slightly different value 0 1px 0 hsla(209,29%,72%,.25) inset;)

> 2 - I think this particular box-shadow here was only set for .devtools-autocomplete-popup in light-theme. Is there a good reason to also apply it to CodeMirror-hints & CodeMirror-Tern-tooltip? Both these classes already have box-shadows defined in light-theme.css and dark-theme.css

> 3 - In case you need to target .devtools-autocomplete-popup in the light-theme only, you should know that the light-theme classname is also added to the container when we are using the Firebug theme (an easy way for the firebug theme to inherit from the light theme). That means that you either need to have a very specific .light-theme:not(.firebug-theme) in your selector, or to override the value for firebug-theme.

> Now, I'm being very nit-picky here, for the sake of explaining the impact these kind of changes can have. We're talking about an almost invisible inset box shadow here, I don't think anybody would notice the difference between before and after your change :) So I don't really ask you to keep the colors 100% as they were before, let's just make sure we don't redefine the box-shadow property several times for no reason. To be honest I would just get rid of this box shadow: it's almost not noticeable, and when it is it feels dated. But it is easier if we do this kind of changes in a separate changeset.

It seems removal of box-shadow is the only thing. Could you please have a look?

Thanks,
Rutu

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8878769 [details] [diff] [review]
fix-1364099-1.patch

Review of attachment 8878769 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! 
Yes looks like this is the only thing left to update :)

Comment 5

[Ruturaj Vartak]

Thanks Julian

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dceb915d3c40
Cleanup autocomplete css. r=jdescottes

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dceb915d3c40