Add styling for .menulist-menupopup and .menulist-compact removed by bug 1030644

RESOLVED FIXED in Thunderbird 36.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

({dogfood, regression})

unspecified
Thunderbird 36.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Thunderbird Tracking Flags

(thunderbird35 affected, thunderbird36 fixed, thunderbird38 fixed, seamonkey2.32 fixed, seamonkey2.33 fixed)

Details

Attachments

(3 attachments, 18 obsolete attachments)

6.84 KB, patch
stefanh
: review+
Details | Diff | Splinter Review
8.67 KB, patch
jsbruner
: review+
Details | Diff | Splinter Review
963 bytes, patch
jsbruner
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
m-c doesn't want to backout this change. We need now to implement this for TB and I'm looking if I can do the same for SM in this bug.

Opened in TB because mailnews has no theme component.
I mentioned this in dev.apps.thunderbird, but we should probably start our own "toolkit" that's shared between apps like TB, SM, and IB. That way we can reduce code duplication and that will make it easier to fix bugs later.

Comment 2

5 years ago
Questions:
1. How do we handle extensions that import chrome://global/skin/menulist.css
e.g. http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/resources/content/tests/allskin.xul#45

2. How do we handle XBL widgets that import menulist.css menu.css popup.css etc
e.g. http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/menulist.xml#16
(In reply to Philip Chee from comment #2)
> Questions:
> 1. How do we handle extensions that import chrome://global/skin/menulist.css
> e.g.
> http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/
> resources/content/tests/allskin.xul#45

I don't think I understand this questions. Inspector doesn't seem to be using menulist-popup nor menulist-compact...?

> 2. How do we handle XBL widgets that import menulist.css menu.css popup.css
> etc
> e.g.
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/
> menulist.xml#16

Ditto.
Assignee

Comment 4

5 years ago
Posted patch Bug1066551.patch (obsolete) β€” β€” Splinter Review
This patch adds the rules to messenger.css of mail and suite.

Neil, classic's messenger.css is used for all platforms. I've added the different platform dependent rules in ifdefs. I haven't tested the SM part but it should work. SM doesn't use the special rules TB has for the aw-menulist.

If you like I can change/simplify the rules nesting for easier reading like:

%ifdef XP_UNIX
%ifdef XP_MACOSX
  all OS X rules
%else
  all Linux rules
%endif
%else
  all Windows rules
%endif
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8488818 - Flags: review?(neil)
Attachment #8488818 - Flags: review?(josiah)
Assignee

Comment 5

5 years ago
Posted patch Bug1066551.patch (obsolete) β€” β€” Splinter Review
Bah, found some trailing whitespaces.
Attachment #8488818 - Attachment is obsolete: true
Attachment #8488818 - Flags: review?(neil)
Attachment #8488818 - Flags: review?(josiah)
Attachment #8488820 - Flags: review?(neil)
Attachment #8488820 - Flags: review?(josiah)
Comment on attachment 8488820 [details] [diff] [review]
Bug1066551.patch

That's strange, I thought the CSS styles for compact menulists on Linux were very similar to those on Windows, yet you've got some rather large ifdefs (which I'd like to avoid altogether, if possible, or at least move to a separate file; on the Mac we already fork the CSS, but you'll need review from :stefanh anyway).

What benefit do we get from the menulist-menupopup style rules?

When will toolkit decide that the menulist-compact binding is also a left-over remnant?
(In reply to neil@parkwaycc.co.uk from comment #6)
> When will toolkit decide that the menulist-compact binding is also a
> left-over remnant?

I was in fact about to file that bug...
Assignee

Comment 8

5 years ago
Posted patch cc/mail patch (obsolete) β€” β€” Splinter Review
Splitting the patches into mail and suite parts.
Attachment #8488820 - Attachment is obsolete: true
Attachment #8488820 - Flags: review?(neil)
Attachment #8488820 - Flags: review?(josiah)
Attachment #8488873 - Flags: review?(josiah)
Assignee

Comment 9

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Now with three messenger files.

Neil, is this what you thought? The menulist-menupopup rules are used for the folder selection in menulists like in the filters.
Attachment #8488874 - Flags: review?(stefanh)
Attachment #8488874 - Flags: feedback?(neil)
Comment on attachment 8488874 [details] [diff] [review]
c-c/suite patch

Thanks for doing this Richard. I have only looked at and tested the mac part, so Neil might want to take a look at the win/nix part.

+
+.menulist-compact {
+  -moz-binding: url("chrome://global/skin/globalBindings.xml#menulist-compact");

Mac uses the binding in global/content:  (”-moz-binding: url("chrome://global/content/bindings/menulist.xml#menulist-compact”);”). Iotw, you should change this in the mail/ patch too.

The rest is solely about removal. In suite/ we rely more on the native styling, but some of these comments could be valid for mail/ too.

+}
+
+/* ::::: menu/menuitems in menulist popups ::::: */
+
+.menulist-menupopup > menuitem,
+.menulist-menupopup > menu {
+  max-width: none;
+  font: inherit;
+  color: -moz-FieldText;

Please remove the last 2 lines (we get font inheritance from menu.css and the color rule makes the label text not change to white upon selection/hover).

}
+
+/* ::::: compact menulists ::::: */
+
+.menulist-compact {
+  -moz-box-align: center;
+  -moz-box-pack: center;
+  margin: 0;
+  border: 2px solid;
+  -moz-border-top-colors: ThreeDLightShadow ThreeDHighlight;
+  -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
+  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
+  -moz-border-left-colors: ThreeDLightShadow ThreeDHighlight;
+  background-color: -moz-Dialog;
+}

Please remove the last 6 rules (border, bg-color). These rules have no effect because menu lists are native styled (in menulist.css).

+
+.menulist-compact > .menulist-label {
+  margin: 0 3px !important;

And the margin rule here can also go.

+
+.menulist-compact > .menulist-dropmarker {
+  -moz-margin-start: 2px;
+  border: none;
+  padding: 0 !important;
+  background: transparent;
+}

The .menulist-dropmarker has ”display: none;” in menulist.css. On mac, it’s the ”-moz-appearance: menulist;” rule that takes care of the whole thing instead. So we don’t need any of the above rules.

+
+.menulist-compact[open="true"] {
+  -moz-border-top-colors: ThreeDDarkShadow ThreeDShadow;
+  -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
+  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
+  -moz-border-left-colors: ThreeDDarkShadow ThreeDShadow;
+  background-color: ThreeDShadow;
+}
+

All these rules can also go :-)

r+ with all the above changes, thanks.
Attachment #8488874 - Flags: review?(stefanh) → review+
Assignee

Comment 11

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Fixed the comments and removed also unneeded rules in Linux and Windows files. There's still more in them because both are using -moz-appearance: none.

Neil, please can check on Linux and Windows if all is correct? The mail part, where I checked, is exact the same.
Attachment #8488874 - Attachment is obsolete: true
Attachment #8488874 - Flags: feedback?(neil)
Attachment #8489015 - Flags: review+
Attachment #8489015 - Flags: feedback?(neil)
Assignee

Comment 12

5 years ago
Posted patch c-c/mail patch (obsolete) β€” β€” Splinter Review
Made the same changes as in suite patch proposed by stefanh.
Attachment #8488873 - Attachment is obsolete: true
Attachment #8488873 - Flags: review?(josiah)
Attachment #8489017 - Flags: review?(josiah)
(In reply to Richard Marti from comment #9)
> The menulist-menupopup rules are used for the folder selection in menulists like in the filters.

Indeed, but are they actually useful? I think the folder selection menulists look fine without them and it would reduce our maintenance burden if we could remove them. And I already found one menulist that forgot them - the IMAP Trash folder menulist.
Assignee

Comment 14

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #13)
> (In reply to Richard Marti from comment #9)
> > The menulist-menupopup rules are used for the folder selection in menulists like in the filters.
> 
> Indeed, but are they actually useful?

I checked again and the menulist-menupopup rules could be removed. Maybe a follow-up bug removing this id's should be filed.

But with the menulist-compact you're okay to add them? I can't say how it looks on SM but for TB they are needed for the aw-menulist in composer.
I guess we could put the menulist-compact rules in messengercompose if we only want to use them for the aw-menulist.
Assignee

Comment 16

5 years ago
I can apply the menulist-compact rules directly to aw-menulist. This is what I thought to do in the mail patch because this is the only consumer of the menulist-compact rules.

Comment 17

5 years ago
I remember adding those classes to the pickers in TB in the past, e.g. in bug 808974. The bugs mean the classes were useful and had an effect at that time (otherwise various pickers looked differently). Also see bug 814041.
Maybe this was somehow changed in bug 878805, but I do not see css changes in there.
Maybe today all the needed style is attached to the folderMenuItem class.
Assignee

Comment 18

5 years ago
Posted patch c-c/mail patch (obsolete) β€” β€” Splinter Review
The menulist-menupopup rules removed and the other rules directly to aw-menulist applied.
Attachment #8489017 - Attachment is obsolete: true
Attachment #8489017 - Flags: review?(josiah)
Attachment #8489358 - Flags: review?(josiah)
Assignee

Comment 19

5 years ago
Posted patch cc/suite patch (obsolete) β€” β€” Splinter Review
The menulist-menupopup rules removed and the other rules directly to messengercompose.css applied.
Attachment #8489015 - Attachment is obsolete: true
Attachment #8489015 - Flags: feedback?(neil)
Attachment #8489436 - Flags: review?(stefanh)
Please also implement the menulist-compact binding on comm-central rather than using the obsolete toolkit ones.
Assignee

Comment 21

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Added the binding in mailnews.
Attachment #8489436 - Attachment is obsolete: true
Attachment #8489436 - Flags: review?(stefanh)
Attachment #8489591 - Flags: review?(neil)
Assignee

Comment 22

5 years ago
Posted patch c-c/mail patch (obsolete) β€” β€” Splinter Review
Uses the binding from suite patch -> needs it also applied to work.
Attachment #8489358 - Attachment is obsolete: true
Attachment #8489358 - Flags: review?(josiah)
Attachment #8489592 - Flags: review?(josiah)

Updated

5 years ago
Attachment #8489591 - Flags: review?(stefanh)
Comment on attachment 8489591 [details] [diff] [review]
c-c/suite patch

The Linux and Windows CSS is very similar. The only differences are:

>diff --git a/suite/themes/classic/messenger/messengercompose/messengercompose.css b/suite/themes/classic/linux/messenger/messengercompose/messengercompose.css
>+/* ::::: compact menulists ::::: */
>+
>+.menulist-compact {
>+  -moz-binding: url("chrome://messenger/content/messengercompose/menulistCompactBindings.xml#menulist-compact");
>+  -moz-appearance: none;
>+  -moz-box-align: center;
>+  -moz-box-pack: center;
>+  margin: 0;
>+  border: 2px solid;
The Windows CSS doesn't have this border, because it's already the default for menulists on Windows. (But it wouldn't hurt to have it.)

>+.menulist-compact > .menulist-label-box,
>+.menulist-compact[open="true"]:focus > .menulist-label-box {
>+  border: 1px solid transparent;
>+  -moz-appearance: none;
>+}
>+
>+.menulist-compact:focus > .menulist-label-box {
>+  border: 1px dotted;
>+}
...
>+.menulist-compact:-moz-focusring:not([open="true"]) > .menulist-label-box {
>+  border: 1px dotted;
>+}
There are some minor differences here.
The Linux native theme code normally draws the focus, but for compact menulists, we want to specify the border explicitly. This means we have to add extra rules for the borderless and bordered cases. (By comparison the Windows code already reserves the border, and we only have to override the colour.)
The Linux code uses :focus while the Windows code uses :-moz-focusring. I'll have to look to see what the difference in behaviour is there (I wasn't able to spot the difference on Windows XP though).

So we could probably get away with using the Linux styles on Windows too.
You should generally just use :-moz-focusring for drawing focus rings. It should give you the right behavior on all platforms. See bug 418521.
Assignee

Comment 25

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Now Linux and Windows are using again the same messengercompose.css. I'm using :-moz-focusring instead of :focus.
Attachment #8489591 - Attachment is obsolete: true
Attachment #8489591 - Flags: review?(stefanh)
Attachment #8489591 - Flags: review?(neil)
Attachment #8490195 - Flags: review?(stefanh)
Attachment #8490195 - Flags: review?(neil)
Assignee

Comment 26

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Found a whitespace in imported binding. Sorry for the spam.
Attachment #8490195 - Attachment is obsolete: true
Attachment #8490195 - Flags: review?(stefanh)
Attachment #8490195 - Flags: review?(neil)
Attachment #8490201 - Flags: review?(stefanh)
Attachment #8490201 - Flags: review?(neil)
Assignee

Comment 27

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Oh no, forgot to add the menulist-compact 2px border.
Attachment #8490201 - Attachment is obsolete: true
Attachment #8490201 - Flags: review?(stefanh)
Attachment #8490201 - Flags: review?(neil)
Attachment #8490222 - Flags: review?(stefanh)
Attachment #8490222 - Flags: review?(neil)
Comment on attachment 8490222 [details] [diff] [review]
c-c/suite patch

>+.menulist-compact > .menulist-label-box,
>+.menulist-compact[open="true"]:focus > .menulist-label-box {
>+  border: 1px solid transparent;
>+  -moz-appearance: none;
>+}
>+
>+.menulist-compact:-moz-focusring:not([open="true"]) > .menulist-label-box {
>+  border: 1px dotted;
>+}
Since you're using :not here, you don't need the [open="true"]:focus rule in the block above, whose job it was to override the :focus rule.

With the 2px border, this now works fine on Linux. However Windows XP is slightly trickier, as it has a rule to specify a background and text colour for the menulist-label-box which you need to override back to transparent and inherit. The problem does not apply on Windows 7 because you start using the Aero styles which also override the background and text (using !important because they don't have enough specificity) for the menulist-label-box.

So just adding a couple of extra rules background: transparent; and color: inherit; to the .menulist-compact > .menulist-label-box declaration fixes that.

r=me on the Windows/Linux changes with that fixed.

(menulistCompactBindings seems such a long name for the XBL file, but I was never any good at naming. Have you checked with a Thunderbird peer?)
Attachment #8490222 - Flags: review?(neil) → review+
Assignee

Comment 29

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Patch with Neil's comments addressed.
Attachment #8490222 - Attachment is obsolete: true
Attachment #8490222 - Flags: review?(stefanh)
Attachment #8490652 - Flags: review?(stefanh)
Assignee

Comment 30

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #28)
> Comment on attachment 8490222 [details] [diff] [review]
> c-c/suite patch
> 
> (menulistCompactBindings seems such a long name for the XBL file, but I was
> never any good at naming. Have you checked with a Thunderbird peer?)

Yeah, it's a long name together with the path but I have also no shorter name which still describes the function.
Comment on attachment 8490652 [details] [diff] [review]
c-c/suite patch

Richard, I will test this tomorrow or on friday. But when I look at the code again, I think mac might work fine on both thunderbird and seamonkey with the same binding as linux/win. I haven't tested it, but in toolkit/themes/osx/global/menulist.css we have this rule:

35 .menulist-dropmarker {
36   display: none;
37 }

Looking in mail/themes/osx, I don't see anything that overrides the "display: none;" rule. The only "diplay: -moz-box;" rule I can find is the one in osx/mail/editContactOverlay.css#132, but that menulist doesn't use our binding.
Assignee

Comment 32

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
Okay, all platforms are now using the same binding. Is it okay when only one binding exists only link to the whole binding without the binding id? I removed the #menulist-compact to make the line shorter and I get no error message in console.

The only change in Linux/Windows file is the binding shortening.
Attachment #8490652 - Attachment is obsolete: true
Attachment #8490652 - Flags: review?(stefanh)
Attachment #8490898 - Flags: review?(stefanh)
Assignee

Comment 33

5 years ago
Posted patch c-c/mail patch (obsolete) β€” β€” Splinter Review
Using now also only one binding for all platforms. This patch needs also the other applied to work correctly.
Attachment #8489592 - Attachment is obsolete: true
Attachment #8489592 - Flags: review?(josiah)
Attachment #8490901 - Flags: review?(josiah)
(In reply to Richard Marti from comment #32)
> Okay, all platforms are now using the same binding. Is it okay when only one
> binding exists only link to the whole binding without the binding id?
FYI My personal preference would be to retain the binding id in case things need to be moved around for some reason.
Assignee

Comment 35

5 years ago
Readded the binding id.
Attachment #8490898 - Attachment is obsolete: true
Attachment #8490898 - Flags: review?(stefanh)
Attachment #8490979 - Flags: review?(stefanh)
Assignee

Comment 36

5 years ago
Posted patch c-c/mail patch β€” β€” Splinter Review
Also readded the binding id.
Attachment #8490901 - Attachment is obsolete: true
Attachment #8490901 - Flags: review?(josiah)
Attachment #8490980 - Flags: review?(josiah)
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

Sorry, but I now see that I was wrong about the font inheritance in comment #10. Turns out that our cases here (menu items in view picker sub-menus, for example) isn't covered, so you'll need to fork messenger.css for mac and add these lines:
--------------------------------
.menulist-menupopup > menuitem,
.menulist-menupopup > menu {
font: inherit;
}
--------------------------------

Fwiw, Thunderbird have the same problem of course.
Neil tells me that win/nix also needs the above so you don't need to fork the suite version of messenger css - you can just add the above lines to the existing messenger.css.
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

r=me if you add the following to messenger.css (no need to fork it):

.menulist-menupopup > menuitem,
.menulist-menupopup > menu {
font: inherit;
}
Attachment #8490979 - Flags: review?(stefanh) → review+
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

# Parent  b39c4375a50940b8d9cef94c856b01169c4e380b
Bug 1066551 - Bring the removed .menulist-menupopup and .menulist-compact styles back in c-c/suite. r=neil/stephanh

"stefanh" :-)
Assignee

Comment 41

5 years ago
Posted patch c-c/suite patch (obsolete) β€” β€” Splinter Review
(In reply to Stefan [:stefanh] from comment #39)
> Comment on attachment 8490979 [details] [diff] [review]
> c-c/suite patch
> 
> r=me if you add the following to messenger.css (no need to fork it):
> 
> .menulist-menupopup > menuitem,
> .menulist-menupopup > menu {
> font: inherit;
> }

Done.

(In reply to Stefan [:stefanh] from comment #40)
> Comment on attachment 8490979 [details] [diff] [review]
> c-c/suite patch
> 
> # Parent  b39c4375a50940b8d9cef94c856b01169c4e380b
> Bug 1066551 - Bring the removed .menulist-menupopup and .menulist-compact
> styles back in c-c/suite. r=neil/stephanh
> 
> "stefanh" :-)

Sorry, have to much Stephans around me. :-(
Attachment #8490979 - Attachment is obsolete: true
Attachment #8492036 - Flags: review+

Comment 42

5 years ago
A very minor question. Bug 1068188 removed two compact menulist bindings.
> toolkit/content/widgets/menulist.xml
> toolkit/themes/windows/global/globalBindings.xml

You've copied the windows skin version to content/menulistCompactBindings.xml

I don't know what's the difference if any this makes.
Flags: needinfo?(neil)
(In reply to Philip Chee from comment #42)
> A very minor question. Bug 1068188 removed two compact menulist bindings.
> > toolkit/content/widgets/menulist.xml
> > toolkit/themes/windows/global/globalBindings.xml
> 
> You've copied the windows skin version to content/menulistCompactBindings.xml
> 
> I don't know what's the difference if any this makes.

I couldn't see any difference between screenshots of beta and this patch. (I couldn't screenshot aurora because 1050449 has regressed our autocomplete widget.)
Flags: needinfo?(neil)
Assignee

Comment 44

5 years ago
(In reply to Philip Chee from comment #42)
> A very minor question. Bug 1068188 removed two compact menulist bindings.
> > toolkit/content/widgets/menulist.xml
> > toolkit/themes/windows/global/globalBindings.xml
> 
> You've copied the windows skin version to content/menulistCompactBindings.xml
> 
> I don't know what's the difference if any this makes.

Linux has no globalBindings.xml in it's tree, so I suppose Linux copies it from windows tree. The difference to mac is the additional .menulist-label-box. Because of this I added

+.menulist-compact  > .menulist-label-box > .menulist-label {
+  text-align: end;
+}

to the mac file.

Comment 45

5 years ago
> I couldn't see any difference between screenshots of beta and this patch. (I
> couldn't screenshot aurora because 1050449 has regressed our autocomplete
> widget.)

The win global binding was introduced in:
Bug 398874 - addressingwidget appears as sunken, not as button
http://hg.mozilla.org/releases/mozilla-aurora/rev/97a3f175e540
The author of which is neil@parkwaycc.co.uk (I think this is you)

Which is a copy of XPFE Bug 184811 - addressingwidget appears as sunken, not as button

Richard: The screenshots in Bug 184811 might be a useful reference point.

Reading the comments it seems windows/global/globalBindings.xml is in the windows theme to avoid regressing OS X
(In reply to Philip Chee from comment #45)
> Reading the comments it seems windows/global/globalBindings.xml is in the
> windows theme to avoid regressing OS X

See my comment #31. I don't see any issue on Mac using the same binding as win/nix. The mac binding has a dropmarker, but that is hidden anyway.
Assignee

Updated

5 years ago
Blocks: 1073968

Comment 47

5 years ago
(In reply to Richard Marti (:Paenglab) from comment #41)
> Created attachment 8492036 [details] [diff] [review]
> c-c/suite patch

Apparently nobody from the SM side noticed that this is patching classic only but not modern:

>+++ b/suite/themes/classic/mac/messenger/messengercompose/messengercompose.css
>+++ b/suite/themes/classic/messenger/messenger.css
>+++ b/suite/themes/classic/messenger/messengercompose/messengercompose.css

Thus, to fix bug 1073968, changes for suite/themes/classic/messenger would also have to be included for suite/themes/modern/messenger (preferably here or in the other bug).
(In reply to rsx11m from comment #47)
> Apparently nobody from the SM side noticed that this is patching classic
> only but not modern:

That's because Modern was broken by 1068188, not bug 1030644.
(In reply to rsx11m from comment #47)
> Thus, to fix bug 1073968, changes for suite/themes/classic/messenger would
> also have to be included for suite/themes/modern/messenger (preferably here
> or in the other bug).

Or as you pointed out on IRC, the -moz-binding could be moved to suite/mailnews/messenger.css where all themes would pick it up automatically.

Comment 50

5 years ago
Comment on attachment 8492036 [details] [diff] [review]
c-c/suite patch

Yes, that would be the correct solution rather than forcing each theme to include the binding (the same may apply to the Thunderbird version of this patch).
Attachment #8492036 - Flags: feedback-
Assignee

Comment 51

5 years ago
Posted patch Bug1066551-suite.patch (obsolete) β€” β€” Splinter Review
Moved the binding to suite/mailnews/messenger.css.
Attachment #8492036 - Attachment is obsolete: true
Attachment #8497600 - Flags: review?(neil)
Comment #39 appears to have gone missing from the latest patch?
Comment on attachment 8497600 [details] [diff] [review]
Bug1066551-suite.patch

Actually this doesn't help Modern because it really needs the other binding.
Attachment #8497600 - Flags: review?(neil)
Attachment #8497600 - Attachment is obsolete: true
Attachment #8490979 - Attachment is obsolete: false
No longer blocks: 1073968
This will now need to be uplifted to Aurora.
Comment on attachment 8490980 [details] [diff] [review]
c-c/mail patch

Review of attachment 8490980 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8490980 - Flags: review?(josiah) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2e0cf60b9bef
https://hg.mozilla.org/comm-central/rev/17bbeb0efe16
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0

Comment 57

5 years ago
Instead of bringing back menulist-compact with a specific bindings, etc,
one could also have seen that it is only used in messengercompose.xul,
and use some specific styling in messengercompose.css to make the normal menulist "compact"?
Doesn't help I'm afraid, we need to override sizetopopup anyway.
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

We need to uplift this for SeaMonkey 2.32 please.

[Approval Request Comment]
Regression caused by (bug #): 1030644
User impact if declined: Can't compose mail in SeaMonkey 2.32
Testing completed (on c-c, etc.): Fixed in SeaMonkey 2.33
Risk to taking this patch (and alternatives if risky): None
Attachment #8490979 - Flags: approval-comm-aurora?

Comment 60

5 years ago
Moving to MailNews Core so that SeaMonkey tracking flags are available
Component: Theme → Composition
Product: Thunderbird → MailNews Core

Updated

5 years ago
Component: Composition → Themes
Product: MailNews Core → SeaMonkey
Target Milestone: Thunderbird 36.0 → ---

Comment 61

5 years ago
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

a=me for SeaMonkey comm-aurora
Attachment #8490979 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

5 years ago
Component: Themes → Composition
Flags: approval-comm-aurora+
Product: SeaMonkey → MailNews Core
Target Milestone: --- → Thunderbird 36.0

Comment 62

5 years ago
Your "Flags: approval-comm-aurora+" didn't stick.  :-(

Updated

5 years ago
Attachment #8490979 - Flags: approval-comm-aurora?

Comment 63

5 years ago
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

Grrr Manual a+ for comm-aurora.
Attachment #8490979 - Attachment description: c-c/suite patch → c-c/suite patch a=Ratty for comm-aurora

Comment 64

5 years ago
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

After last week's merge this will now have to land on comm-beta instead.
Does the a+ carry forward in this case?
Attachment #8490979 - Flags: approval-comm-aurora? → approval-comm-beta?

Comment 65

5 years ago
Comment on attachment 8490979 [details] [diff] [review]
c-c/suite patch a=Ratty for comm-aurora

a=Ratty for comm-beta

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [c-n: push #8490979 to comm-beta per comment #65]

Comment 66

5 years ago
Pushed to comm-beta
http://hg.mozilla.org/releases/comm-beta/rev/1a6452fefaa8
Sorry Missed 2.31b1 but should be in 2.31b2
Keywords: checkin-needed
Whiteboard: [c-n: push #8490979 to comm-beta per comment #65]
Attachment #8490979 - Flags: approval-comm-beta?
Comment on attachment 8490980 [details] [diff] [review]
c-c/mail patch

>+.aw-menulist [disabled="true"] {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.gif");
>+}

This selector looks broken...

Comment 68

5 years ago
Yeah, a surplus space?
Assignee

Comment 69

5 years ago
Fix of the wrong space.
Attachment #8546566 - Flags: review?(josiah)
Assignee

Comment 70

5 years ago
(In reply to DΓ£o Gottwald [:dao] from comment #67)
> Comment on attachment 8490980 [details] [diff] [review]
> c-c/mail patch
> 
> >+.aw-menulist [disabled="true"] {
> >+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.gif");
> >+}
> 
> This selector looks broken...

DΓ£o, thank you for pointing to this.
Comment on attachment 8546566 [details] [diff] [review]
Bug1066551SpaceFix.patch

Review of attachment 8546566 [details] [diff] [review]:
-----------------------------------------------------------------

:)
Attachment #8546566 - Flags: review?(josiah) → review+
Assignee

Updated

4 years ago
Flags: in-testsuite-
Assignee

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: Check-in Bug1066551SpaceFix.patch
Assignee

Comment 72

4 years ago
Comment on attachment 8546566 [details] [diff] [review]
Bug1066551SpaceFix.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1066551
User impact if declined: not showing correct menulist arrow.
Testing completed (on c-c, etc.): simple typo fix
Risk to taking this patch (and alternatives if risky): low, only typo fix
Attachment #8546566 - Flags: approval-comm-aurora?
It is really difficult to track followup changes that are done after several uplifts, so the followup should really have been a new patch.
Keywords: checkin-needed
Whiteboard: Check-in Bug1066551SpaceFix.patch

Updated

4 years ago
Attachment #8546566 - Flags: approval-comm-aurora? → approval-comm-aurora+

Comment 75

4 years ago
(In reply to Kent James (:rkent) from comment #74)
> It is really difficult to track followup changes that are done after several
> uplifts, so the followup should really have been a new patch.

...new bug?

Comment 76

4 years ago
Three months after the original patch wrapped up, sounds more like filing a new bug for it and making it block the original one, yes.
Yes I meant new bug, sorry.
You need to log in before you can comment on or make changes to this bug.