879980 - Subscribe button appears disabled when in the palette

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) Enter customization mode and put the subscribe button into the palette if it isn't already there.

ER:

The subscribe button icon should appear enabled, not disabled.

AR:

http://i.imgur.com/2416TAb.png

Comment 1

[:Gijs (he/him)]

I'm guessing we don't actually want it to be enabled, so we should probably just override the CSS here?

Comment 2

[:Gijs (he/him)]

Attachment: Patch v1

This was harder than I thought to get right, in particular because checking the [customizing] attribute on the window doesn't get the right results for whether or not we're in customization mode. I suspect this is because we swap the selectedTab/selectedBrowser before we remove the attribute and that might be why the updateFeeds function is called 'early'?

I also wasted 2 hours wondering why using the .disabled property (rather than the attribute) in the onCreated function made everything break. Answer: the node isn't in the document at that point, so it doesn't have a binding, and so setting the property means the XBL property doesn't override the expando you just set, and you're in for all kinds of pain.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 759758 [details] [diff] [review]
Patch v1

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

I think I'm fine with this. I'm not too jazzed by checking for about:customizing in the contentURL, but let's not worry about that here. Would you mind filing a bug about making it easier for widgets to determine if we're in customization mode? Maybe something simple like... gCustomizeMode.enabled... I'm not sure. Not critical.

::: browser/base/content/browser-feeds.js
@@ +117,5 @@
>      var feeds = gBrowser.selectedBrowser.feeds;
>      var haveFeeds = feeds && feeds.length > 0;
>  
>      var feedButton = document.getElementById("feed-button");
> +    // Don't disable the button for customization mode:

I think we need a comment in here about why we're not looking for the customizing attribute.

@@ +118,5 @@
>      var haveFeeds = feeds && feeds.length > 0;
>  
>      var feedButton = document.getElementById("feed-button");
> +    // Don't disable the button for customization mode:
> +    var contentLoc = window.content && window.content.location.href;

We shouldn't introduce new var variables - please use "let" instead.

Comment 4

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/282a519538eb

Comment 5

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #3)
> Comment on attachment 759758 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 759758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I'm fine with this. I'm not too jazzed by checking for
> about:customizing in the contentURL, but let's not worry about that here.
> Would you mind filing a bug about making it easier for widgets to determine
> if we're in customization mode? Maybe something simple like...
> gCustomizeMode.enabled... I'm not sure. Not critical.

I'm not sure we want to. Generally, checking the window's attribute works, but the selectedBrowser changes before then, so updateFeeds fires, and if this code checks that, it leaves the element enabled/disabled when it should really not. IOW, only related to whether the selectedBrowser is the customization browser or not.

Comment 6

[Dão Gottwald [:dao]]

Looks like this patch messed with the button when it's in the toolbar. Why so? We don't enable other otherwise-disabled buttons in the toolbar while customizing.

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #6)
> Looks like this patch messed with the button when it's in the toolbar. Why
> so? We don't enable other otherwise-disabled buttons in the toolbar while
> customizing.

Can you give an example of a button that remains disabled when it's in the toolbar? AIUI, all buttons are actually enabled when you enter customization mode:

http://hg.mozilla.org/projects/ux/file/d34b32544b06/browser/components/customizableui/src/CustomizeMode.jsm#l362

so as to have a consistent look.

The browser-feeds code was then changing this again because it fires on the selectedBrowser change. Hence having to change code there, too.

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Looks like this patch messed with the button when it's in the toolbar. Why
> > so? We don't enable other otherwise-disabled buttons in the toolbar while
> > customizing.
> 
> Can you give an example of a button that remains disabled when it's in the
> toolbar? AIUI, all buttons are actually enabled when you enter customization
> mode:
> 
> http://hg.mozilla.org/projects/ux/file/d34b32544b06/browser/components/
> customizableui/src/CustomizeMode.jsm#l362

It doesn't handle the back/forward buttons or any other nested items.

I'd also question whether saving the disabled state and restoring it is a wise strategy. Code handling the disabled state of random widgets can run while customizing, in which case restoring the earlier state upon exiting customization mode will do the wrong thing.

> The browser-feeds code was then changing this again because it fires on the
> selectedBrowser change.

This would be another reason to stop messing with the disabled state, as there are likely more widgets changing state due to tab switching.

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > (In reply to Dão Gottwald [:dao] from comment #6)
> > > Looks like this patch messed with the button when it's in the toolbar. Why
> > > so? We don't enable other otherwise-disabled buttons in the toolbar while
> > > customizing.
> > 
> > Can you give an example of a button that remains disabled when it's in the
> > toolbar? AIUI, all buttons are actually enabled when you enter customization
> > mode:
> > 
> > http://hg.mozilla.org/projects/ux/file/d34b32544b06/browser/components/
> > customizableui/src/CustomizeMode.jsm#l362
> 
> It doesn't handle the back/forward buttons or any other nested items.
> 
> I'd also question whether saving the disabled state and restoring it is a
> wise strategy. Code handling the disabled state of random widgets can run
> while customizing, in which case restoring the earlier state upon exiting
> customization mode will do the wrong thing.
> 
> > The browser-feeds code was then changing this again because it fires on the
> > selectedBrowser change.
> 
> This would be another reason to stop messing with the disabled state, as
> there are likely more widgets changing state due to tab switching.

This sounds like you think we should stop doing this more generally. If you haven't already, can you file a new bug for this purpose and CC Unfocused, jaws, mconley and me, and make it block australis-cust? I don't think this bug is the right venue to have a discussion about the general principle and/or fix it. It would be good if you could outline how you would like to then ensure that buttons have a consistent look while in customization mode (presumably using CSS?) while maintaining their original disabled states. Thank you!

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #9)
> This sounds like you think we should stop doing this more generally.

I think so, but in particular I also think widgets shouldn't cater for about:customizing as done by your patch.

Comment 11

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > This sounds like you think we should stop doing this more generally.
> 
> I think so, but in particular I also think widgets shouldn't cater for
> about:customizing as done by your patch.

That also makes sense, but I'm not a fan of backing this out, especially right before user review, before we have an alternative solution. So let's figure out what we want to do instead, and then we can back this out in one go.

Comment 12

[Dão Gottwald [:dao]]

filed bug 882306

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/282a519538eb