Closed Bug 807997 Opened 12 years ago Closed 12 years ago

In content pages: No action performed when attempting to open a contact in Facebook sidebar

Categories

(Firefox Graveyard :: SocialAPI, defect)

17 Branch
defect
Not set
normal

Tracking

(firefox18+ verified, firefox19+ verified, firefox20 verified)

VERIFIED FIXED
Firefox 20
Tracking Status
firefox18 + verified
firefox19 + verified
firefox20 --- verified

People

(Reporter: virgil.dicu, Assigned: markh)

References

Details

(Keywords: reproducible, Whiteboard: [testday-20130301])

Attachments

(2 files, 4 obsolete files)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0

STR:

1. Activate Facebook messenger for Firefox: https://www.facebook.com/about/messenger-for-firefox
2. Load any in content page: about:preferences, about:addons
3. Click a friend name in the sidebar to open a window conversation

Actual result: no action is performed and Social API buttons are absent

Expected: either disable the Sidebar in In Content pages or implement full functionality.
This is weird. Are we trying to suppress chat windows in windows where the chrome is hidden?
This is quite easily reproducible. Could this be a possible regression from disabling sharing of about: pages?
Keywords: reproducible
Nope, not related to that. That was specific to share, and affects all about: pages, whereas this only seems to affect about: pages that hide the browser toolbar (about:config works fine).
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> This is weird. Are we trying to suppress chat windows in windows where the
> chrome is hidden?

yeah, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#242
Ah, I guess the issue here then is that the sidebar isn't being hidden (because we don't call updateSidebar when the "chromeless" state changes due to loading an in-content page).

But it raises the question of whether we really want to be hiding the sidebar/chats in this situation... I guess I could go either way.
I think we should just stop hiding the chrome for these pages. Note that hiding the toolbar gets confusing for Social API.
This bug certainly causes "unexpected" behaviour IMO - Gavin was surprised and I can't see a justification for having the sidebar visible with contacts displayed, but clicking on a contact silently takes no action.  Further, existing chat windows remain displayed when switching/opening such a tab - so things are quite inconsistent.

So I think we should remove the "chromehidden" checks from the chat code so things are consistent, then tackle the rest of the "what should we show, and when" questions in bug 798198.

This patch does just that - it also makes isLoggedInUser() more consistent with Social.uiVisible, so code which checks the former need not check the latter.
Assignee: nobody → mhammond
Attachment #687619 - Flags: review?(jaws)
Comment on attachment 687619 [details] [diff] [review]
Make opening of chat windows consistent with the rest of the social ui

We have two checks for "chromeless": the presence of the disablechrome attribute, and a chromehidden attribute containing "extrachrome". The former is what controls the hiding of the toolbars when navigating to pages like about:addons, the latter is what makes popup windows have less chrome visible.

I think it probably makes sense to lose the former, but keep the latter?
Comment on attachment 687619 [details] [diff] [review]
Make opening of chat windows consistent with the rest of the social ui

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

So what stops the chats from getting opened into popup windows (chromehidden=extrachrome)?

::: browser/base/content/browser-social.js
@@ +197,5 @@
>      this.notificationPanel.hidePopup();
>    },
>  
>    haveLoggedInUser: function SocialUI_haveLoggedInUser() {
> +    return !!(Social.provider && Social.provider.enabled && Social.provider.profile && Social.provider.profile.userName);

nit, this line is getting pretty long now. Can you reformat it to:
> return !!(Social.provider && Social.provider.enabled &&
>           Social.provider.profile && Social.provider.profile.userName);
Attachment #687619 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] from comment #9)

> So what stops the chats from getting opened into popup windows
> (chromehidden=extrachrome)?

Something should, but I'm not convinced it is this :)  Chats initiated by the sidebar etc shouldn't have the problem as the mozSocial API isn't (or shouldn't be) injected into such windows.

Chats opened by the worker uses Services.wm.getMostRecentWindow("navigator:browser") - so if we just blocked the chat open in browser-social, we still have the situation that the chat isn't opened at all, rather than being opened in the correct window.  So the correct solution there would be to *enumerate* all such windows and find a candidate window.

So while keeping that check in browser-social might be prudent, if the check ever fails it means we've done something else wrong.

Thoughts?
Best I can tell, mozSocial is injected into "extrachrome" windows.  Looking to the future when me might allow regular content pages from a provider to have access to this (or a similar) object, it might not make sense to disable this now.  So I propose the following change:

this.openChatWindow =
 function openChatWindow(chromeWindow, provider, url, callback, mode) {

This function treats |chromeWindow| as a hint, and use it if it has 'extrachrome'.
Otherwise, we enumerate all |navigator:browser| windows looking for ones with 'extrachrome' *and* a chat already open.  If that fails, we just use "any" (ideally, most recent) one.  Finally, once we've found the window to use, we check if it is focused and if not, we 'flash' it.

If we fail to locate a target window, it almost certainly means only a !extrachrome window is open - in which case we can log an error to the console.

End result is:
* When opened from a sidebar, the window with the sidebar is always used.  It will not flash unless somehow it managed to be not focused when this happens.

* When opened from a worker, a window with existing chats will be used (to save "randomly" ending up with chats in multiple top-level windows.  If that window isn't focused it flashes so the user notices.

* The only time the chat fails to open is when the only window open is a popup.

On one hand this sounds a little like overkill - but OTOH it sounds like it would match user expectations.
This is a first cut at what I described re finding the most appropriate window to host the chats.  It doesn't have the browser-social.js change from the previous patch and needs tests, but I thought I'd get early feedback to see what you guys think if the idea itself.
Attachment #688088 - Flags: feedback?(jaws)
Attachment #688088 - Flags: feedback?(gavin.sharp)
Comment on attachment 688088 [details] [diff] [review]
first cut at finding an appropriate window to host chats

This looks great.

You could use: !docElem.getAttribute("chromehidden").contains("extrachrome")

The getTopWin() call shouldn't be necessary - getZOrderDOMWindowEnumerator only returns top-level windows. I'd consider avoiding the double loop by storing the first suitable candidate window in a variable and only overridding it if we find a "better" (hasChats) window, but I guess that's ugly in a different way so up to taste, perhaps :)

Adding a "hasChats()" method to the chatbar binding rather than checking chatbar.firstElementChild would be nice, I think.

IIRC getZOrderDOMWindowEnumerator is buggy on some platforms, that's why we have BROKEN_WM_Z_ORDER - not sure whether that's actually still the case (that workaround is very old), but maybe worth investigating.
Attachment #688088 - Flags: feedback?(gavin.sharp) → feedback+
This patch should be close to ready to go.  Asking Gavin for feedback as he mentioned the BROKEN_WM_Z_ORDER issue, so feedback on the new code in MozSocialAPI.js would be great.  Gavin: if you are keen and this looks OK a full review would be great - otherwise, if I get f+, I'll ask Jaws for the full review.

Only tested on Windows but a try run is at https://tbpl.mozilla.org/?tree=Try&rev=df909acfd444
Attachment #687619 - Attachment is obsolete: true
Attachment #688088 - Attachment is obsolete: true
Attachment #688088 - Flags: feedback?(jaws)
Attachment #688546 - Flags: feedback?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> You could use: !docElem.getAttribute("chromehidden").contains("extrachrome")

Bug 793781 disabled String.contains in Firefox 17 due to a compatibility issue with MooTools, which is likely to cause a similar backout in Firefox 18 and possibly even Firefox 19. Since this patch is likely to get uplifted, it would be better to just use indexOf(str) != -1 in the meantime.
Well, to emulate !contains, I should have said indexOf(str) == -1 :)
Attached patch avoid .contains() as per comment (obsolete) — Splinter Review
Attachment #688546 - Attachment is obsolete: true
Attachment #688546 - Flags: feedback?(gavin.sharp)
Attachment #688961 - Flags: review-
Attachment #688961 - Flags: feedback?(gavin.sharp)
Comment on attachment 688961 [details] [diff] [review]
avoid .contains() as per comment

oops - clearing the r- flag I accidentally set.
Attachment #688961 - Flags: review-
Comment on attachment 688961 [details] [diff] [review]
avoid .contains() as per comment

I think it might make more sense to keep the "extrachrome" check in isAvailable, but call isAvailable from isWindowGoodForChats? That way if someone tries to add chats without checking the equivalent of isWindowGoodForChats (unlikely scenario, to be sure), they won't get added.

Should we call focus() rather than just getAttention()? I guess not - this is a "background" action and stealing focus would probably be annoying, right?

Can the test use childElementCount rather than querySelectorAll("*").length?

The updateButtonHiddenState and SocialChatBar.update() changes are unrelated to this bug, right? Generally not a good idea to be bundling unrelated changes in the same patch, but I suppose I'll let it fly :)

(It seems like we're switching from a bunch of UI being controlled by Social.uiVisible to it being controlled by SocialUI.haveLoggedInUser(). Makes me think that haveLoggedInUser should live on Social (global state) rather than SocialUI (state specific to a window). I'm also not sure I like moving in that direction in general - seems like we'd maybe want to make the "logged in" state optional as we experiment with providers who don't necessarily need/want that concept. But that we can revisit later.)

r=me with those addressed/considered.
Attachment #688961 - Flags: feedback?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> The updateButtonHiddenState and SocialChatBar.update() changes are unrelated
> to this bug, right? Generally not a good idea to be bundling unrelated
> changes in the same patch, but I suppose I'll let it fly :)

Actually, thinking about it more, I'd really prefer for the fix for bug 807997 to be split into its own patch in that bug.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #19)
> > The updateButtonHiddenState and SocialChatBar.update() changes are unrelated
> > to this bug, right? Generally not a good idea to be bundling unrelated
> > changes in the same patch, but I suppose I'll let it fly :)
> 
> Actually, thinking about it more, I'd really prefer for the fix for bug
> 807997 to be split into its own patch in that bug.

The updatebuttonHiddenState one is wrong for this patch, yeah.  The SocialChatBar.update() change is necessary for the tests to pass - they create a second window to check the "window chooser" semantics.  I guess there's no reason not to make this one depend on that though, so I'll do that.
Depends on: 817782
Ah, I see! Makes sense, carry on then!
Attached patch UpdatedSplinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> 
> I think it might make more sense to keep the "extrachrome" check in
> isAvailable

Done.

> Should we call focus() rather than just getAttention()? I guess not - this
> is a "background" action and stealing focus would probably be annoying,
> right?

Yeah, I think focus() is the wrong thing to do here - I've left it as getAttention()

> Can the test use childElementCount rather than querySelectorAll("*").length?

It can, and now does!

> The updateButtonHiddenState and SocialChatBar.update() changes are unrelated
> to this bug, right? Generally not a good idea to be bundling unrelated
> changes in the same patch, but I suppose I'll let it fly :)

All gone.

> r=me with those addressed/considered.

Not sure if this means you want to see the new one or not, so I'll play it safe :)
Attachment #688961 - Attachment is obsolete: true
Attachment #689408 - Flags: review?(gavin.sharp)
Attachment #689408 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/168603da177a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
This patch has more than a minimal amount of risk, I'm not sure whether we should aim to uplift on beta at this point.

Mark, what do you think?
Triage drive-by: will track for Aurora but wait on tracking decision for beta considering the question in comment 27.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #27)
> This patch has more than a minimal amount of risk, I'm not sure whether we
> should aim to uplift on beta at this point.
> 
> Mark, what do you think?

Agreed.  Although this patch did morph from the description to one better described as "find a good chat window to use".  As this is user-facing, maybe a simpler patch that just solves the described problem on beta would be worthwhile (which would just be to remove that disablechrome check)?
(In reply to Mark Hammond (:markh) from comment #29)
> As this is user-facing, maybe a simpler patch that just solves the described
> problem on beta would be worthwhile (which would just be to remove that
> disablechrome check)?

Yeah, that's a good idea!
This is a simpler patch against for beta - it only fixes the described problem without the enhancements made by the patch which landed on m-c.
Attachment #690005 - Flags: review?(gavin.sharp)
Attachment #690005 - Flags: review?(gavin.sharp) → review+
Comment on attachment 690005 [details] [diff] [review]
Minimal patch for beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Chat windows can not be created when on most about:* pages, even though existing chats remain visible.
Testing completed (on m-c, etc.): A much smaller version of this patch landed on m-c.  Tested locally, but will require QA verification.
Risk to taking this patch (and alternatives if risky): Small, limited to social chat code only.
String or UUID changes made by this patch: None
Attachment #690005 - Flags: approval-mozilla-beta?
Comment on attachment 690005 [details] [diff] [review]
Minimal patch for beta.

Thanks for the lower-risk patch that affect social only - we'll take this on beta - please remember to nominate the other patch for aurora so we can close the loop on this.
Attachment #690005 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 689408 [details] [diff] [review]
Updated

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Chat windows might silently fail to open.
Testing completed (on m-c, etc.): Landed on m-c, includes tests.
Risk to taking this patch (and alternatives if risky): Low risk limited to social.
String or UUID changes made by this patch: None
Attachment #689408 - Flags: approval-mozilla-aurora?
Comment on attachment 689408 [details] [diff] [review]
Updated

I was debating whether we should take this or the simpler patch for Aurora. This fix is really isolated to chat code, so I think it's probably fine for Aurora.
Attachment #689408 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 820743
Summary: In content pages: No action performed when attempting to open a contact in Facebook sidebar → In content pages: No action performed when attempting to open a contact in Facebook sidebar
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0

Verified on Ubuntu 12.04, Windows 7, Mac OS 10.7 with beta 4. Opening/closing, chatting across tabs and windows with In-content pages works as expected.
Keywords: verifyme
QA Contact: virgil.dicu
Verified the fix on Firefox 19.0 beta 2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Macintosh: Intel Mac OS X 10.7: rv:19.0) Gecko/20100101 Firefox/19.0)
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130116072953

Opening/closing, chatting across tabs and windows with In-content pages works as expected.
I followed the reporter's STR:

1. I activated Facebook messenger for Firefox: https://www.facebook.com/about/messenger-for-firefox
2. I loaded about:preferences.
3. I clicked a friend name in the sidebar to open a window conversation

Actual result: full functionality (I was able to open a window conversation and chat).





Verified fixed on Firefox 20.0 beta 2 and Windows 7 64 bit.
Whiteboard: [testday-20130301]
Thanks for your help Gabriela.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: