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)
Tracking
(firefox18+ verified, firefox19+ verified, firefox20 verified)
VERIFIED
FIXED
Firefox 20
People
(Reporter: virgil.dicu, Assigned: markh)
References
Details
(Keywords: reproducible, Whiteboard: [testday-20130301])
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
Gavin
:
review+
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
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
Comment 3•12 years ago
|
||
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).
Comment 4•12 years ago
|
||
(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
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
I think we should just stop hiding the chrome for these pages. Note that hiding the toolbar gets confusing for Social API.
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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 9•12 years ago
|
||
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-
Assignee | ||
Comment 10•12 years ago
|
||
(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?
Assignee | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
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 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
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)
Comment 15•12 years ago
|
||
(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.
Comment 16•12 years ago
|
||
Well, to emulate !contains, I should have said indexOf(str) == -1 :)
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #688546 -
Attachment is obsolete: true
Attachment #688546 -
Flags: feedback?(gavin.sharp)
Attachment #688961 -
Flags: review-
Attachment #688961 -
Flags: feedback?(gavin.sharp)
Assignee | ||
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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+
Comment 20•12 years ago
|
||
(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.
Comment 21•12 years ago
|
||
Er, I meant bug 817782.
Assignee | ||
Comment 22•12 years ago
|
||
(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
Comment 23•12 years ago
|
||
Ah, I see! Makes sense, carry on then!
Assignee | ||
Comment 24•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #689408 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 25•12 years ago
|
||
status-firefox18:
--- → affected
status-firefox19:
--- → affected
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
Flags: in-testsuite+
Comment 26•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Comment 27•12 years ago
|
||
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?
Comment 28•12 years ago
|
||
Triage drive-by: will track for Aurora but wait on tracking decision for beta considering the question in comment 27.
Assignee | ||
Comment 29•12 years ago
|
||
(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)?
Comment 30•12 years ago
|
||
(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!
Assignee | ||
Comment 31•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #690005 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 32•12 years ago
|
||
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?
Updated•12 years ago
|
Comment 33•12 years ago
|
||
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+
Assignee | ||
Comment 34•12 years ago
|
||
Assignee | ||
Comment 35•12 years ago
|
||
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 36•12 years ago
|
||
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+
Updated•12 years ago
|
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
Comment 37•12 years ago
|
||
status-firefox20:
--- → fixed
Reporter | ||
Comment 38•12 years ago
|
||
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.
Reporter | ||
Comment 39•12 years ago
|
||
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.
Comment 40•12 years ago
|
||
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]
Comment 41•12 years ago
|
||
Thanks for your help Gabriela.
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•