Closed
Bug 658530
Opened 13 years ago
Closed 13 years ago
Update about:permissions style to use common in-content page styles
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 8
People
(Reporter: Margaret, Assigned: Unfocused)
References
Details
(Whiteboard: [qa!])
Attachments
(4 files, 5 obsolete files)
95.11 KB,
image/png
|
Details | |
100.43 KB,
image/png
|
Details | |
5.54 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
15.12 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•13 years ago
|
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•13 years ago
|
||
Note: Requires patch from bug 658431.
Attachment #536211 -
Flags: review?(dao)
Comment 2•13 years ago
|
||
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-
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
Dao: Any chance you could get to this this week?
Comment 5•13 years ago
|
||
yep
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #538436 -
Attachment is obsolete: true
Attachment #538436 -
Flags: review?(dao)
Assignee | ||
Comment 7•13 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #540683 -
Flags: review?(dao)
Comment 9•13 years ago
|
||
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-
Comment 10•13 years ago
|
||
Comment 11•13 years ago
|
||
Updated•13 years ago
|
Attachment #543689 -
Attachment description: about:permissions with native widget styling → about:permissions with native XP Luna styling
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
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).
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #540682 -
Attachment is obsolete: true
Attachment #550958 -
Flags: review?(dao)
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #540683 -
Attachment is obsolete: true
Attachment #540683 -
Flags: review?(dao)
Attachment #550959 -
Flags: review?(dao)
Assignee | ||
Comment 16•13 years ago
|
||
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•13 years ago
|
||
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?
Assignee | ||
Comment 18•13 years ago
|
||
(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•13 years ago
|
||
The meantime probably includes the Firefox 8 release, so I'd prefer any interim solution to be limited to extensions.css.
Assignee | ||
Comment 20•13 years ago
|
||
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 21•13 years ago
|
||
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?
Assignee | ||
Comment 22•13 years ago
|
||
(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•13 years ago
|
||
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 24•13 years ago
|
||
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+
Assignee | ||
Comment 25•13 years ago
|
||
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 26•13 years ago
|
||
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 27•13 years ago
|
||
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
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Comment 28•13 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.
Description
•