652315 - [Modern 2.1] global fixes: notification.css to zymurgy

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Philip Chee]

Notes:
Bug 418521 Focus ring appears on mouse interactions (as opposed to only when tabbing through items)
  ->Add -moz-focusring property to apply only when focus rings should be drawn.

notification.css
  Bug 615318 - Add a "not now" choice to all doorhanger notification split buttons
  We can use global/icons/closebox.gif

resizer.css
  Bug 554810, use transparent resizer images.
  Hmm. I remember Neil doesn't like transparent resizers. Skipping.

splitter.css
  Bug 547492 - Use correct resize cursor for collapsed splitters (also make grippy arrows rtl-friendly).
  Bug 589570 Disabled splitters should use the default cursor.

toolbarbutton.css
  Bug 418521 Add -moz-focusring property to apply only when focus rings should be drawn.
  Bug 479439 toolbarbutton-icon shouldn't have a margin if the toolbarbutton doesn't have a label.

tree.css
  Bug 176244 Fix column resize and reorder issues when direction is rtl.
  Bug 418521 Add -moz-focusring property to apply only when focus rings should be drawn.

alerts/alert.css
  Bug 133527 New mail notification "banner" at wrong place (always pops up at bottom right hand corner).

To test use:
Components.classes["@mozilla.org/alerts-service;1"]
          .getService(Components.interfaces.nsIAlertsService)
          .showAlertNotification(null, "Notification test",
                                 "Surprise! I'm here to test notifications!",
                                 false, "foobarcookie", null);

media/videocontrols.css
  Bug 465839 - Add a binding for touch enabled video and audio controls.
  -> Hide Firefox Mobile specific UI.

> This is basically duplicating the existing controls, with the small addition of
> <label class="positionLabel"/>. I think we should just go ahead and pull that
> label into the main videocontrols (with display:none, and you'll unhide it for
> mobile).

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fixes.

> Notes:
> Bug 418521 Focus ring appears on mouse interactions (as opposed to only when
> tabbing through items)
>   ->Add -moz-focusring property to apply only when focus rings should be drawn.
> 
> notification.css
>   Bug 615318 - Add a "not now" choice to all doorhanger notification split
> buttons
>   We can use global/icons/closebox.gif
> 
> resizer.css
>   Bug 554810, use transparent resizer images.
>   Hmm. I remember Neil doesn't like transparent resizers. Skipping.
> 
> splitter.css
>   Bug 547492 - Use correct resize cursor for collapsed splitters (also make
> grippy arrows rtl-friendly).
>   Bug 589570 Disabled splitters should use the default cursor.
> 
> toolbarbutton.css
>   Bug 418521 Add -moz-focusring property to apply only when focus rings should
> be drawn.
>   Bug 479439 toolbarbutton-icon shouldn't have a margin if the toolbarbutton
> doesn't have a label.
> 
> tree.css
>   Bug 176244 Fix column resize and reorder issues when direction is rtl.
>   Bug 418521 Add -moz-focusring property to apply only when focus rings should
> be drawn.
> 
> alerts/alert.css
>   Bug 133527 New mail notification "banner" at wrong place (always pops up at
> bottom right hand corner).
> 
> To test use:
> Components.classes["@mozilla.org/alerts-service;1"]
>           .getService(Components.interfaces.nsIAlertsService)
>           .showAlertNotification(null, "Notification test",
>                                  "Surprise! I'm here to test notifications!",
>                                  false, "foobarcookie", null);
> 
> media/videocontrols.css
>   Bug 465839 - Add a binding for touch enabled video and audio controls.
>   -> Hide Firefox Mobile specific UI.
> 
>> This is basically duplicating the existing controls, with the small addition of
>> <label class="positionLabel"/>. I think we should just go ahead and pull that
>> label into the main videocontrols (with display:none, and you'll unhide it for
>> mobile).

Comment 2

[[e-mail]]

Comment on attachment 527925 [details] [diff] [review]
Patch v1.0 Proposed fixes.

>+.alertBox[orient="vertical"] > .alertTextBox {
>+  margin: 0 4px 6px;
>+  -moz-box-align: center;
Nit: include the comment about bug 311557 perhaps?

>+  cursor: e-resize;
I don't have an OS with a separate e-resize cursor... DOM Inspector says this works, I guess that'll have to do.

>-toolbarbutton:focus,
>+toolbarbutton:-moz-focusring,
I couldn't find out where this made a difference. (Maybe because the toolbarbutton I picked was a menubutton?)

>-.focusring:focus > .tree-stack > .tree-rows > .tree-bodybox {
>+.focusring:-moz-focusring > .tree-stack > .tree-rows > .tree-bodybox {
Again, I don't see any difference here.

[Is it me or has the message pane lost its focusring?]

Comment 3

[[e-mail]]

(In reply to comment #2)
> [Is it me or has the message pane lost its focusring?]
[It has, and I filed bug 652470]

Comment 4

[Philip Chee]

Attachment: Patch v1.1 fix nits.

> [e-mail]      2011-04-23 09:26:02 PDT
 
> >+.alertBox[orient="vertical"] > .alertTextBox {
> >+  margin: 0 4px 6px;
> >+  -moz-box-align: center;
> Nit: include the comment about bug 311557 perhaps?
Added comment. I thought I didn't have to include it since bug 311557 is now fixed.

> >-toolbarbutton:focus,
> >+toolbarbutton:-moz-focusring,
> I couldn't find out where this made a difference. (Maybe because the
> toolbarbutton I picked was a menubutton?)
Reverted change.

> >-.focusring:focus > .tree-stack > .tree-rows > .tree-bodybox {
> >+.focusring:-moz-focusring > .tree-stack > .tree-rows > .tree-bodybox {
> Again, I don't see any difference here.
Reverted change.

> [Is it me or has the message pane lost its focusring?]
What are focus rings anyway?

Comment 5

[[e-mail]]

(In reply to comment #4)
> (From update of attachment 528074 [details] [diff] [review])
> > >+.alertBox[orient="vertical"] > .alertTextBox {
> > >+  margin: 0 4px 6px;
> > >+  -moz-box-align: center;
> > Nit: include the comment about bug 311557 perhaps?
> I thought I didn't have to include it since bug 311557 is now fixed.
Ah, I hadn't checked that far, I just looked in winstripe's alert.css; feel free to remove the comment, and ideally file a bug on fixing toolkit's themes and alert.js too.

> > [Is it me or has the message pane lost its focusring?]
> What are focus rings anyway?
It's a border around the message pane that appears when the message (or any element in the message) has focus. Try pressing F6 the next time you have a 3-pane messenger window open.

Comment 6

[[e-mail]]

Comment on attachment 528074 [details] [diff] [review]
Patch v1.1 fix nits.

Replying to a comment doesn't update the attachment. Oops ;-)

> .toolbarbutton-icon {
>+  list-style-image: inherit;
I forgot to mention that this is probably unnecessary, and you might as well remove it as you're touching it anyway.

Comment 7

[Philip Chee]

Attachment: Patch v1.2 more flyspecking. r=Neil

> [e-mail]      2011-04-25 04:18:45 PDT

>>>>+  -moz-box-align: center;
>>> Nit: include the comment about bug 311557 perhaps?
>> I thought I didn't have to include it since bug 311557 is now fixed.
> Ah, I hadn't checked that far, I just looked in winstripe's alert.css; feel
> free to remove the comment,
Removed.

> and ideally file a bug on fixing toolkit's themes and alert.js too.
Filed Bug 652536.

>> .toolbarbutton-icon {
>>+  list-style-image: inherit;
> I forgot to mention that this is probably unnecessary, and you might as well
> remove it as you're touching it anyway.
Fixed.
I just noticed that the XBL does this:
<xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>

Comment 8

[Philip Chee]

Comment on attachment 528105 [details] [diff] [review]
Patch v1.2 more flyspecking. r=Neil

Hadn't noticed that I have r+ already.
Carrying forward.

Comment 9

[Philip Chee]

Pushed:
http://hg.mozilla.org/releases/comm-2.0/rev/baf3507d8a4c
http://hg.mozilla.org/comm-central/rev/fa4ea41c266d

Comment 10

[[e-mail]]

(In reply to comment #7)
> I just noticed that the XBL does this:
> <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>
That's so that <toolbarbutton image="about:logo"/> (or whatever) works; if you set the image using list-style-image somewhere in CSS that already works because it's an inherited property.