GNOME Bugzilla – Bug 744680
Track wip/sass code changes
Last modified: 2015-05-13 12:55:24 UTC
Here are the code changes to be reviewed
Created attachment 297052 [details] [review] appDisplay: Show a dot when application is running Show a dot in the icon of running applications. Design request.
Created attachment 297053 [details] [review] panelMenu: Use a normal St.Bin to allow styling Currently we don't add some style to ButtonBox like padding etc, we add it programatically taking into account minimum padding and natural padding. The thing is that with natural padding it works as expected even for low resolutions. As a design request, we need to style from css, and since the current ButtonBox class doesn't add any worth functionality, change that and use a simple St.Bin that allow normal styling.
Created attachment 297054 [details] [review] panelMenu: Don't fade appMenuButton icon and put it on the side Design request, don't fade the icon of the appMenuButton and put it on the side instead of overlaping with the text.
Created attachment 297055 [details] [review] popupMenu: Separate active and hover state So we can style both diferently
Review of attachment 297052 [details] [review]: *So* much better than the previous version on the branch - no more weird stuff in the code, and dash items are Fitts'able again. Chapeau! ::: js/ui/appDisplay.js @@ +1636,1 @@ this.actor.add_style_class_name('running'); With the running dot in the code - do we still need the .running class? Not that it hurts, but it looks unused now ...
Review of attachment 297053 [details] [review]: This breaks keyboard and a11y indicators, see https://git.gnome.org/browse/gnome-shell/commit/?h=wip/sass&id=49aec95cb864c6f698. But even with those fixes, the change looks questionable to me. "We need to style from CSS" is an odd justification, given that the current padding is based on values taken from ... CSS! For context, bug 651299 is where the flexible padding was introduced. With tablets, portrait orientation is now more a thing than it was back then, so if we regress there, I want a better justification for it. (And yes, when I lower the resolution and switch to portrait mode, the original issue is back with this patch).
Review of attachment 297054 [details] [review]: Apart from the comments below, I'd like to see a follow-up patch that removes shell_app_get_faded_icon(). ::: js/ui/panel.js @@ +204,1 @@ Lang.bind(this, this._updateIconBoxClip)); ^^^All those app-icon-bottom-clip shenanigans are obsolete now as well - the panel highlight used to be implemented by using a background image, but with the app-icon being taller than the top bar itself, it overlapped the :active indicator. That's why the bottom was clipped away in those cases, to create the illusion that the underline was over the icon. No longer an issue, kill with fire! @@ +287,3 @@ return; + let icon = this._targetApp.create_icon_texture(2 * PANEL_ICON_SIZE); I guess that should be simply PANEL_ICON_SIZE now?
Review of attachment 297055 [details] [review]: I'm sympathetic to the idea of making :active follow the CSS semantics, however this patch looks fairly half-baked. I'd also like to point out that this is hardly the only place where we abuse the :active pseudo class to mean "active item" - there's also "button with associated open menu", "currently selected day in calendar" etc. ::: js/ui/popupMenu.js @@ +140,2 @@ _onKeyFocusIn: function (actor) { + this._setHover(true); I'm a bit wary that this interferes with the whole popup-menu machinery - setActive() will *also* set the active property and emit 'active-changed', so this not just merely changing a style class. I can't point to anything in particular, but I would not be surprised if something will subtly break with this change. @@ +168,3 @@ this.active = active; if (active) { + this.actor.add_style_pseudo_class('hover'); Using :active for the active menu item is a bit questionable, because it has actual meaning in CSS. However the same is true for :hover, which does not mean "the keyboard focus moved here" or "the menu item has been selected programmatically" either. So maybe the best option is to not pick anything that already has semantics attached to it - a bit of a cop-out, but I suggest '.active' (i.e. a style class rather than a pseudo class). .active:active looks stupid, but hey, it's fun :-) @@ +1136,3 @@ _onButtonReleaseEvent: function(actor) { this._setOpenState(!this._getOpenState()); + this.actor.remove_style_pseudo_class('active'); Doing this only for SubMenuMenuItems is fairly random. If we do this, we should do it right, i.e. for *all* menu items. It would also be proper to set :active for Space/Return key press/release pairs, but then we activate on press. However touch events definitively should be included here as well. Also: the code does not appear to deal with press and release events on different menu items, e.g. hover an item, press the button (but don't release it), move to another menu item and release. I think it is safe to assume that only the "active" (popup menu) item may be ":active" (css). So maybe remove the :active pseudo class from setActive(false) as well?
(In reply to Florian Müllner from comment #6) > Review of attachment 297053 [details] [review] [review]: > > This breaks keyboard and a11y indicators, see > https://git.gnome.org/browse/gnome-shell/commit/?h=wip/ > sass&id=49aec95cb864c6f698. > > But even with those fixes, the change looks questionable to me. "We need to > style from CSS" is an odd justification, given that the current padding is > based on values taken from ... CSS! > For context, bug 651299 is where the flexible padding was introduced. With > tablets, portrait orientation is now more a thing than it was back then, so > if we regress there, I want a better justification for it. (And yes, when I > lower the resolution and switch to portrait mode, the original issue is back > with this patch). indeed, putting the correct values to natural-hpadding and -minimum-hpadding solves the problem I was trying to fix with this patch. General padding doesn't work in this way, but is enough for us since we only want horizontal padding.
Review of attachment 297053 [details] [review]: no longer needed
(In reply to Florian Müllner from comment #8) > Review of attachment 297055 [details] [review] [review]: > > I'm sympathetic to the idea of making :active follow the CSS semantics, > however this patch looks fairly half-baked. I'd also like to point out that > this is hardly the only place where we abuse the :active pseudo class to > mean "active item" - there's also "button with associated open menu", > "currently selected day in calendar" etc. > > ::: js/ui/popupMenu.js > @@ +140,2 @@ > _onKeyFocusIn: function (actor) { > + this._setHover(true); > > I'm a bit wary that this interferes with the whole popup-menu machinery - > setActive() will *also* set the active property and emit 'active-changed', > so this not just merely changing a style class. I can't point to anything in > particular, but I would not be surprised if something will subtly break with > this change. > > @@ +168,3 @@ > this.active = active; > if (active) { > + this.actor.add_style_pseudo_class('hover'); > > Using :active for the active menu item is a bit questionable, because it has > actual meaning in CSS. However the same is true for :hover, which does not > mean "the keyboard focus moved here" or "the menu item has been selected > programmatically" either. > So maybe the best option is to not pick anything that already has semantics > attached to it - a bit of a cop-out, but I suggest '.active' (i.e. a style > class rather than a pseudo class). .active:active looks stupid, but hey, > it's fun :-) > > @@ +1136,3 @@ > _onButtonReleaseEvent: function(actor) { > this._setOpenState(!this._getOpenState()); > + this.actor.remove_style_pseudo_class('active'); > > Doing this only for SubMenuMenuItems is fairly random. If we do this, we It's because is the only class which overrides buttonReleaseEvent. I actually do it in the parent for everybody else. > should do it right, i.e. for *all* menu items. It would also be proper to > set :active for Space/Return key press/release pairs, but then we activate > on press. However touch events definitively should be included here as well. As you said we activate on press, so not needed no? Or better just do it for consistency? > Also: the code does not appear to deal with press and release events on > different menu items, e.g. hover an item, press the button (but don't > release it), move to another menu item and release. I think it is safe to > assume that only the "active" (popup menu) item may be ":active" (css). So > maybe remove the :active pseudo class from setActive(false) as well?
(In reply to Carlos Soriano from comment #11) > > Doing this only for SubMenuMenuItems is fairly random. If we do this, we > It's because is the only class which overrides buttonReleaseEvent. I > actually do it in the parent for everybody else. Not in the patch I reviewed, nor in the current branch.
Created attachment 297108 [details] [review] popupMenu: Add active CSS pseudo class So we can style it differently than :hover. We already have a active state for the menu items which includes more than hover. For example, when the keyboard focus moves to a item or we select programatically a item. For this reason we need a style class named active for the meaning we give to it in menu items, and a pseudo class active with the meaning CSS has.
(In reply to Florian Müllner from comment #12) > (In reply to Carlos Soriano from comment #11) > > > Doing this only for SubMenuMenuItems is fairly random. If we do this, we > > It's because is the only class which overrides buttonReleaseEvent. I > > actually do it in the parent for everybody else. > > Not in the patch I reviewed, nor in the current branch. sorry for that, I messed up.
Created attachment 297115 [details] [review] panelMenu: Don't fade appMenuButton icon and put it on the side Design request, don't fade the icon of the appMenuButton and put it on the side instead of overlaping with the text.
Created attachment 297116 [details] [review] shell-app: Remove shell_app_get_faded_icon In previous commit we removed the only use of shell_app_get_faded_icon so we can remove the function on shell-app as well.
Created attachment 297120 [details] [review] panelMenu: Don't fade appMenuButton icon and put it on the side Design request, don't fade the icon of the appMenuButton and put it on the side instead of overlaping with the text.
Review of attachment 297115 [details] [review]: Some more chances for code removal: ::: js/ui/panel.js @@ +198,3 @@ Lang.bind(this, this._onIconThemeChanged)); this._iconBox = new Shell.Slicer({ name: 'appMenuIcon' }); Could use a regular StBin now, and possibly junk ShellSlicer as well @@ +203,1 @@ this._label = new TextShadower(); We can probably do without this now as well
Review of attachment 297116 [details] [review]: LGTM
Review of attachment 297108 [details] [review]: ::: js/ui/popupMenu.js @@ +175,3 @@ } else { + this.actor.remove_style_class_name('active'); + this.actor.remove_style_pseudo_class ('active'); Isn't the whole point here to get rid of the .active <-> :active mixup?
Review of attachment 297055 [details] [review]: another has been updated
(In reply to Florian Müllner from comment #18) > Review of attachment 297115 [details] [review] [review]: > > Some more chances for code removal: > > ::: js/ui/panel.js > @@ +198,3 @@ > Lang.bind(this, this._onIconThemeChanged)); > > this._iconBox = new Shell.Slicer({ name: 'appMenuIcon' }); > > Could use a regular StBin now, and possibly junk ShellSlicer as well > > @@ +203,1 @@ > this._label = new TextShadower(); > > We can probably do without this now as well done
Attachment 297052 [details] pushed as 6cf53a8 - appDisplay: Show a dot when application is running Attachment 297108 [details] pushed as 8d66fff - popupMenu: Add active CSS pseudo class Attachment 297116 [details] pushed as ef3285d - shell-app: Remove shell_app_get_faded_icon Attachment 297120 [details] pushed as 3732a68 - panelMenu: Don't fade appMenuButton icon and put it on the side
Please, help me. I found this commit https://git.gnome.org/browse/gnome-shell/commit/js/ui/appDisplay.js?id=6cf53a8d1cb7453336c027f7e6944321b6d49cb8 I want apply old style for running program. i don't want see strange blue stripe for running program. How to do this?
(In reply to vitalik_p from comment #24) > Please, help me. > > I found this commit > https://git.gnome.org/browse/gnome-shell/commit/js/ui/appDisplay. > js?id=6cf53a8d1cb7453336c027f7e6944321b6d49cb8 > > I want apply old style for running program. > i don't want see strange blue stripe for running program. > > How to do this? Bugzilla is not intended for this. Ask on forums or IRC :)