804442 - Tools > Social menu not showing up on nightly since bug 772808 was fixed.

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

The menu is not showing up on nightly since bug 772808 was fixed.  This is a different problem than bug 804242 where it sometimes isn't showing up on Aurora/Beta.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: update command instead of individual items

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 674088 [details] [diff] [review]
update command instead of individual items

This patch doesn't work correctly - see bug 772808 comment 12.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Use setAttribute instead of .hidden

Setting .hidden doesn't work correctly, and neither does removing the hidden attribute (bug 772808 only updates the hidden attribute if the command has the attribute).  Fortunately XUL elements are hidden explicitly by checking hidden="true" - so hidden="false" works.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5077791b4ea4

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #3)
> Created attachment 674154 [details] [diff] [review]
> Use setAttribute instead of .hidden
> 
> Setting .hidden doesn't work correctly, and neither does removing the hidden
> attribute (bug 772808 only updates the hidden attribute if the command has
> the attribute).

Setting .hidden and removing the attribute should be equivalent (the hidden setter removes the attribute under the hood). That it doesn't work is a bug in the patch for bug 772808, right? Is there a bug on file?

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> That it doesn't work is a bug in the patch for bug 772808, right?

Attribute removals were generally not observed prior to and regardless of bug 772808, afaik.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)

> Setting .hidden and removing the attribute should be equivalent (the hidden
> setter removes the attribute under the hood).

Right - IIRC that part was working.

> That it doesn't work is a bug
> in the patch for bug 772808, right? Is there a bug on file?

772808 explicitly only copies the value of the "hidden" attribute from the command to the menu item if the command has a "hidden" attribute.  The fact that setting .hidden=false removes the attribute means that whatever value the menuitem had for hidden stays the same - so it isn't also removed from the menuitem (ie, the menuitem remains hidden even if the command had the attribute removed).

The code touched by bug 772808 uses this same behaviour for the 'label', 'accesskey', 'checked' and now 'hidden' attributes.  However, the attribute 'disabled' is treated differently - if no such attribute exists on the command it is also removed from the menuitem.  At a guess, I'd say the bug here is that 772808 should have treated 'hidden' in the same way it treats 'disabled', but I'm not familiar enough with things to know for sure - and given that XUL's css explicitly uses [hidden="true"] rather than simply [hidden], it isn't clear to me that this really is the/a problem.  So there is no bug on file I'm aware of.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5077791b4ea4

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #7)
> At a guess, I'd say the bug here is that 772808 should have treated 'hidden' in the
> same way it treats 'disabled', but I'm not familiar enough with things to know for
> sure

Yes, I think that's exactly right.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Attribute removals were generally not observed prior to and regardless of
> bug 772808, afaik.

The XUL properties for these attributes remove the attribute, so not observing attribute removal is a pretty obvious bug. What other cases are there that aren't currently covered by bug 805653?

Comment 11

[Dão Gottwald [:dao]]

I can't remember where. I'm sure this isn't the first time we're setting attributes to false rather than removing them, but it's possible that I confused attribute observing with attribute persisting.