Closed Bug 479992 Opened 11 years ago Closed 11 years ago

Better fix for the "Halo"/Extended border around buttons on modern theme

Categories

(SeaMonkey :: Themes, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #418090 +++

<RattyAway> NeilZZZ: regarding Bug 418090 why couldn't -moz-background-clip: padding; have been used instead of extending the margin?

Memo 1 from NeilAway (Feb 21 11:57:20 2009 PST)
I hadn't heard of that - the earlier bug that implemented it deliberately made it the default when using -moz-border-xxx-colour: transparent; so when vlad rewrote border handling I didn't know how easy it was to fix :-( file a patch to revert all my changes and use that style instead?

So basically we need to backout all the changes in bug 418090 and instead use:
button {
  -moz-background-clip: padding;
}
That's not quite true since not all of the patches landed and the "error"-type pages were "fixed" in a different bug.
> the "error"-type pages were "fixed" in a different bug.
I didn't see any dependencies (blocks/depends) in Bug 418090?
Please add those bugs to the dependency list here.
Depends on: 36810
Neil, I think you mean Bug 459550, specifically attachment 360367 [details] [diff] [review]. Which changes in this patch need to be backed out or all of it?
(In reply to comment #3)
> Neil, I think you mean Bug 459550, specifically attachment 360367 [details] [diff] [review].
> Which changes in this patch need to be backed out or all of it?
All of it; plus aboutSessionStore.css would need fixing.
Attached patch Patch v1.0 Combat Evolved (obsolete) — Splinter Review
>> Neil, I think you mean Bug 459550, specifically attachment 360367 [details] [diff] [review].
>> Which changes in this patch need to be backed out or all of it?
> All of it; plus aboutSessionStore.css would need fixing.

I fixed about:Sessionrestore in netError.css

But I have a problem with artefacts at the corners of the buttons.
Assignee: neil → philip.chee
Attached image Screenshot of problem.
There are faint cusps in each corner of the buttons. I can make them disappear by making removing the radius on the button borders but then the buttons look too "square".
Comment on attachment 365196 [details] [diff] [review]
Patch v1.0 Combat Evolved

>+  -moz-background-clip: padding;
I'd prefer this next to the background colour, it makes more sense there.

>+  
> /* .......... disabled state .......... */
Looks like this had trailing whitespace. No point putting it back ;-)

>+#buttons,
>+#errorPageContainer > #errorTryAgain {
These are to avoid changing aboutSessionStore.css?
> (From update of attachment 365196 [details] [diff] [review])
>>+  -moz-background-clip: padding;
> I'd prefer this next to the background colour, it makes more sense there.
Sure.
 
>>+  
>> /* .......... disabled state .......... */
> Looks like this had trailing whitespace. No point putting it back ;-)
Righto!


>>+#buttons,
>>+#errorPageContainer > #errorTryAgain {
> These are to avoid changing aboutSessionStore.css?

Well reading through Bug 459550 particularly Bug 459550 Comment 24 it seems that you prefer that styles be moved out of aboutSessionrestore.css and moved to netError.css.

If this isn't the case I'll move the relevant lines to aboutSessionrestore.css.
Depends on: 459550
(In reply to comment #8)
>>(From update of attachment 365196 [details] [diff] [review])
>>>+#buttons,
>>>+#errorPageContainer > #errorTryAgain {
>>These are to avoid changing aboutSessionStore.css?
>Well reading through Bug 459550 particularly Bug 459550 Comment 24 it seems
>that you prefer that styles be moved out of aboutSessionrestore.css and moved
>to netError.css.
True, although in that case the rules were exact duplicates.
Depends on: 456219
Keywords: helpwanted
Attached patch Patch v2.0 Collectors Edition (obsolete) — Splinter Review
The fix for bug 456219 has landed on branch.

>>+  -moz-background-clip: padding;
> I'd prefer this next to the background colour, it makes more sense there.
Fixed.

>>+  
>> /* .......... disabled state .......... */
> Looks like this had trailing whitespace. No point putting it back ;-)
Fixed!

>>+#buttons,
>>+#errorPageContainer > #errorTryAgain {
> These are to avoid changing aboutSessionStore.css?
Moved #buttons to aboutSessionRestore.css
Attachment #365196 - Attachment is obsolete: true
Attachment #373117 - Flags: superreview?(neil)
Attachment #373117 - Flags: review?(neil)
Attached image Screenshot of fix.
The artifacts at the corners are now gone thanks to Bug 456219
Comment on attachment 373117 [details] [diff] [review]
Patch v2.0 Collectors Edition

>diff --git a/suite/themes/modern/communicator/aboutSessionRestore.css b/suite/themes/modern/communicator/aboutSessionRestore.css
>--- a/suite/themes/modern/communicator/aboutSessionRestore.css
>+++ b/suite/themes/modern/communicator/aboutSessionRestore.css
>@@ -35,16 +35,21 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #tabList {
>   width: 100%;
>   height: 12em;
> }
> 
>+#buttons {
>+  margin-top: 2em;
>+  -moz-margin-start: 80px;
>+}
Nit: this belongs at the end of the file.
Attachment #373117 - Flags: superreview?(neil)
Attachment #373117 - Flags: superreview+
Attachment #373117 - Flags: review?(neil)
Attachment #373117 - Flags: review+
>>+#buttons {
>>+  margin-top: 2em;
>>+  -moz-margin-start: 80px;
>>+}
> Nit: this belongs at the end of the file.
Fixed
Attachment #373117 - Attachment is obsolete: true
Attachment #373140 - Flags: superreview+
Attachment #373140 - Flags: review+
Keywords: checkin-needed
Comment on attachment 373140 [details] [diff] [review]
Patch v2.1 Cortana [for checkin]
[Checkin: Comment 14]


http://hg.mozilla.org/comm-central/rev/b56a6a7435fd
Attachment #373140 - Attachment description: Patch v2.1 Cortana [for checkin] → Patch v2.1 Cortana [for checkin] [Checkin: Comment 14]
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1
You need to log in before you can comment on or make changes to this bug.