Last Comment Bug 658530 - Update about:permissions style to use common in-content page styles
: Update about:permissions style to use common in-content page styles
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 8
Assigned To: Blair McBride [:Unfocused] (UNAVAILABLE)
:
Mentors:
Depends on: 685101 658431
Blocks: 476430 573176
  Show dependency treegraph
 
Reported: 2011-05-20 07:33 PDT by :Margaret Leibovic
Modified: 2013-12-27 14:33 PST (History)
15 users (show)
blair: in‑testsuite-
blair: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.69 KB, patch)
2011-05-30 19:19 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dao+bmo: review-
Details | Diff | Splinter Review
Patch v2 (16.45 KB, patch)
2011-06-09 22:12 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
Part 1 - in-content CSS - v3 (13.09 KB, patch)
2011-06-20 23:01 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dao+bmo: review-
Details | Diff | Splinter Review
Part 2 - about:permissions - v3 (5.70 KB, patch)
2011-06-20 23:03 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
about:permissions with native XP Luna styling (95.11 KB, image/png)
2011-07-03 12:25 PDT, Dão Gottwald [:dao]
no flags Details
about:permissions with custom styling (100.43 KB, image/png)
2011-07-03 12:25 PDT, Dão Gottwald [:dao]
no flags Details
Part 1 - in-content CSS - v4 (13.88 KB, patch)
2011-08-05 00:23 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
[checked-in] Part 2 - about:permissions - v4 (5.54 KB, patch)
2011-08-05 00:23 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dao+bmo: review+
Details | Diff | Splinter Review
[checked-in] Part 1 - in-content CSS - v4.1 (15.12 KB, patch)
2011-08-09 05:27 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dao+bmo: review+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2011-05-20 07:33:04 PDT
Blair is working in bug 658431 to create common styles for in-content pages. When that is finished, we should use those styles in about:permissions.
Comment 1 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-05-30 19:19:59 PDT
Created attachment 536211 [details] [diff] [review]
Patch v1

Note: Requires patch from bug 658431.
Comment 2 Dão Gottwald [:dao] 2011-06-02 14:06:08 PDT
Comment on attachment 536211 [details] [diff] [review]
Patch v1

>-      <textbox id="sites-filter"
>+      <textbox id="sites-filter" class="search"
>                emptytext="&sites.search;"
>                oncommand="AboutPermissions.filterSitesList();"
>                type="search"/>

This doesn't seem to fit here.
Comment 3 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-06-09 22:12:05 PDT
Created attachment 538436 [details] [diff] [review]
Patch v2

Moves the button styling to this patch, from the original patch in bug 658431. Since that original patch, bug 653637 has landed - which provides a way for extensions to have in-line preferences. We can't control the classes of the buttons/menulists there, so I've made the styles apply to all buttons (except .button-link and .text-link). Which works out ok, because we were trying to style all buttons anyway. There were some issues with styling after 653637 which are more evident in about:permissions, so I've cleaned that up here.
Comment 4 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-06-20 05:48:19 PDT
Dao: Any chance you could get to this this week?
Comment 5 Dão Gottwald [:dao] 2011-06-20 05:49:10 PDT
yep
Comment 6 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-06-20 07:01:06 PDT
Warning: the current patch is rather bitrotten, due to addons manager changes. I'll get the updated patch finished tomorrow, which has some additions, but no major changes.
Comment 7 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-06-20 23:01:00 PDT
Created attachment 540682 [details] [diff] [review]
Part 1 - in-content CSS - v3

I've split this into 2 parts. Part 1 (this patch) is moving over more additional styles into in-content stylesheets (this patch) so about:permissions can use it. Part 2 is converting about:permissions itself. 

Had to fix more bitrot from additions to inline prefs for the addons manager (eg, bug 662012), and did a bunch of tweaks to make those fit in better with the overall in-content style.
Comment 8 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-06-20 23:03:12 PDT
Created attachment 540683 [details] [diff] [review]
Part 2 - about:permissions - v3
Comment 9 Dão Gottwald [:dao] 2011-07-03 12:24:20 PDT
Comment on attachment 540682 [details] [diff] [review]
Part 1 - in-content CSS - v3

Sorry for the delay. I was worried about how this would integrate with non-Aero environments (e.g. XP Luna or Classic). My testing mostly confirms my worries; I think we should keep integrating with the host OS rather than ditching the native appearance. You may consider it ugly, but then you probably don't use those OS themes all day...

This also makes menulists look like buttons, which seems somewhat unexpected.
Comment 10 Dão Gottwald [:dao] 2011-07-03 12:25:07 PDT
Created attachment 543689 [details]
about:permissions with native XP Luna styling
Comment 11 Dão Gottwald [:dao] 2011-07-03 12:25:33 PDT
Created attachment 543690 [details]
about:permissions with custom styling
Comment 12 Dão Gottwald [:dao] 2011-07-03 12:36:59 PDT
Comment on attachment 540683 [details] [diff] [review]
Part 2 - about:permissions - v3

>--- a/browser/themes/winstripe/browser/preferences/aboutPermissions.css
>+++ b/browser/themes/winstripe/browser/preferences/aboutPermissions.css

>+#sites-list {
>+  /* Needed to allow the radius to clip the inner content, see bug 595656 */
>+  /* Disabled because of bug 623615
>+  overflow: hidden;
>+  border-radius: 3.5px;
>+  */
>+  -moz-appearance: none;
>+  border: 1px solid rgba(0, 0, 0, 0.32);
>+  background-color: rgba(255, 255, 255, 0.4);
>+  margin: 5px 0 0 0;
>+}

The commented out code doesn't seem to make sense, since #sites-list doesn't touch the container's border.
Comment 13 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-07-03 13:21:29 PDT
Thanks for getting to this :)

(In reply to comment #9)
> I was worried about how this would integrate with
> non-Aero environments (e.g. XP Luna or Classic). My testing mostly confirms
> my worries; I think we should keep integrating with the host OS rather than
> ditching the native appearance.

Just to clarify: you mean keep native styling on only non-Aero themes (XP, Classic), correct? ie, Aero still gets non-native buttons, etc.

I had wondered about doing that, but was unsure. So that's fine with me. I'll have to double check to make sure there are no regressions in the addons manager, and include any fixes.

> This also makes menulists look like buttons, which seems somewhat unexpected.

I'll ask shorlander about this.

(In reply to comment #12)
> The commented out code doesn't seem to make sense, since #sites-list doesn't
> touch the container's border.

In this case, #sites-list is the container. If the richlistbox has border-radius, it needs overflow:hidden; to make clipping work correctly (bug 595656 / bug 597927) - and that currently results in poor scrolling performance (bug 623615).
Comment 14 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-08-05 00:23:00 PDT
Created attachment 550958 [details] [diff] [review]
Part 1 - in-content CSS - v4
Comment 15 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-08-05 00:23:42 PDT
Created attachment 550959 [details] [diff] [review]
[checked-in] Part 2 - about:permissions - v4
Comment 16 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-08-05 00:26:39 PDT
Forgot to mention: This version fixes the styling of menulists on OSX. Spoke with shorlander, and he gave a verbal r+ on it.
Comment 17 Dão Gottwald [:dao] 2011-08-08 02:02:27 PDT
Comment on attachment 550958 [details] [diff] [review]
Part 1 - in-content CSS - v4

>--- a/toolkit/themes/winstripe/global/inContentUI.css
>+++ b/toolkit/themes/winstripe/global/inContentUI.css
>@@ -100,8 +100,80 @@
>   */
>   background-color: rgba(255, 255, 255, 0.35);
>   background-image: -moz-linear-gradient(top,
>                                          rgba(255, 255, 255, 0),
>                                          rgba(255, 255, 255, 0.75));
>   border: 1px solid #C3CEDF;
>   border-radius: 5px;
> }
>+
>+%ifdef WINSTRIPE_AERO
>+@media all and (-moz-windows-compositor) {
>+  /* Buttons */
>+  button:not(.button-link):not(.text-link),

button-link is about:addons-specific, isn't it? I don't think we want to establish that class more broadly. Why are these buttons in the first place?
Comment 18 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-08-08 02:42:43 PDT
(In reply to Dão Gottwald [:dao] from comment #17)
> button-link is about:addons-specific, isn't it? I don't think we want to
> establish that class more broadly. Why are these buttons in the first place?

Yes, it's specific to about:addons right now. They look like a link, but act like buttons (you'd have to ask Boriss/shorlander as to why they were originally designed to look like links). There had been talk of the possibility of making them look like buttons, but nothing came of it - I just filed bug 677170 for it.

In the mean-time (until bug 677170 is fixed), I can leave that exception in inContentUI.css or override everything in extensions.css (which is a lot more code change). Or I guess I could roll the fix for bug 677170 into this patch.
Comment 19 Dão Gottwald [:dao] 2011-08-08 04:16:57 PDT
The meantime probably includes the Firefox 8 release, so I'd prefer any interim solution to be limited to extensions.css.
Comment 20 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-08-09 05:27:32 PDT
Created attachment 551731 [details] [diff] [review]
[checked-in] Part 1 - in-content CSS - v4.1

I've taken the exceptions out of inContentUI.css and made the .button-link class in extensions.css override additional things (the change to the selector is needed to make it more specific than the button selector in inContentUI.css).
Comment 21 Dão Gottwald [:dao] 2011-08-09 05:44:24 PDT
Comment on attachment 551731 [details] [diff] [review]
[checked-in] Part 1 - in-content CSS - v4.1

>--- a/toolkit/themes/pinstripe/global/inContentUI.css
>+++ b/toolkit/themes/pinstripe/global/inContentUI.css

>+/* Buttons */
>+button:not(.text-link),

What is :not(.text-link) needed for?
Comment 22 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-08-09 06:12:09 PDT
(In reply to Dão Gottwald [:dao] from comment #21)
> What is :not(.text-link) needed for?

Oops, I thought .text-link was for xul:button, but it's for xul:label. Will remove that.
Comment 23 Dão Gottwald [:dao] 2011-08-10 01:12:47 PDT
Comment on attachment 550959 [details] [diff] [review]
[checked-in] Part 2 - about:permissions - v4

>--- a/browser/themes/pinstripe/browser/preferences/aboutPermissions.css
>+++ b/browser/themes/pinstripe/browser/preferences/aboutPermissions.css

>+#sites-list {
>+  /* Needed to allow the radius to clip the inner content, see bug 595656 */

I don't think this comment applies here.

>+  /* Disabled because of bug 623615
>+  overflow: hidden;
>+  border-radius: 3.5px;
>+  background-clip: padding-box;
>+  */

This whole block should probably go away.
Comment 24 Dão Gottwald [:dao] 2011-08-10 01:16:38 PDT
Comment on attachment 551731 [details] [diff] [review]
[checked-in] Part 1 - in-content CSS - v4.1

r=me with :not(.text-link) removed throughout this patch
Comment 26 Rob Campbell [:rc] (:robcee) 2011-08-12 06:57:21 PDT
Comment on attachment 551731 [details] [diff] [review]
[checked-in] Part 1 - in-content CSS - v4.1

http://hg.mozilla.org/mozilla-central/rev/fd84808b1edd
Comment 27 Rob Campbell [:rc] (:robcee) 2011-08-12 06:57:50 PDT
Comment on attachment 550959 [details] [diff] [review]
[checked-in] Part 2 - about:permissions - v4

http://hg.mozilla.org/mozilla-central/rev/c14bbf65c00e
Comment 28 Ioana (away) 2011-11-03 06:03:13 PDT
Verified as fixed on:
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111101 Firefox/9.0a2
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111101 Firefox/10.0a1

Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111102 Firefox/9.0a2
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111026 Firefox/10.0a1

Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111103 Firefox/9.0a2
Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111103 Firefox/10.0a1

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0a2) Gecko/20111027 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111101 Firefox/10.0a1

Note You need to log in before you can comment on or make changes to this bug.