Closed Bug 658530 Opened 9 years ago Closed 8 years ago

Update about:permissions style to use common in-content page styles

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 8

People

(Reporter: Margaret, Assigned: Unfocused)

References

Details

(Whiteboard: [qa!])

Attachments

(4 files, 5 obsolete files)

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.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Note: Requires patch from bug 658431.
Attachment #536211 - Flags: review?(dao)
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.
Attachment #536211 - Flags: review?(dao) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #536211 - Attachment is obsolete: true
Attachment #538436 - Flags: review?(dao)
Dao: Any chance you could get to this this week?
yep
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.
Attachment #538436 - Attachment is obsolete: true
Attachment #538436 - Flags: review?(dao)
Attached patch Part 1 - in-content CSS - v3 (obsolete) — Splinter Review
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.
Attachment #540682 - Flags: review?(dao)
Attached patch Part 2 - about:permissions - v3 (obsolete) — Splinter Review
Attachment #540683 - Flags: review?(dao)
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.
Attachment #540682 - Flags: review?(dao) → review-
Attachment #543689 - Attachment description: about:permissions with native widget styling → about:permissions with native XP Luna styling
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.
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).
Attached patch Part 1 - in-content CSS - v4 (obsolete) — Splinter Review
Attachment #540682 - Attachment is obsolete: true
Attachment #550958 - Flags: review?(dao)
Attachment #540683 - Attachment is obsolete: true
Attachment #540683 - Flags: review?(dao)
Attachment #550959 - Flags: review?(dao)
Forgot to mention: This version fixes the styling of menulists on OSX. Spoke with shorlander, and he gave a verbal r+ on it.
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?
(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.
The meantime probably includes the Firefox 8 release, so I'd prefer any interim solution to be limited to extensions.css.
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).
Attachment #550958 - Attachment is obsolete: true
Attachment #550958 - Flags: review?(dao)
Attachment #551731 - Flags: review?(dao)
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?
(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 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.
Attachment #550959 - Flags: review?(dao) → review+
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
Attachment #551731 - Flags: review?(dao) → review+
Comment on attachment 551731 [details] [diff] [review]
[checked-in] Part 1 - in-content CSS - v4.1

http://hg.mozilla.org/mozilla-central/rev/fd84808b1edd
Attachment #551731 - Attachment description: Part 1 - in-content CSS - v4.1 → [checked-in] Part 1 - in-content CSS - v4.1
Comment on attachment 550959 [details] [diff] [review]
[checked-in] Part 2 - about:permissions - v4

http://hg.mozilla.org/mozilla-central/rev/c14bbf65c00e
Attachment #550959 - Attachment description: Part 2 - about:permissions - v4 → [checked-in] Part 2 - about:permissions - v4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
You need to log in before you can comment on or make changes to this bug.