965209 - SDK add-ons placed in panel menu cause styling glitches in sub panels (History, Bookmarks...) on OS X

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Defects

[Hit enter too soon here]
On Mac, all sub panels are shifted down a few pixels.
The hover effect of the menu items also looks seriously glitchy right now. (see attachment)

Comment 2

[:Gijs (he/him)]

You P-'d this, do you think the current state is shippable? That screenshot doesn't look it to me...

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

The current state sure isn't shipable.
I did the P- because this bug blocks bug 960258 which is a P2.
Feel free to change, because I still don't have a good grasp of how prioritizations and meta bugs play together.

Comment 4

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

(In reply to Philipp Sackl [:phlsa] from comment #3)
> The current state sure isn't shipable.
> I did the P- because this bug blocks bug 960258 which is a P2.
> Feel free to change, because I still don't have a good grasp of how
> prioritizations and meta bugs play together.

Heh, yeah, I don't think we're being cut / dry / clear about it.

Here's how I'm thinking about it:

If a bug is a small part of a larger goal (like the individual probes I did for BrowserUITelemetry, or the ones I'm filing for making customize mode go smoother), I would mark those P-.

If a bug is a part of a larger goal (making the panel perfect), but is itself a behavioural or visual problem, that's when I'd _not_ put the P-, to ensure that the problem is visible on the tracker.

So that's where I'd draw the line - I'd P- if the bug is a small part of a larger, multi-step bug.

That's my approach, anyhow.

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Thanks for the clarification, Mike!
P2 it is.

Comment 6

[:Gijs (he/him)]

This is confusing. I can't reproduce this inside the main panel, but I can reproduce it when using the subview as its own panel. Philipp, can you still reproduce this on today's nightly when using these views as a subview inside the main menu panel?

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yes, with Nightly 2014-01-30, it still has the same problems. (http://cl.ly/image/442r1Q202W0i)
What do you mean by using the subview as its own panel? Like when you have the history button in the toolbar?

Comment 8

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #7)
> Yes, with Nightly 2014-01-30, it still has the same problems.
> (http://cl.ly/image/442r1Q202W0i)
> What do you mean by using the subview as its own panel? Like when you have
> the history button in the toolbar?

Yes. I wonder why I can't reproduce the subview header issue. Can you reproduce on a clean profile?

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Now that's interesting: With a clean profile, it happens neither in the main panel, nor when the button is placed in the toolbar.

Comment 10

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #9)
> Now that's interesting: With a clean profile, it happens neither in the main
> panel, nor when the button is placed in the toolbar.

Check whether hardware acceleration is turned off/on in the broken profile? And/or check whether opening the other profile in safe mode reproduces the issue.

Comment 11

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

So, the issue for me was the SDK add-on snooze tabs (http://people.mozilla.org/~bwinton/). The problem only appears on Mac and only when the button of the add-on is placed in the panel. In this case, I can reproduce it even with an otherwise fresh profile.
It appears to be a general add-on SDK problem though, because I was able to reproduce it with Free Memory (https://addons.mozilla.org/en-US/firefox/addon/freememory) as well. Again, the issue only surfaces when the button of that add-on is in the panel.

Comment 12

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #11)
> So, the issue for me was the SDK add-on snooze tabs
> (http://people.mozilla.org/~bwinton/). The problem only appears on Mac and
> only when the button of the add-on is placed in the panel.

I can reproduce this on Mac. Did you test Windows? Do neither of the two problems happen there?

Comment 13

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #12)
> I can reproduce this on Mac. Did you test Windows? Do neither of the two
> problems happen there?

I just checked again. The issue occurs on Mac (10.9.1) and Windows XP.
It does not occur on Ubuntu and Windows 8.1

Comment 15

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > I can reproduce this on Mac. Did you test Windows? Do neither of the two
> > problems happen there?
> 
> I just checked again. The issue occurs on Mac (10.9.1) and Windows XP.
> It does not occur on Ubuntu and Windows 8.1

bug 970282 was reported on Win7, so I'm just going to move this to all/all.

Comment 16

[:Gijs (he/him)]

Attachment: workaround Australis panel issues with sdk iframe widgets,

I've poked at this for a while, but I can't figure out what the issue is. The coordinates reported by DOMI are correct (that is, the vertical coordinates of the subview are the same as those of the mainview and don't change); it's the visual bits that are moving about. I don't know why. I do know that this fixes it, and I think we should take this fix to avoid the havoc that these things are currently creating. Obviously this has the side effect that the icon will disappear while a subview is open, which may cause a layout shift of the text, if it's in the right-most row of the panel. However, considering that widgets are already deprecated, I think this is acceptable.

Comment 17

[:Gijs (he/him)]

Attachment: workaround Australis panel issues with sdk iframe widgets,

Actually, we should only do this for small items. For wide items, I suspect the layout change for not displaying the iframe will be too big.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8376337 [details] [diff] [review]
workaround Australis panel issues with sdk iframe widgets,

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

r=me but please try switch to `visibility: hidden` as it seems to work for me.
The benefit of this workaround seems to outweigh the cost of one jetpack extension ruining the layout of subviews.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +261,5 @@
>    width: calc(@menuPanelButtonWidth@);
>    margin: 0 !important;
>  }
>  
> +toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"]:not(.panel-wide-item) {

Noting here that this was fixing a separate mistake for the class name to help people looking through blame.

@@ +270,5 @@
>  toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"] > iframe {
>    margin: 4px auto;
>  }
>  
> +#PanelUI-multiView[viewtype="subview"] toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"]:not(.panel-wide-item) > iframe {

Please add a comment to indicate what this is working around. Please also file a new bug for a proper fix (as I'm sure someone will file a bug about the icon disappearing in no time) and include that bug number in the comment. It seems like there may be a layout bug which would ideally be fixed.

@@ +271,5 @@
>    margin: 4px auto;
>  }
>  
> +#PanelUI-multiView[viewtype="subview"] toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"]:not(.panel-wide-item) > iframe {
> +  display: none;

I can't say I like this workaround but it does seem to work. It seems (at least for me) that `visibility: hidden` works as well and doesn't shift content so please switch to that if you see the same.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8376337 [details] [diff] [review]
workaround Australis panel issues with sdk iframe widgets,

I've gone ahead and landed this with the comment and using visibility:hidden. I filed bug 975375 for the Core followup.

remote:   https://hg.mozilla.org/integration/fx-team/rev/bfecd06b79f2

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bfecd06b79f2

Comment 21

[Dave Townsend [:mossop]]

Does this need to be uplifted to aurora?

Comment 22

[:Gijs (he/him)]

Comment on attachment 8376337 [details] [diff] [review]
workaround Australis panel issues with sdk iframe widgets,

Yes.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: subviews look wonky if you have SDK add-ons in the panel
Testing completed (on m-c, etc.): on m-c for a long time
Risk to taking this patch (and alternatives if risky): low. Makes add-on SDK icons disappear when opening a subview, but that was deemed nicer than the actual layout issue which it fixes.
String or IDL/UUID changes made by this patch: none

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0a38af7a993c