Richard Marti (:Paenglab) Assignee
	Description • 10 years ago

about:permissions should use the new in-content style-sheet to be able to remove the old inContentUI.css.

This bug is only for the style-sheet change. Further improvements should be done in a new bug.

	Richard Marti (:Paenglab) Assignee
	Comment 1 • 10 years ago

Use global/skin/in-content/common.css and small alignment fixes to the new styles.

	:Gijs (he/him)
	Comment 2 • 10 years ago

Comment on attachment 8600592 [details] [diff] [review]
Bug1160733.patch

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

Sorry I missed my self-imposed 24-hour review turnaround time - I'm still getting on top of everything after being away for a very long weekend.

I'm surprised how few changes were necessary to bring this into line with other things - the patch makes things look a lot better! I do have some feedback, so we're not quite there yet, but it should hopefully not take too many iterations to get right.

I've tested on OS X + Windows for now. Will look at Linux for the next iteration.

This: http://imgur.com/vbNcAba seems to be a pre-existing issue (screenshot from OS X, but this is also misaligned on Windows, it seems.). Can you ensure we have a bug on file about this, or fix it here? (probably makes sense to have a separate bug)

::: browser/themes/osx/preferences/aboutPermissions.css
@@ +47,5 @@
>  
>  /* permissions box */
>  
>  #permissions-box {
> +  padding-top: 10px;

This has lost a padding-bottom, but kept the padding-top.

This is all a little annoying to get right, because in the current scheme of things, the title isn't aligned with the sites-box, but that seems to be because of the vertical alignment of the text - if you select a site, the "forget about this site" button lines up well, and the text of the (site) title lines up with the button. However, the alignment with the #sites-box is destroyed as soon as you scroll the #permissions-box, because the padding is inside the scrollable area.

The scrollable area is also longer than the list, because the list gets compressed by the padding on the #sites-box, but there's no padding or margin on this box. Setting padding doesn't help, because again, that's part of the scrollable area.

It seems like this should no longer have padding-top, but instead get both margin top and bottom of 10px. I'm not sure what to do about the title, so let's leave that alone for now.

... actually, considering the already-quite-generous padding/margins here, maybe we should just nuke the padding-top here and the top/bottom padding on the sites-box? As it is, not all the permissions fit on my (pretty big!) hidpi retina mbp screen without scrolling, which is a bit naff, as the British would say...

@@ +55,5 @@
>  }
>  
>  #site-description {
>    font-size: 125%;
> +  -moz-margin-start: 0;

This is unnecessary. xul|descriptions get -moz-margin-start: 0 anyway:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?force=1#51

(should be removable on Windows and Linux as well)

@@ +67,5 @@
>  
>  #defaults-description {
>    font-size: 125%;
>    font-weight: bold;
> +  -moz-margin-start: 0;

Same.

@@ +131,5 @@
>    font-weight: bold;
>  }
>  
>  .pref-menulist {
> +  min-width: 10em; /* native menulists ellipsize their longest entries by default on many themes */

We fixed this differently for the font dialog, to wit, bug 1108302.

Is this still an issue, and if so, where / which OSes/themes do you mean? Can you experiment with fixing any paddings that might be breaking this?

The min-width makes me somewhat uncomfortable ("Allow" and "Block", at least on OS X, are swimming in space right now, and it's not wide enough for the other options for e.g. cookies, so I'm confused why this is necessary).

@@ +136,5 @@
>  }
> +
> +#cookies-label,
> +#passwords-label {
> +  -moz-margin-start: 4px; /* align with the menulists */

This aligns with the labels, but not with the menulists on OS X:

http://imgur.com/Xdim8Bt

(with apologies for my poor Preview.app skills)

	Richard Marti (:Paenglab) Assignee
	Comment 3 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8600592 [details] [diff] [review]
> Bug1160733.patch
> 
> Review of attachment 8600592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This: http://imgur.com/vbNcAba seems to be a pre-existing issue (screenshot
> from OS X, but this is also misaligned on Windows, it seems.). Can you
> ensure we have a bug on file about this, or fix it here? (probably makes
> sense to have a separate bug)

Should be fixed now. I tested it on Win7, Linux and a OS X VM.

> ::: browser/themes/osx/preferences/aboutPermissions.css
> @@ +47,5 @@
> >  
> >  /* permissions box */
> >  
> >  #permissions-box {
> > +  padding-top: 10px;
> 
> This has lost a padding-bottom, but kept the padding-top.

I made around the content a 48px padding like it's desired for the in-content pages. All other paddings are removed.

> This is all a little annoying to get right, because in the current scheme of
> things, the title isn't aligned with the sites-box, but that seems to be
> because of the vertical alignment of the text - if you select a site, the
> "forget about this site" button lines up well, and the text of the (site)
> title lines up with the button. However, the alignment with the #sites-box
> is destroyed as soon as you scroll the #permissions-box, because the padding
> is inside the scrollable area.
> 
> The scrollable area is also longer than the list, because the list gets
> compressed by the padding on the #sites-box, but there's no padding or
> margin on this box. Setting padding doesn't help, because again, that's part
> of the scrollable area.
> 
> It seems like this should no longer have padding-top, but instead get both
> margin top and bottom of 10px. I'm not sure what to do about the title, so
> let's leave that alone for now.
> 
> ... actually, considering the already-quite-generous padding/margins here,
> maybe we should just nuke the padding-top here and the top/bottom padding on
> the sites-box? As it is, not all the permissions fit on my (pretty big!)
> hidpi retina mbp screen without scrolling, which is a bit naff, as the
> British would say...

I made the title fixed and the scrollable area has the same height as the #sites-box.
> 
> @@ +55,5 @@
> >  }
> >  
> >  #site-description {
> >    font-size: 125%;
> > +  -moz-margin-start: 0;
> 
> This is unnecessary. xul|descriptions get -moz-margin-start: 0 anyway:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/common.inc.css?force=1#51
> 
> (should be removable on Windows and Linux as well)
> 
> @@ +67,5 @@
> >  
> >  #defaults-description {
> >    font-size: 125%;
> >    font-weight: bold;
> > +  -moz-margin-start: 0;
> 
> Same.

Both done.

> @@ +131,5 @@
> >    font-weight: bold;
> >  }
> >  
> >  .pref-menulist {
> > +  min-width: 10em; /* native menulists ellipsize their longest entries by default on many themes */
> 
> We fixed this differently for the font dialog, to wit, bug 1108302.
> 
> Is this still an issue, and if so, where / which OSes/themes do you mean?
> Can you experiment with fixing any paddings that might be breaking this?
> 
> The min-width makes me somewhat uncomfortable ("Allow" and "Block", at least
> on OS X, are swimming in space right now, and it's not wide enough for the
> other options for e.g. cookies, so I'm confused why this is necessary).

This was already on Windows theme and I copied it to the other. I removed it now on all platforms.

> 
> @@ +136,5 @@
> >  }
> > +
> > +#cookies-label,
> > +#passwords-label {
> > +  -moz-margin-start: 4px; /* align with the menulists */
> 
> This aligns with the labels, but not with the menulists on OS X:
> 
> http://imgur.com/Xdim8Bt

OS X needs 2px.

I also added to the .pref-icon a margin-top: 15px; as for me the position was too high.

	:Gijs (he/him)
	Comment 4 • 10 years ago

Comment on attachment 8603541 [details] [diff] [review]
Bug1160733.patch

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

This is where I get punished for not testing every single one of my suggestions from last round. We need to keep the min-width or fix the padding on the label of the menulist somehow, or otherwise on OS X the labels all get elided ("Allow se..." instead of "Allow session").

It looks like nsMenuFrame checks the padding and border of the menulist box, but not of the label in which the item is then placed, which then has a dropmarker as well... I don't know exactly why the math doesn't work out, but I presume it's something like that. :-\

Let's put the min-width back in (at least on OS X - it wfm as-is on win8?) and file a followup bug. Sorry about the hassle. :-(

::: browser/themes/linux/preferences/aboutPermissions.css
@@ +49,3 @@
>    overflow-y: auto;
> +  padding-top: 5px;
> +  -moz-padding-end: 44px;

Playing with this, this seems odd; there's empty space now, but because the scrollbar is on the edge, it looks weird and causes "premature" horizontal scrollbars. Maybe we should make an exception here and make this 10px like it was before?

@@ +67,4 @@
>  }
>  
>  .pref-item {
> +  margin-bottom: 1px;

Hmm, why this change? It looks a bit cramped now. If you did this to make it not so high, I guess I would still go with something like 5px.

@@ +67,5 @@
>  }
>  
>  .pref-item {
> +  margin-bottom: 1px;
> +  -moz-padding-end: 44px;

Ah, so there is now also double padding, isn't there? Maybe that's what gave the impression described above. Why is there end-padding twice? Can we remove one, at least?

@@ +73,5 @@
>  
>  .pref-icon {
>    width: 36px;
>    height: 36px;
> +  margin-top: 15px;

Hrm. But now it doesn't align either with the contents of the item, or with the header, nor is it properly centered because the items have different heights. Can we leave this out and file a followup bug for it with a ux needinfo?

::: browser/themes/osx/preferences/aboutPermissions.css
@@ +132,5 @@
>  }
>  
>  .pref-title {
>    font-size: 125%;
> +  -moz-margin-start: 4px;

This should also be 2px like below, right?

	Richard Marti (:Paenglab) Assignee
	Comment 5 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8603541 [details] [diff] [review]
> Bug1160733.patch
> 
> Review of attachment 8603541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is where I get punished for not testing every single one of my
> suggestions from last round. We need to keep the min-width or fix the
> padding on the label of the menulist somehow, or otherwise on OS X the
> labels all get elided ("Allow se..." instead of "Allow session").
> 
> It looks like nsMenuFrame checks the padding and border of the menulist box,
> but not of the label in which the item is then placed, which then has a
> dropmarker as well... I don't know exactly why the math doesn't work out,
> but I presume it's something like that. :-\
> 
> Let's put the min-width back in (at least on OS X - it wfm as-is on win8?)
> and file a followup bug. Sorry about the hassle. :-(

I've added it on all platforms to have the same look.
> 
> ::: browser/themes/linux/preferences/aboutPermissions.css
> @@ +49,3 @@
> >    overflow-y: auto;
> > +  padding-top: 5px;
> > +  -moz-padding-end: 44px;
> 
> Playing with this, this seems odd; there's empty space now, but because the
> scrollbar is on the edge, it looks weird and causes "premature" horizontal
> scrollbars. Maybe we should make an exception here and make this 10px like
> it was before?

I leaved it at 44px (+4px of margins of the last elements in the box = 48px) to match the 48px on the left. I removed the double padding below.

> @@ +67,4 @@
> >  }
> >  
> >  .pref-item {
> > +  margin-bottom: 1px;
> 
> Hmm, why this change? It looks a bit cramped now. If you did this to make it
> not so high, I guess I would still go with something like 5px.

I overlooked this .pref-item class is also used in content. I use now #site-header and #defaults-header to set the 1px.

> @@ +67,5 @@
> >  }
> >  
> >  .pref-item {
> > +  margin-bottom: 1px;
> > +  -moz-padding-end: 44px;
> 
> Ah, so there is now also double padding, isn't there? Maybe that's what gave
> the impression described above. Why is there end-padding twice? Can we
> remove one, at least?

Same here with the class. I use now #header-deck for the -moz-margin-end: 44px; to align the header end with the content.

> @@ +73,5 @@
> >  
> >  .pref-icon {
> >    width: 36px;
> >    height: 36px;
> > +  margin-top: 15px;
> 
> Hrm. But now it doesn't align either with the contents of the item, or with
> the header, nor is it properly centered because the items have different
> heights. Can we leave this out and file a followup bug for it with a ux
> needinfo?

removed, I'll file a bug.

> ::: browser/themes/osx/preferences/aboutPermissions.css
> @@ +132,5 @@
> >  }
> >  
> >  .pref-title {
> >    font-size: 125%;
> > +  -moz-margin-start: 4px;
> 
> This should also be 2px like below, right?

Yes.

	Dão Gottwald [:dao]
	Comment 6 • 10 years ago

*Sigh* It's sad to see how much time has already been spent here. about:permissions is unfinished, has fundamental design problems and we have no plans on exposing it to users anytime soon. It has been in this state for years, so I would in fact like to just remove it so people don't keep burning their time on it.

	:Gijs (he/him)
	Comment 7 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #6)
> *Sigh* It's sad to see how much time has already been spent here.
> about:permissions is unfinished, has fundamental design problems and we have
> no plans on exposing it to users anytime soon. It has been in this state for
> years, so I would in fact like to just remove it so people don't keep
> burning their time on it.

I'm not sure I see your point. about:permissions keeps coming up in conversation on m.d.platform and fx-dev, and it seems like at least we want "something like it" even if not about:permissions in its current form.

Do you have buy-in to just rm -rf all the relevant files and move on? Because I somewhat doubt this (it's still here! (and also, I talked with madhava about it recently, and heard nothing along the line of "let's kill it")).

If you do not have said buy-in, I don't see what point you're trying to make here. If we're not getting rid of it, it seems fair to take patches that are strict improvements on the status quo.

Can you explain what you expect me to do here?

	Dão Gottwald [:dao]
	Comment 8 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > *Sigh* It's sad to see how much time has already been spent here.
> > about:permissions is unfinished, has fundamental design problems and we have
> > no plans on exposing it to users anytime soon. It has been in this state for
> > years, so I would in fact like to just remove it so people don't keep
> > burning their time on it.
> 
> I'm not sure I see your point. about:permissions keeps coming up in
> conversation on m.d.platform and fx-dev, and it seems like at least we want
> "something like it" even if not about:permissions in its current form.

Yes, it keeps coming up disproportionally often. That's mostly because it's just there, so people understandably assume it's a feature we care about (they often don't know that we never officially released it to end users).

> Do you have buy-in to just rm -rf all the relevant files and move on?
> Because I somewhat doubt this (it's still here! (and also, I talked with
> madhava about it recently, and heard nothing along the line of "let's kill
> it")).

No, I think dolske poked madhava or shorlander once or twice, but I don't think anything meaningful came out of that, be it positive or negative. about:permissions remains a zombie with no salvation in sight.

> If you do not have said buy-in, I don't see what point you're trying to make
> here. If we're not getting rid of it, it seems fair to take patches that are
> strict improvements on the status quo.
> 
> Can you explain what you expect me to do here?

Up to you! If you think it's good use of Richard's and your time, go ahead. I personally don't think it's good use of anyone's time at this point.

	Richard Marti (:Paenglab) Assignee
	Comment 9 • 10 years ago

At least it's good to get rid of inContentUI.css. I for me have no problems to spend time on this bug.

	:Gijs (he/him)
	Comment 10 • 10 years ago

Comment on attachment 8604845 [details] [diff] [review]
Bug1160733.patch

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

Looks good on Windows, minor nit on Mac. Linux looks OK from code-inspection, so with the OSX nit adjusted this is shippable as far as I'm concerned.

::: browser/themes/osx/preferences/aboutPermissions.css
@@ +67,5 @@
>    margin-bottom: 0;
>  }
>  
> +#site-visit-count {
> +  margin-bottom: 3px;

On my mac, I noticed after zooming in that 4px is more appropriate here. (otherwise it's still out)

Also, the Linux styles don't include this. Not necessary there?

	Richard Marti (:Paenglab) Assignee
	Comment 11 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8604845 [details] [diff] [review]
> Bug1160733.patch
> 
> Review of attachment 8604845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good on Windows, minor nit on Mac. Linux looks OK from
> code-inspection, so with the OSX nit adjusted this is shippable as far as
> I'm concerned.
> 
> ::: browser/themes/osx/preferences/aboutPermissions.css
> @@ +67,5 @@
> >    margin-bottom: 0;
> >  }
> >  
> > +#site-visit-count {
> > +  margin-bottom: 3px;
> 
> On my mac, I noticed after zooming in that 4px is more appropriate here.
> (otherwise it's still out)

Fixed

> Also, the Linux styles don't include this. Not necessary there?

I checked again and on my Mint Linux they are correctly aligned.

	Richard Marti (:Paenglab) Assignee
	Comment 12 • 10 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46e7e011da14

I don't think the failed tests are from my patch as they are on all platforms different.

	Pulsebot
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/7a18ec4ba2fb

	Carsten Book [:Tomcat]
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7a18ec4ba2fb

	Rezaul Huque Nayeem
	Comment 15 • 9 years ago

Reproduced the bug in 40.0a1 (2015-05-02) on windows 10 x64

Verified as fixed with latest firefox release 41.0.2 (Build ID: 20151014143721)

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0

	Nazir Ahmed Sabbir [:NaSb]
	Comment 16 • 9 years ago

Reproduced this bug on Nightly 40.0a1 (2015-05-02) in Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Release 41.0.2

Build ID: 20151015172900
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0