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

VERIFIED FIXED in Firefox 8

Status

()

Firefox
Theme
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Unfocused)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa!])

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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
Created attachment 536211 [details] [diff] [review]
Patch v1

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-
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.
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)
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.
Attachment #540682 - Flags: review?(dao)
Created attachment 540683 [details] [diff] [review]
Part 2 - about:permissions - v3
Attachment #540683 - Flags: review?(dao)
Blocks: 476430
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-
Created attachment 543689 [details]
about:permissions with native XP Luna styling
Created attachment 543690 [details]
about:permissions with custom styling

Updated

6 years ago
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).
Created attachment 550958 [details] [diff] [review]
Part 1 - in-content CSS - v4
Attachment #540682 - Attachment is obsolete: true
Attachment #550958 - Flags: review?(dao)
Created attachment 550959 [details] [diff] [review]
[checked-in] Part 2 - about:permissions - v4
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.
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).
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+
https://hg.mozilla.org/integration/fx-team/rev/fd84808b1edd
https://hg.mozilla.org/integration/fx-team/rev/c14bbf65c00e
Flags: in-testsuite-
Flags: in-litmus-
Whiteboard: [fixed-in-fx-team]
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Depends on: 685101

Comment 28

6 years ago
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.