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

RESOLVED FIXED in seamonkey2.0b1

Status

SeaMonkey
Themes
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

({regression})

Trunk
seamonkey2.0b1
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
+++ 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;
}

Comment 1

9 years ago
That's not quite true since not all of the patches landed and the "error"-type pages were "fixed" in a different bug.
(Assignee)

Comment 2

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

Updated

9 years ago
Depends on: 36810
(Assignee)

Comment 3

9 years ago
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?

Comment 4

9 years ago
(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.
(Assignee)

Comment 5

9 years ago
Created attachment 365196 [details] [diff] [review]
Patch v1.0 Combat Evolved

>> 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
(Assignee)

Comment 6

9 years ago
Created attachment 365198 [details]
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 7

9 years ago
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?
(Assignee)

Comment 8

9 years ago
> (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

Comment 9

9 years ago
(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.
(Assignee)

Updated

9 years ago
Depends on: 456219
(Assignee)

Updated

9 years ago
Keywords: helpwanted
(Assignee)

Comment 10

8 years ago
Created attachment 373117 [details] [diff] [review]
Patch v2.0 Collectors Edition

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)
(Assignee)

Comment 11

8 years ago
Created attachment 373121 [details]
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+
(Assignee)

Comment 13

8 years ago
Created attachment 373140 [details] [diff] [review]
Patch v2.1 Cortana [for checkin]
[Checkin: Comment 14]

>>+#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+
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 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.