827454 - Private browsing theme changes for Linux

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[(no longer active)]

This is a follow-up from bug 749394 to implement the Linux theme.

It turns out that we have to support Ubuntu's global menu bar too, which causes us to not have a menu bar or an app button.  Stephen, can you please attach a mock-up on how that should look like?

Comment 1

[(no longer active)]

We need to fix this for shipping Firefox 20 with per-window PB support.

Comment 2

[(no longer active)]

I first need mock-ups for this.  Also, Chris, can you please tell me how I can get a build of the global menu extension which I can use in Firefox 20 for testing?  Thanks!

Comment 3

[Justin Dolske [:Dolske]]

Hmm. This raises a few questions which I'm surprised haven't come up before. (At least AFAIK?)

My first reaction is that this is squarely a bug for Ubuntu's customization to deal with and fix. We support a baseline linux theme. If an addon or other customization disables baseline UI and replaces it with an alternative, it's their responsibility to keep up to date with the baseline UI and ensure their replacement is adequate. Mozilla doesn't control external code, and that this is Ubuntu's addon is fundamentally no different that if it was some random addon written by someone for their personal use.

[Sanity check: I assume from context that we're talking about one of the default / pre-installed addons all Ubuntu users get from their distro?]

It also makes me wonder if the Ubuntu changes for Unity integration should be uplifted into baseline Firefox, to ensure they're getting the same review and quality checks that other Firefox code gets (Gnome or otherwise). Are there perhaps existing bugs for doing so?


That said... I don't think there's any reason to make a big deal out of this right here or right now. It's the first time we've hit an issue like this. No need to change the whole world as a prerequisite to getting per-window private browsing (even though ehsan has mad skillz ;-). Nor is this a radical UI request; the case of "no Firefox button, no menu bar" is going to be the norm on Windows with Australis... So seems like a common solution will be needed anyway.

Comment 4

[Stephen Horlander [:shorlander]]

(In reply to Justin Dolske [:Dolske] from comment #3)
> It also makes me wonder if the Ubuntu changes for Unity integration should
> be uplifted into baseline Firefox, to ensure they're getting the same review
> and quality checks that other Firefox code gets (Gnome or otherwise). Are
> there perhaps existing bugs for doing so?

I think this has been discussed before, but I can't find the bug ATM (if it was in a bug…). I think it makes sense to have tighter integration with the more popular distros. Not only for review quality checks but also so we just integrate nicer out of the box.


> That said... I don't think there's any reason to make a big deal out of this
> right here or right now. It's the first time we've hit an issue like this.
> No need to change the whole world as a prerequisite to getting per-window
> private browsing (even though ehsan has mad skillz ;-). Nor is this a
> radical UI request; the case of "no Firefox button, no menu bar" is going to
> be the norm on Windows with Australis... So seems like a common solution
> will be needed anyway.

The desired long term solution is a more obvious visual change: http://people.mozilla.com/~shorlander/private-browsing-mode/mockups/australis-pbm.png (also covers the Australis case)

This would cover all configurations but is a more complex change :)


In this case if we don't have a menubar or a Firefox button to attach to then we could insert the indicator at the end of the tab-strip.

Comment 5

[(no longer active)]

(In reply to comment #3)
> Hmm. This raises a few questions which I'm surprised haven't come up before.
> (At least AFAIK?)
> 
> My first reaction is that this is squarely a bug for Ubuntu's customization to
> deal with and fix. We support a baseline linux theme. If an addon or other
> customization disables baseline UI and replaces it with an alternative, it's
> their responsibility to keep up to date with the baseline UI and ensure their
> replacement is adequate. Mozilla doesn't control external code, and that this
> is Ubuntu's addon is fundamentally no different that if it was some random
> addon written by someone for their personal use.

That was my reaction as well, but Dao told me that this is not the responsibility of that add-on.  I don't mind handling that case at all but it seems like there's no good way to actually test that add-on on trunk, which makes this exceptionally tricky.  :(

> [Sanity check: I assume from context that we're talking about one of the
> default / pre-installed addons all Ubuntu users get from their distro?]

I _think_ so.  I've never seen that add-on myself on Ubuntu (which is the distro that I use) but then again I always run mozilla.org binaries, so perhaps that's why...

> That said... I don't think there's any reason to make a big deal out of this
> right here or right now. It's the first time we've hit an issue like this. No
> need to change the whole world as a prerequisite to getting per-window private
> browsing (even though ehsan has mad skillz ;-). Nor is this a radical UI
> request; the case of "no Firefox button, no menu bar" is going to be the norm
> on Windows with Australis... So seems like a common solution will be needed
> anyway.

As Stephen mentioned, the Australist solution would not apply here.

Comment 6

[(no longer active)]

(In reply to comment #4)
> In this case if we don't have a menubar or a Firefox button to attach to then
> we could insert the indicator at the end of the tab-strip.

Can you please attach a mock-up of how that should look like exactly?

(I hope that we can avoid sticking a <xul:image> in the UI for this, as that has not been necessary on Windows, Mac, or Linux without the global menubar add-on, and I really hope that we can avoid having a different DOM for browser.xul when this add-on is present... :/ )

Comment 7

[Justin Dolske [:Dolske]]

(In reply to :Ehsan Akhgari from comment #5)

> That was my reaction as well, but Dao told me that this is not the
> responsibility of that add-on.

I'd say (as I kinda did above) that's it's formally indeterminate at the moment. But until we figure it out, it seems like the right call to not make users suffer from the confusion.

> it seems like there's no good way to actually test that add-on on trunk,
> which makes this exceptionally tricky.  :(
>
> > [Sanity check: I assume from context that we're talking about one of the
> > default / pre-installed addons all Ubuntu users get from their distro?]
> 
> I _think_ so.  I've never seen that add-on myself on Ubuntu (which is the
> distro that I use) but then again I always run mozilla.org binaries, so
> perhaps that's why...

My impression was that it's one of the system-installed addons Ubuntu ships with. (Someone needs to verify this. :) And so running Nightly on Ubuntu should just pick it up... Does it not? Are you running the latest Ubuntu release?

We should make sure this is actually a problem most Ubuntu users are going to run into. If it's actually an optional addon, then we don't need to handle it.

Comment 8

[(no longer active)]

(In reply to Justin Dolske [:Dolske] from comment #7)
> (In reply to :Ehsan Akhgari from comment #5)
> 
> > That was my reaction as well, but Dao told me that this is not the
> > responsibility of that add-on.
> 
> I'd say (as I kinda did above) that's it's formally indeterminate at the
> moment. But until we figure it out, it seems like the right call to not make
> users suffer from the confusion.

Hmm OK.  In that case I'll submit my previous patch here again to see what Dao thinks.

> > it seems like there's no good way to actually test that add-on on trunk,
> > which makes this exceptionally tricky.  :(
> >
> > > [Sanity check: I assume from context that we're talking about one of the
> > > default / pre-installed addons all Ubuntu users get from their distro?]
> > 
> > I _think_ so.  I've never seen that add-on myself on Ubuntu (which is the
> > distro that I use) but then again I always run mozilla.org binaries, so
> > perhaps that's why...
> 
> My impression was that it's one of the system-installed addons Ubuntu ships
> with. (Someone needs to verify this. :) And so running Nightly on Ubuntu
> should just pick it up... Does it not? Are you running the latest Ubuntu
> release?

I'm on a couple of releases behind, and I don't see that add-on in Nightly.  But I just tested on another Ubuntu installation, and the add-on is installed in the Canonical Firefox.  But that add-on is not compatible with trunk builds, which means that it is not useful for my testing.

> We should make sure this is actually a problem most Ubuntu users are going
> to run into. If it's actually an optional addon, then we don't need to
> handle it.

I think that users running Canonical Firefox will run into it, which should be all Ubuntu Firefox users.

Comment 9

[(no longer active)]

Attachment: The patch that doesn't handle Ubuntu specifically

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 699373 [details] [diff] [review]
The patch that doesn't handle Ubuntu specifically

There's little reason not to handle the case of the menu bar and button being hidden. We should do it. I really don't want Ubuntu's extension to mess with our theme and toolbars.

The bug for integrating global menubar support is bug 619899.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to :Ehsan Akhgari from comment #8)

> > I'd say (as I kinda did above) that's it's formally indeterminate at the
> > moment. But until we figure it out, it seems like the right call to not make
> > users suffer from the confusion.
> 
> Hmm OK.  In that case I'll submit my previous patch here again to see what
> Dao thinks.

I think you might have misread this -- I was trying to say that even _if_ it is strictly true that this is entirely Ubuntu's problem to deal with, as a practical matter this is likely to be a new and surprising declaration from both their point of view and those of users caught in the middle. Egro, I'm pretty sympathetic to making this our problem right now. Err, I mean, your problem. Sorry. ;)

I talked with Mike Conley about this a bit earlier in the week; I think we'll be wanting to work with the Canonical folks so that we can get to some shared understanding for the future... Somewhere between (inclusive) us fully supporting Unity in stock Firefox to them handling Unity issues but coordinating on things better. Group hug! :)

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to :Ehsan Akhgari from comment #8)
> 
> > > I'd say (as I kinda did above) that's it's formally indeterminate at the
> > > moment. But until we figure it out, it seems like the right call to not make
> > > users suffer from the confusion.
> > 
> > Hmm OK.  In that case I'll submit my previous patch here again to see what
> > Dao thinks.
> 
> I think you might have misread this -- I was trying to say that even _if_ it is
> strictly true that this is entirely Ubuntu's problem to deal with, as a
> practical matter this is likely to be a new and surprising declaration from
> both their point of view and those of users caught in the middle. Egro, I'm
> pretty sympathetic to making this our problem right now. Err, I mean, your
> problem. Sorry. ;)

I have no objection to fix this, if someone tells me how I can get their add-on to work on trunk.  To be bluntly clear, without that, there is nothing that I (or anybody else) can do here, and we'll just ship without any theme changes for Linux.  Which would of course suck.  But this will _not_ block us shipping.

> I talked with Mike Conley about this a bit earlier in the week; I think we'll
> be wanting to work with the Canonical folks so that we can get to some shared
> understanding for the future... Somewhere between (inclusive) us fully
> supporting Unity in stock Firefox to them handling Unity issues but
> coordinating on things better. Group hug! :)

While I appreciate that as is a future goal, can we please stay focused on this bug?  Here are the things that I need in order to be able to make progress here.

1. A mock-up showing how the interaction with the Ubuntu add-on should look like in private windows.
2. A version of the Ubuntu add-on which works on trunk.

Comment 13

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #12)
> (In reply to comment #11)
> > (In reply to :Ehsan Akhgari from comment #8)
> > 
> > > > I'd say (as I kinda did above) that's it's formally indeterminate at the
> > > > moment. But until we figure it out, it seems like the right call to not make
> > > > users suffer from the confusion.
> > > 
> > > Hmm OK.  In that case I'll submit my previous patch here again to see what
> > > Dao thinks.
> > 
> > I think you might have misread this -- I was trying to say that even _if_ it is
> > strictly true that this is entirely Ubuntu's problem to deal with, as a
> > practical matter this is likely to be a new and surprising declaration from
> > both their point of view and those of users caught in the middle. Egro, I'm
> > pretty sympathetic to making this our problem right now. Err, I mean, your
> > problem. Sorry. ;)
> 
> I have no objection to fix this, if someone tells me how I can get their
> add-on to work on trunk.  To be bluntly clear, without that, there is
> nothing that I (or anybody else) can do here, and we'll just ship without
> any theme changes for Linux.  Which would of course suck.  But this will
> _not_ block us shipping.

(And if I had to pick between shipping no theme change on any Linux distro and no theme changes on all Linux distros except for Ubuntu, I would personally pick the latter, which is what the existing patch does.)

Comment 14

[Dão Gottwald [:dao]]

You can hide the menu toolbar by setting hidden="true" on it. Or you can hide it through the UI such that the Firefox button is shown and just ignore the button for the purpose of this bug.

Comment 15

[(no longer active)]

(In reply to comment #14)
> You can hide the menu toolbar by setting hidden="true" on it. Or you can hide
> it through the UI such that the Firefox button is shown and just ignore the
> button for the purpose of this bug.

What should I used in my selectors though?  I think that we need to have two sets of selectors for this bug, since without the menubar, we might need to reserve some padding on the right side of the tab bar, and put the icon in the background of the tabbar, and that requires me to be able to detect the Ubuntu add-on in the theme using a selector.  (The only other alternative that I can see would be adding a <xul:image> which sucks.

Comment 16

[Dão Gottwald [:dao]]

I'd prefer if we put the indicator in a fixed spot regardless of whether the menu bar or the menu button are shown. And I'd prefer the left side of the tab bar since on the right side it would look like one of the tab strip buttons.

Comment 17

[(no longer active)]

(In reply to comment #16)
> I'd prefer if we put the indicator in a fixed spot regardless of whether the
> menu bar or the menu button are shown. And I'd prefer the left side of the tab
> bar since on the right side it would look like one of the tab strip buttons.

Hmm, shall I put it in an <image>?

Comment 18

[Dão Gottwald [:dao]]

Something like this would also work:

#main-window[privatebrowsingmode=temporary] #TabsToolbar::before {
  display: -moz-box;
  content: "";
  background: url(...) center no-repeat;
  width: ...px;
}

Comment 19

[(no longer active)]

Attachment: Patch (v2)

Thanks, that's a nice trick!

But after trying it out, I decided that the image is too big when the appmenu button shows up, so I ended up keeping the smaller version of that image.

Screenshots from this patch will follow shortly.

Comment 20

[(no longer active)]

Attachment: Regular private window

Comment 21

[(no longer active)]

Attachment: Private window with appmenu visible

Comment 22

[(no longer active)]

Attachment: Private window with no menubar

(By setting hidden=true on the menubar element.)

Comment 23

[Stephen Horlander [:shorlander]]

(In reply to :Ehsan Akhgari from comment #22)
> Created attachment 701239 [details]
> Private window with no menubar
> 
> (By setting hidden=true on the menubar element.)

That looks ok to me. Could you please add some padding so the indicator is centered over the back button and the first tab aligns with the locationbar? e.g. http://cl.ly/image/0A2d1z262h2B

Comment 24

[(no longer active)]

Attachment: Wider icon

Sure, this is how it looks now.

Comment 25

[(no longer active)]

Attachment: Patch (v3)

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 701259 [details] [diff] [review]
Patch (v3)

I don't see the necessity to move the image inside the Firefox button when present. The button isn't otherwise styled for private browsing and the popup doesn't have any special private browsing content. I'd also keep this simple rather than catering for the soon-to-be removed button. So r=me with the browser.js part, privatebrowsing-mask-small.png and the associated CSS removed.

Comment 27

[Chris Coulson]

(In reply to :Ehsan Akhgari from comment #2)
> I first need mock-ups for this.  Also, Chris, can you please tell me how I
> can get a build of the global menu extension which I can use in Firefox 20
> for testing?  Thanks!

Hi,

Sorry I missed this.

You can grab the source from https://code.launchpad.net/~extension-hackers/globalmenu-extension/trunk

To build it, you need a fully built Firefox tree. Then you should be able to do:

autoconf2.13
./configurehelper.sh --enable-application=extensions --enable-extensions=globalmenu --with-libxul-sdk=/<path_to_firefox_objdir>/dist
make

That should work

Comment 28

[(no longer active)]

(In reply to Chris Coulson from comment #27)
> autoconf2.13
> ./configurehelper.sh --enable-application=extensions
> --enable-extensions=globalmenu
> --with-libxul-sdk=/<path_to_firefox_objdir>/dist
> make

Thanks for the instructions, this let me build the extension successfully.  However, now when I try to run Firefox with a profile which has that extension installed, I get a crash at startup.  Here's a backtrace: https://gist.github.com/4531055 (you can ignore frames 0-4 since they're just triggering the profile lock signal handler which is a result of the crash.)

I'm going to land the patch here with Dao's changes.  It would be great if you could try it out to make sure that it works well with the global menubar extension (but I believe that it should.)

Comment 29

[(no longer active)]

Attachment: Updated patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Left-over work from bug 749394
User impact if declined: No theme customization for private windows on Linux
Testing completed (on m-c, etc.): locally, see the screenshots.
Risk to taking this patch (and alternatives if risky): Not risky.
String or UUID changes made by this patch: None.

Comment 30

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd49338d61c7

Comment 31

[Chris Coulson]

(In reply to :Ehsan Akhgari from comment #28)
> (In reply to Chris Coulson from comment #27)
> > autoconf2.13
> > ./configurehelper.sh --enable-application=extensions
> > --enable-extensions=globalmenu
> > --with-libxul-sdk=/<path_to_firefox_objdir>/dist
> > make
> 
> Thanks for the instructions, this let me build the extension successfully. 
> However, now when I try to run Firefox with a profile which has that
> extension installed, I get a crash at startup.  Here's a backtrace:
> https://gist.github.com/4531055 (you can ignore frames 0-4 since they're
> just triggering the profile lock signal handler which is a result of the
> crash.)
> 

Did you run it with the exact same build of Firefox that you built the addon with? It might crash if you don't, particularly if it's debug vs non-debug

> I'm going to land the patch here with Dao's changes.  It would be great if
> you could try it out to make sure that it works well with the global menubar
> extension (but I believe that it should.)

Thanks. I'll try this out once it appears in the nightly :)

Comment 32

[(no longer active)]

(In reply to comment #31)
> (In reply to :Ehsan Akhgari from comment #28)
> > (In reply to Chris Coulson from comment #27)
> > > autoconf2.13
> > > ./configurehelper.sh --enable-application=extensions
> > > --enable-extensions=globalmenu
> > > --with-libxul-sdk=/<path_to_firefox_objdir>/dist
> > > make
> > 
> > Thanks for the instructions, this let me build the extension successfully. 
> > However, now when I try to run Firefox with a profile which has that
> > extension installed, I get a crash at startup.  Here's a backtrace:
> > https://gist.github.com/4531055 (you can ignore frames 0-4 since they're
> > just triggering the profile lock signal handler which is a result of the
> > crash.)
> > 
> 
> Did you run it with the exact same build of Firefox that you built the addon
> with? It might crash if you don't, particularly if it's debug vs non-debug

Yes, I did.

> > I'm going to land the patch here with Dao's changes.  It would be great if
> > you could try it out to make sure that it works well with the global menubar
> > extension (but I believe that it should.)
> 
> Thanks. I'll try this out once it appears in the nightly :)

Great.  I expect this to go into tomorrow's Nightly, but please watch this bug to see when it gets marked as RESOLVED FIXED.  Thanks!

Comment 33

[(no longer active)]

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

Comment 34

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9424fc1b16c6

Comment 35

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0