1007601 - Sort out framing of the Loop panel

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

The Loop panel currently has a white boarder around it, which shouldn't be there.

I've looked at it before, but I'm not totally sure where it is coming from.

From what I can tell, the size of the loop panel is correct, but the white boarder is probably from the panel element, but that hasn't got any obvious css that would cause it.

Comment 1

[Mark Banner (:standard8)]

After discussion with Adam, moving out to mvp.

Comment 2

[Mark Banner (:standard8)]

Something in nightly just changed and broke the panel layout really badly, in fixing the layout, I'm also able to fix this, so bringing it back to MLP and I'll do both the fixes here.

Comment 3

[Mark Banner (:standard8)]

Attachment: Fix styling of the loop panel so it is correctly sized and without a white border.

This fixes the styling of the loop panel. It re-uses the styles from the social panel to get the right padding and borders.

It also uses the hack of setting a rough size of the panel before layout so that we get approximately the right layout - it seemed to prefer to lay things out in a portrait style rather than landscape otherwise. This is a similar work around that the social panel uses.

Comment 4

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

Comment on attachment 8422435 [details] [diff] [review]
Fix styling of the loop panel so it is correctly sized and without a white border.

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

I should have done this in the first place, but what I'd really prefer is that the iframe itself is created in browser-loop.js first time it is created - that way we don't have the iframe in the DOM until the panel is actually used.  eg, see the _createFrame() methods in browser-social.  social also has a "shared frame" concept we might want to borrow too, so we avoid the total number of such iframes.

Would it be possible for you to have a look at creating the iframe on demand?  ie, remove the iframe entirely from browser.xul, and the new attributes you added in this patch are just applied as the frame is created during first show of the panel?

Comment 5

[Mark Banner (:standard8)]

That makes sense, I'll look at that this morning.

Comment 6

[Mark Banner (:standard8)]

Ok, I've taken a bit of a dive into the issues around this. I've been looking at the issues around this, and possible solutions.

Something I've also found is that the current set-up doesn't work properly if the loop button is in the hamburger menu as well.

I think the best solution for this would be to see if we can extract the panel creation parts of social into a separate module, with some sort of wrapper - as otherwise we're going to be massively duplicating the code for this.

I'll see if I can mock this up to some extent later today and see if it looks reasonable.

Comment 7

[Mark Banner (:standard8)]

Attachment: Concept Patch for shared panel functionality

This is a non-functional concept patch that I've whipped up for demonstrating how we might make the panel functionality available to both Social and Loop.

The basic idea is a new include for browser.js that splits out the functionality. This will mean we get to reuse some of the functionality for moving frames across windows, the hamburger menu etc.

As mentioned above, this is non-functional, and I'm not quite sure why at the moment. The code appears fine, but the social panel doesn't show.

I also think there's quite a bit more work to do to get it fully tidied up and functional.

Mark: If you agree that we want to go forward with this route, then I think I am at a place where this work would be best delayed until we've got the more critical items of loop landed.

Hence, I'd like to have the other styling patch landed/ready to land, so that we can move forward, and after we've done the minimal landing, we can pick up sharing the functionality properly (and make loop work in the hamburger menu).

This ties in with the minimal landing definition that we've had of getting the minimum thing landed, even if it has rough edges. Due to the hamburger menu issue, we'd have to fix the remaining parts before we released the first version of loop.

Comment 8

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

Comment on attachment 8423400 [details] [diff] [review]
Concept Patch for shared panel functionality

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

Yeah, an approach like this sounds good longer term, and I agree we don't need to get too bogged down at the moment.  (Also, please see if you can convince your editor to only strip whitespace from lines you touch - there is a file touched here that has nothing other than unrelated whitespace, which is just distracting for reviewers and breaks hg/git blame)

Comment 9

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

Comment on attachment 8422435 [details] [diff] [review]
Fix styling of the loop panel so it is correctly sized and without a white border.

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

I guess I don't see why this is all-or-nothing - can't we still still create the iframe in browser-loop.js and end up with something just as functional as this patch?

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8423400 [details] [diff] [review]
Concept Patch for shared panel functionality

I moved this out to bug 1011392.

Comment 11

[Mark Banner (:standard8)]

Attachment: Fix styling of the loop panel so it is laid out properly and without a white border. Also make the iframe creation dynamic, to avoid an iframe in the DOM when we don't need one.

Good point about creating the iframe dynamicially. This makes that change. There's something still not quite with the overall size, I've punted that to bug 1011394, I suspect it'll either be fixed by bug 1011392, or we'll need to dig a bit deeper.

Comment 12

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

Comment on attachment 8423697 [details] [diff] [review]
Fix styling of the loop panel so it is laid out properly and without a white border. Also make the iframe creation dynamic, to avoid an iframe in the DOM when we don't need one.

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

Looks good, thanks.  Please also add an XXX/TODO/whatever comment that the iframe should ideally be managed by SharedFrame.jsm - I'm not too bothered if you open a bug or not for it, but it's probably something we should end up doing.

Comment 13

[Mark Banner (:standard8)]

Attachment: Fix styling of the loop panel so it is laid out properly and without a white border. Also make the iframe creation dynamic, to avoid an iframe in the DOM when we don't need one.

Added the XXX comment. Thanks Mark.

Comment 14

[Mark Banner (:standard8)]

Attachment: Fix styling of the loop panel so it is laid out properly and without a white border. Also make the iframe creation dynamic, to avoid an iframe in the DOM when we don't need one.

Correct version.

Comment 15

[Mark Banner (:standard8)]

Landed on our git integration repo:

https://github.com/adamroach/gecko-dev/commit/d9aad23e05c68a2248e7792225cbb983f91daf62

Closing for tracking purposes, will comment with mercurial changeset when this lands properly.

Comment 16

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/0249d623c867

Comment 17

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/0249d623c867

Comment 18

[u279076]

Verified fixed through previous smoketesting.