Bug 546857 killed content XUL, we need to convert at least our feed preview and cert error pages as done there for FF so that they still work.

This should do it, I have tested very sparsely so far.

Do a bit more changes to satisfy certError needs

Comment on attachment 469330 [details] [diff] [review] some missed changes. >diff --git a/suite/common/certError.xhtml b/suite/common/certError.xhtml I don't like what Firefox have done for their certError changes, so I'll try to write my own version based on feeds. (And you missed some stuff anyway.) >+#feedSubscribeLine { >+ -moz-binding: url(subscribe.xml#feedreaderUI); Nit: prefer full chrome: URL > <link rel="stylesheet" > href="chrome://communicator/skin/feed-subscribe.css" > type="text/css" > media="all"/> >+ <link rel="stylesheet" >+ href="chrome://communicator/content/feeds/subscribe.css" >+ type="text/css" >+ media="all"/> Nit: content CSS first please >+<?xml version="1.0" encoding="utf-8"?> [I didn't think encoding was needed, utf-8 should be the default] >+ <!ENTITY % globalDTD >+ SYSTEM "chrome://global/locale/global.dtd"> >+ %globalDTD; Not used. (So you can use the DOCTYPE shorthand.) > content/communicator/feeds/subscribe.xhtml (feeds/subscribe.xhtml) >+ content/communicator/feeds/subscribe.xml (feeds/subscribe.xml) > content/communicator/feeds/subscribe.js (feeds/subscribe.js) >+ content/communicator/feeds/subscribe.css (feeds/subscribe.css) Ideally this would be in alphabetical order... >+ _handlersMenuList: null, [I wonder why this one was cached. I would have cached feedSubscribeLine.] >+ this._document.getElementById("feedSubscribeLine").offsetTop; offsetTop is deprecated ;-) Please use getBoundingClientRect() instead. >+.alwaysUse { >+ padding: 5px; This wasn't in our previous CSS, and doesn't belong here either. >+.handlersMenuList > menupopup > menuitem { >+ -moz-padding-start: 23px; No point, since all of our menuitems are iconic, aren't they? >+.handlersMenuList > menupopup > menuitem.menuitem-iconic { You can abbreviate this to .menuitem-iconic since nothing else in the binding will have that class. >+ -moz-padding-start: 2px; Nit: I prefer 4px, but I only tried this on Windows. >+.handlersMenuList > menupopup > .menuitem-iconic > .menu-iconic-left { And this can be abbreviated to .menu-iconic-left for the same reason. >+ min-width: 16px; .menu-iconic-icon has a width of 16px so we probably don't need this. >+ -moz-padding-end: 2px; Nit: I prefer 3px, but again, only tested on Windows. >+.messengerFeedsMenuItem { >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); KaiRo made this obsolete when he landed Places, but forgot to remove it. Boo! >+menupopup:-moz-locale-dir("rtl") { No quotes around rtl. >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css This is almost all completely wrong, except for the rtl, which is only partially wrong.

Actually there's one thing that it would be nice to add to the CSS - currently the font for the XUL bit is wrong (particularly noticable with the menulist) so it would be handy if you could set the vbox font to message-box (both themes).

(In reply to comment #4) > >diff --git a/suite/common/certError.xhtml b/suite/common/certError.xhtml > I don't like what Firefox have done for their certError changes, so I'll try to > write my own version based on feeds. (And you missed some stuff anyway.) Well, a fix should land the days before yesterday, but that said, could you, in this case, please backport all your changes to our side to theirs? Porting adaptations from them is already hard enough with how we are diverging, and I still think it's wrong that this even lives in app-specific code anyhow.

I'd prefer xbl:inherits="id" but I thought that would get r- ;-)

Comment on attachment 469596 [details] [diff] [review] certError fix [Checked in: Comment 11] >diff --git a/suite/common/certError.css b/suite/common/certError.css >new file mode 100644 >--- /dev/null >+++ b/suite/common/certError.css >@@ -0,0 +1,3 @@ >+span { >+ -moz-binding: url("chrome://communicator/content/certError.xml#button"); >+} nit: #getMeOutOfHereButton So we don't get any weird surprises later by someone adding a span in here.

(In reply to comment #4) > Comment on attachment 469330 [details] [diff] [review] > some missed changes. > > >+<?xml version="1.0" encoding="utf-8"?> > [I didn't think encoding was needed, utf-8 should be the default] I assume you wanted me to drop encoding="..." > Not used. (So you can use the DOCTYPE shorthand.) No idea what this "shorthand" is, but I dropped the unused part. > >+ this._document.getElementById("feedSubscribeLine").offsetTop; > offsetTop is deprecated ;-) Please use getBoundingClientRect() instead. Of course, offsetTop and getBoundingClientRect have very different semantics, and offsetTop (per MDC) does not say deprecated. I did not change this. > >+.messengerFeedsMenuItem { > >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > KaiRo made this obsolete when he landed Places, but forgot to remove it. Boo! Not sure what you were asking me to do here, but I think you meant to remove it from the css. > >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css > This is almost all completely wrong, except for the rtl, which is only > partially wrong. Not sure what you were asking me to do here either, so I did the same changes as the classic .css Hope this is satisfactory. I'm unsure if the tabbrowser.xml and the utilityOverlay.js changes are still needed/correct with the other patch by you.

Comment on attachment 469596 [details] [diff] [review] certError fix [Checked in: Comment 11] http://hg.mozilla.org/comm-central/rev/b9b8db196a54

Err, 2.0 Branch?!

(In reply to comment #9) >(In reply to comment #4) >>(From update of attachment 469330 [details] [diff] [review]) >>>+<?xml version="1.0" encoding="utf-8"?> >>[I didn't think encoding was needed, utf-8 should be the default] >I assume you wanted me to drop encoding="..." Actually I ended up confusing you. We seem to assume utf-8 for XUL and XBL files, but not for xhtml files, so that change wasn't needed. >> Not used. (So you can use the DOCTYPE shorthand.) > No idea what this "shorthand" is, but I dropped the unused part. <!DOCTYPE <tagname> SYSTEM "chrome://<package>/locale/<file>"> > > >+ this._document.getElementById("feedSubscribeLine").offsetTop; > > offsetTop is deprecated ;-) Please use getBoundingClientRect() instead. > Of course, offsetTop and getBoundingClientRect have very different semantics Neither of which we are using here; we're actually using them to flush layout. > and offsetTop (per MDC) does not say deprecated. I did not change this. Hmm, well I'm sure getBoundingClientRect was added to replace something... > > >+.messengerFeedsMenuItem { > > >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > > KaiRo made this obsolete when he landed Places, but forgot to remove it. Boo! > Not sure what you were asking me to do here, but I think you meant to remove it > from the css. Correct! > > >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css > > This is almost all completely wrong, except for the rtl, which is only > > partially wrong. > Not sure what you were asking me to do here either, so I did the same changes > as the classic .css No, it nearly all has to go. > I'm unsure if the tabbrowser.xml and the utilityOverlay.js changes are still > needed/correct with the other patch by you. No, they are not. (Did you not test it?)

Comment on attachment 469738 [details] [diff] [review] address review comments. >+ <div id="feedSubscribeLine"></div> In addition to my replies to your comments, one thing I forgot to mention before is that this is an XHTML document, and therefore does not need to use HTML end tag syntax. >diff --git a/suite/themes/classic/communicator/feed-subscribe-ui.css b/suite/themes/classic/communicator/feed-subscribe-ui.css >new file mode 100644 >--- /dev/null >+++ b/suite/themes/classic/communicator/feed-subscribe-ui.css >@@ -0,0 +1,12 @@ >+.menuitem-iconic { >+ -moz-padding-start: 4px; >+} >+ >+.menu-iconic-left { >+ display: -moz-box; >+ -moz-padding-end: 3px; >+} [I'm rebuilding my Linux build for bug 591078, so I hope be able to test this on Linux tonight.] >diff --git a/suite/themes/modern/communicator/feed-subscribe-ui.css b/suite/themes/modern/communicator/feed-subscribe-ui.css >new file mode 100644 >--- /dev/null >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css >@@ -0,0 +1,12 @@ >+.menuitem-iconic { >+ -moz-padding-start: 4px; >+} >+ >+.menu-iconic-left { >+ display: -moz-box; >+ -moz-padding-end: 3px; >+} [So just to clarify, we don't need these two rules, just the third one.]

> Hmm, well I'm sure getBoundingClientRect was added to replace something... IIRC document.height/document.width

Comment on attachment 469738 [details] [diff] [review] address review comments. >+<?xml version="1.0"> Oops, missing a ? r- for this and the other comments above. >diff --git a/suite/themes/classic/communicator/feed-subscribe-ui.css b/suite/themes/classic/communicator/feed-subscribe-ui.css >new file mode 100644 >--- /dev/null >+++ b/suite/themes/classic/communicator/feed-subscribe-ui.css >@@ -0,0 +1,12 @@ >+.menuitem-iconic { >+ -moz-padding-start: 4px; >+} >+ >+.menu-iconic-left { >+ display: -moz-box; >+ -moz-padding-end: 3px; >+} So, these figures are too high for Linux, which wants them to be 1px :-( I guess the old figure of 2px was the best available compromise... Sorry to mess you around like that.

Ftr: http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey&maxdate=1282766141&hours=24&legend=0&norules=1 http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282727715.1282728818.12246.gz Linux comm-central-trunk debug test mochitests-5/5 on 2010/08/25 02:15:15 http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282737209.1282738153.1829.gz Linux comm-central-trunk debug test mochitests-5/5 on 2010/08/25 04:53:29 { 231 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_bug364677.html | Feed served as text/xml without a channel/link should have been sniffed - got undefined, expected "feedHandler" } http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a758e294ed9e&tochange=cec16a1741fb (Bug 546857 Part 6)

(In reply to comment #13) > (In reply to comment #9) > >(In reply to comment #4) > >>(From update of attachment 469330 [details] [diff] [review]) > >>>+<?xml version="1.0" encoding="utf-8"?> > >>[I didn't think encoding was needed, utf-8 should be the default] > >I assume you wanted me to drop encoding="..." > Actually I ended up confusing you. We seem to assume utf-8 for XUL and XBL > files, but not for xhtml files, so that change wasn't needed. Err ok, fixed. > >> Not used. (So you can use the DOCTYPE shorthand.) > > No idea what this "shorthand" is, but I dropped the unused part. > <!DOCTYPE <tagname> SYSTEM "chrome://<package>/locale/<file>"> Ahh yes, fixed. > > > >+ this._document.getElementById("feedSubscribeLine").offsetTop; > > > offsetTop is deprecated ;-) Please use getBoundingClientRect() instead. > > Of course, offsetTop and getBoundingClientRect have very different semantics > Neither of which we are using here; we're actually using them to flush layout. And per bz, offsetTop is not deprecated or going away, and one vs the other doesn't really matter for a layout-flush-use. So I'd rather stay compat with Firefox here. Did not change. [frankly, I'm unsure on why we even _need_ to flush here, but thats another bug/story entirely] > > I'm unsure if the tabbrowser.xml and the utilityOverlay.js changes are still > > needed/correct with the other patch by you. > No, they are not. (Did you not test it?) I hadn't at the time of posting, but I reverted them. (In reply to comment #14) > >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css > [So just to clarify, we don't need these two rules, just the third one.] Ok. (In reply to comment #16) > >+<?xml version="1.0"> > Oops, missing a ? Fixed > >diff --git a/suite/themes/classic/communicator/feed-subscribe-ui.css b/suite/themes/classic/communicator/feed-subscribe-ui.css > So, these figures are too high for Linux, which wants them to be 1px :-( > I guess the old figure of 2px was the best available compromise... > Sorry to mess you around like that. I understand, and fixed. Attaching patch in just a moment (about to test it with mochi 5)

(In reply to comment #18) > Attaching patch in just a moment (about to test it with mochi 5) And we no longer hit the failure relating to this bug with that suite.

Comment on attachment 471394 [details] [diff] [review] This should be it >diff --git a/suite/common/feeds/subscribe.css b/suite/common/feeds/subscribe.css >new file mode 100644 >--- /dev/null >+++ b/suite/common/feeds/subscribe.css >@@ -0,0 +1,3 @@ >+#feedSubscribeLine { >+ -moz-binding: url(chrome://communicator/content/feeds/subscribe.xml#feedreaderUI); >+} Sorry for not noticing before but all your new files have DOS line endings :-( >+ <binding id="feedreaderUI"> I suddenly realised how to get rid of that offsetTop hack. What we need to do is to add a <constructor> to this binding, and get that to init the UI.

Gavin, did you want the offsetTop hack removal changes (basically the bulk of the interdiff between this patch and attachment 471394 [details] [diff] [review], but this also has some whitespace cleanup and also a couple of font changes) ported?

Comment on attachment 471394 [details] [diff] [review] This should be it Looks good from a mac pov.

Subscribing to feeds is completely broken currently, could you guys please get this in really soon now, preferably yesterday?

Comment on attachment 471449 [details] [diff] [review] With fewer hacks [Checked in: Comment 25] http://hg.mozilla.org/comm-central/rev/e7918600229d

(In reply to comment #17) > 231 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_bug364677.html > | Feed served as text/xml without a channel/link should have been sniffed - got > undefined, expected "feedHandler" http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283650257.1283651482.3646.gz Linux comm-central-trunk debug test mochitests-5/5 on 2010/09/04 18:30:57 http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283655300.1283656282.20969.gz OS X 10.5 comm-central-trunk debug test mochitests-5/5 on 2010/09/04 19:55:00 Test (and related leak) fixed :-) V.Fixed

Gavin, ping for feedback and review.

Comment on attachment 492293 [details] [diff] [review] Firefox port of offsetTop hack removal This looks fine, but we apparently do not have test coverage for any of this UI, so I'm not very comfortable making changes to it. Can you write a test (or do you have a SeaMonkey test that I could port?)