Closed Bug 919803 Opened 11 years ago Closed 11 years ago

ambient panels may load incorrectly

Categories

(Firefox Graveyard :: SocialAPI, defect)

26 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox26 verified, firefox27 verified)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox26 --- verified
firefox27 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached image panelbug.png
Here are the STR:

- start firefox with facebook enabled from previous sessoin
- before facebook sidebar is showing, click on one of the jewels
- leave it open and wait for sidebar to appear

what should happen is the content refreshes at some point when the worker begins initializing everything.  what does happen is the panel reloads causing the panel to be 3x the width (ie. width of all three jewel panels since the same page handles all 3).

I've tracked this down to a regression from bug 891216 and have a fix for it.

I also have a fix (coming) but am unsure about how to test this effectively, since it is really implementation specific issue in combination with an edge case bug in firefox.
Attached patch panelfix.patch (obsolete) — Splinter Review
This fixes the removal of panel iframes so that they are not removed if they are for the current provider (it used to work that way), thus avoiding a repeated remove/create-then-load of the iframe.
Assignee: nobody → mixedpuppy
Attachment #808895 - Flags: feedback?(mhammond)
Comment on attachment 808895 [details] [diff] [review]
panelfix.patch

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

This is complex and hard to read - it would be great if it can be simplified a little - but in general, something like this will be fine.

::: browser/base/content/browser-social.js
@@ +876,5 @@
>      toggleNotificationsCommand.setAttribute("hidden", !socialEnabled);
>  
> +    // we need to remove buttons and frames if !socialEnabled or the provider
> +    // has changed (frame origin does not match current provider)
> +    let currentOrigin = socialEnabled && Social.provider ? Social.provider.origin : null;

The check of both socialEnabled && Social.provider is redundant

@@ +891,3 @@
>          let frame = document.getElementById(nid);
> +        // if the provider changed, forget any attached frames
> +        if (!socialEnabled || frame && frame.getAttribute("origin") != currentOrigin) {

please use parens here - I can never remember what has higher precedence between && and ||

@@ +894,5 @@
> +          if (frame) {
> +            SharedFrame.forgetGroup(frame.id);
> +            parent.removeChild(frame);
> +          }
> +          tbi.parentNode.removeChild(child);

If I read the above correctly, if social is enabled and there is no frame, you don't remove the child.  Is that correct?  I'd have expected the child to be removed in that case?
Attachment #808895 - Flags: feedback?(mhammond) → feedback+
(In reply to Mark Hammond (:markh) from comment #2)
> Comment on attachment 808895 [details] [diff] [review]
> panelfix.patch
> 
> Review of attachment 808895 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +894,5 @@
> > +          if (frame) {
> > +            SharedFrame.forgetGroup(frame.id);
> > +            parent.removeChild(frame);
> > +          }
> > +          tbi.parentNode.removeChild(child);
> 
> If I read the above correctly, if social is enabled and there is no frame,
> you don't remove the child.  Is that correct?  I'd have expected the child
> to be removed in that case?

It's being removed (it is outside "if(frame)").  The check is only whether we need to remove the frame also.
Attached patch panelfix.patch (obsolete) — Splinter Review
Simplified as much as I could, other feedback addressed.
Attachment #808895 - Attachment is obsolete: true
Attachment #810073 - Flags: review?(mhammond)
Comment on attachment 810073 [details] [diff] [review]
panelfix.patch

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

this looks much simpler, thanks!

::: browser/base/content/browser-social.js
@@ +882,5 @@
>      if (tbi) {
>        // buttons after social-provider-button are ambient icons
> +      let next = tbi.nextSibling;
> +      let currentOrigin = Social.provider ? Social.provider.origin : null;
> +  

trailing space
Attachment #810073 - Flags: review?(mhammond) → review+
Attached patch panelfix.patchSplinter Review
https://hg.mozilla.org/integration/fx-team/rev/ef25041ec334

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 891216
User impact if declined: notification panels will size and position incorrectly
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #810073 - Attachment is obsolete: true
Attachment #810683 - Flags: review+
Attachment #810683 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ef25041ec334
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Attachment #810683 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Shane, I am not able to reproduce the initial issue because Social API panel is displayed inside the FF window not outside as is displayed in your attachement. Were you using a demo build or a particular build because it doesn't look like a central one?
Flags: needinfo?(mixedpuppy)
(In reply to Mihai Morar, QA (:MihaiMorar) from comment #10)
> Shane, I am not able to reproduce the initial issue because Social API panel
> is displayed inside the FF window not outside as is displayed in your
> attachement. Were you using a demo build or a particular build because it
> doesn't look like a central one?

Any updates on the above?
(In reply to Mihai Morar, QA (:MihaiMorar) from comment #10)
> Shane, I am not able to reproduce the initial issue because Social API panel
> is displayed inside the FF window not outside as is displayed in your
> attachement. Were you using a demo build or a particular build because it
> doesn't look like a central one?

The bug is that the panel would jump to the location illustrated in the attached image if the STR are followed.  Since facebook is broken right now, you cannot follow the STR.
Flags: needinfo?(mixedpuppy)
Was able to reproduce the initial issue on Nightly from 2013-09-23, verified as fixed on Firefox 27 beta 5 and Firefox 26RC using Mac OS X 10.9.1.
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: