Last Comment Bug 533348 - Correct changeset 0174b636d5ee (fix broken Modern rules in alert.css/accountCentral.css/pageInfo.css) (new mail notification/download complete/update "toaster" tray area popup text and Page Info Feeds Tab items missing margin/padding)
: Correct changeset 0174b636d5ee (fix broken Modern rules in alert.css/accountC...
Status: RESOLVED FIXED
: fixed-seamonkey2.0.3
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-07 15:03 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2009-12-15 11:35 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
apply Classic rules to Modern (901 bytes, patch)
2009-12-07 15:07 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
faked Modern download alert (5.32 KB, image/jpeg)
2009-12-07 15:07 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details
faked Modern mail alert (4.99 KB, image/jpeg)
2009-12-07 15:08 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details
faked Classic download alert (5.66 KB, image/jpeg)
2009-12-07 15:08 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details
faked Classic mail alert (5.18 KB, image/jpeg)
2009-12-07 15:09 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details
example toaster trigger script (1011 bytes, text/plain)
2009-12-07 15:10 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details
correct changeset 0174b636d5ee [Checkin: comment 11] (1.60 KB, patch)
2009-12-14 10:28 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: approval‑seamonkey2.0.3+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:03:54 PST
The file global/alerts/alert.css is responsible for the styling of new mail notifications/download complete/update "toaster" tray area popups. Classic and Modern are mostly the same regarding this file but there is one notable difference:

Modern has:

.alertTextBox {
  margin-top: 4px;
  margin: 0 6px;
}

Classic has:

.alertBox[orient="horizontal"] > .alertTextBox {
  -moz-padding-end: 10px;
  padding-top: 5px;
}

.alertBox[orient="vertical"] > .alertTextBox {
  -moz-padding-start: 5px;
  -moz-padding-end: 5px;
  margin-bottom: 8px;
  -moz-box-align: center; /* also hard-coded in alert.js, see bug 311557 */
}

Don't ask me why but the margin-top doesn't work (anymore?). I'll attach screen shots, a little script to trigger the alerts, and a patch that replaces Modern's rule by Classic's (currently applies cleanly to both trunk and branch).
Comment 1 Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:07:14 PST
Created attachment 416463 [details] [diff] [review]
apply Classic rules to Modern 

Patch applies to both trunk and branch.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:07:50 PST
Created attachment 416464 [details]
faked Modern download alert
Comment 3 Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:08:25 PST
Created attachment 416465 [details]
faked Modern mail alert
Comment 4 Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:08:43 PST
Created attachment 416466 [details]
faked Classic download alert
Comment 5 Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:09:02 PST
Created attachment 416467 [details]
faked Classic mail alert
Comment 6 Jens Hatlak (:InvisibleSmiley) 2009-12-07 15:10:22 PST
Created attachment 416468 [details]
example toaster trigger script
Comment 7 neil@parkwaycc.co.uk 2009-12-07 16:04:48 PST
(In reply to comment #0)
> .alertTextBox {
>   margin-top: 4px;
>   margin: 0 6px;
> }

> Don't ask me why but the margin-top doesn't work (anymore?).
Oops. It used to have separate top, right and left margins. They weren't properly combined in bug 474807 :-(
Comment 8 Jens Hatlak (:InvisibleSmiley) 2009-12-13 13:17:15 PST
FWIW if possible I'd like to have at least the rather striking visual glitch described here corrected on the 2.0 branch. On trunk we might have to fix all rules that were combined incorrectly through the checkin for bug 474807 but that should probably be done in a new bug then, for all affected parties.
Comment 9 Jens Hatlak (:InvisibleSmiley) 2009-12-14 10:28:35 PST
Created attachment 417506 [details] [diff] [review]
correct changeset 0174b636d5ee [Checkin: comment 11]

This simply fixes the "definite issues" from bug 474807 comment 21 without changing the actual approach (i.e. I left out the .alertTextBox -> alertBox[orient="horizontal|vertical"] > .alertTextBox change).

Notes:
* alert.css: introduces margin-bottom:0 compared to before changeset 0174b636d5ee but that's OK since no other rule sets margin-bottom here (I checked)
* accountCentral.css: the re-introduced padding-left:10px has no visual effect since the .acctCentralRow rule already indents the whole area using -moz-margin-start:10px. If there's a desire to drop the padding-left due to that, IMHO the rule should at least be adapted to read padding: 10px 0;
Comment 10 neil@parkwaycc.co.uk 2009-12-14 16:20:26 PST
Comment on attachment 417506 [details] [diff] [review]
correct changeset 0174b636d5ee [Checkin: comment 11]

I don't see why this can't go in 2.0.2, given that it's a medium-reward low-risk regression fix.
Comment 11 Jens Hatlak (:InvisibleSmiley) 2009-12-15 11:34:32 PST
Comment on attachment 417506 [details] [diff] [review]
correct changeset 0174b636d5ee [Checkin: comment 11]

http://hg.mozilla.org/comm-central/rev/111318ebf2da
http://hg.mozilla.org/releases/comm-1.9.1/rev/27bc34d4c18a

Note You need to log in before you can comment on or make changes to this bug.