Created attachment 200055 [details] [review]
combo-box-menu-item: Propagate style changes to the combo menu

ComboBoxMenuItems use ClutterClones to reconstruct the active item
in the associated ComboMenu to not impose a particular MenuItem type
in the menu. However, this results in style changes (for instance
those triggered by icon-theme or text-scaling-factor changes) of
the ComboBoxMenuItem not having a visual effect until the ComboBoxMenu
is shown.
As a fix, force a style update on the ComboBoxMenu when the item's
style changes.

Created attachment 200056 [details] [review]
combo-box-menu-item: Propagate :insensitive state to the combo menu

ComboBoxMenuItems use ClutterClones to reconstruct the active item
in the associated ComboMenu, so when making the ComboBoxMenuItem
insensitive, the item's content lacks a visual hint (as the actual
style information is taken from the associated ComboBoxMenu item).
As a fix, propagate the :insensitive state to the active ComboBoxMenu
item.

Review of attachment 200055 [details] [review]:

Makes sense.

Review of attachment 200056 [details] [review]:

::: js/ui/popupMenu.js
@@ +1527,3 @@
     _onSourceActorStyleChanged: function() {
+        let activeItem = this._getMenuItems()[this._activeItemPos];
+        if (this.sourceActor.pseudo_class.match(/ *insensitive */))

Is there something wrong with has_style_pseudo_class?

Created attachment 200060 [details] [review]
combo-box-menu-item: Propagate :insensitive state to the combo menu

(In reply to comment #4)
> Is there something wrong with has_style_pseudo_class?

Nothing, except that I somehow remembered the has_foo_class() methods as being static and didn't bother to check ;-)

Review of attachment 200060 [details] [review]:

A comment would be nice. Besides that, should we propagate any other pseudo classes? If so:

    activeItem.pseudo_class = this.sourceActor.pseudo_class;

is probably clearer.

Created attachment 200066 [details] [review]
combo-box-menu-item: Propagate :insensitive state to the combo menu

(In reply to comment #6)
> Review of attachment 200060 [details] [review]:
> 
> A comment would be nice.

Sure.


> Besides that, should we propagate any other pseudo
> classes?

I was going to say 'no', but changed my mind while writing the comment. The pseudo classes relevant for PopupMenuItems are

 :open (specific to Submenus)
 :alternate (specific to AlternatingMenuItem)
 :active

So only the last one is relevant here, and it's a tricky case - just copying the style class is buggy: if the active item changes *after* hovering the ComboBoxMenuItem but *before* activating it, the popup menu ends up with two items being marked :active (:insensitive on the other hand is inherently safe, as the ComboBoxMenu cannot be triggered at all in that case).
Given that (at least at the moment) :active does not change anything that needs propagating, I had left it out originally. I figured that setActive() propagates the change correctly, so I've included it now - still, changes to e.g. the grab handling in setActive() may now have unintended side effects in the combo box, so I'd still consider leaving it out ...


> If so:
> 
>     activeItem.pseudo_class = this.sourceActor.pseudo_class;
> 
> is probably clearer.

I agree it's clearer - I originally did this, that's how I found the issue mentioned above.


Aaaanyway, I forgot to update the commit message, so if we decide to propagate :active after all, I know it'll need updating ;-)

Review of attachment 200066 [details] [review]:

Looks good.

Comment on attachment 200066 [details] [review]
combo-box-menu-item: Propagate :insensitive state to the combo menu

Speaking of "changes to grab handling in setActive() having unintended side effects" - it was only working because I was testing some (unrelated) patch ...

To fix, we'd need to do more than copying the style, less than setActive() ...

Created attachment 200090 [details] [review]
combo-box-menu-item: Propagate pseudo classes to the combo menu

ComboBoxMenuItems use ClutterClones to reconstruct the active item
in the associated ComboMenu, so pseudo class changes due to state
changes of the ComboBoxMenuItem don't have the intended effect
(since the actual style information is taken from the associated
ComboBoxMenu item).
As a fix, propagate relevant pseudo class changes to the active
ComboBoxMenu item.

Review of attachment 200090 [details] [review]:

I believe this went through IRC review, but I forget the outcome.

Attachment 200055 [details] pushed as d20e646 - combo-box-menu-item: Propagate style changes to the combo menu
Attachment 200090 [details] pushed as b88657a - combo-box-menu-item: Propagate pseudo classes to the combo menu

(In reply to comment #11)
> I believe this went through IRC review, but I forget the outcome.

You requested some more comments.

*** Bug 664746 has been marked as a duplicate of this bug. ***