Closed
Bug 590725
Opened 14 years ago
Closed 14 years ago
Convert suite/ files for content XUL being killed
Categories
(SeaMonkey :: Feed Discovery and Preview, defect)
SeaMonkey
Feed Discovery and Preview
Tracking
(blocking-seamonkey2.1 b1+)
VERIFIED
FIXED
seamonkey2.1b1
Tracking | Status | |
---|---|---|
blocking-seamonkey2.1 | --- | b1+ |
People
(Reporter: kairo, Assigned: Callek)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(3 files, 4 obsolete files)
7.93 KB,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
20.69 KB,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•14 years ago
|
||
This should do it, I have tested very sparsely so far.
Assignee | ||
Comment 3•14 years ago
|
||
Do a bit more changes to satisfy certError needs
Attachment #469272 -
Attachment is obsolete: true
Attachment #469330 -
Flags: review?(neil)
Attachment #469272 -
Flags: review?(neil)
Comment 4•14 years ago
|
||
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.
Attachment #469330 -
Flags: review?(neil) → review-
Comment 5•14 years ago
|
||
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).
Reporter | ||
Comment 6•14 years ago
|
||
(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.
Comment 7•14 years ago
|
||
I'd prefer xbl:inherits="id" but I thought that would get r- ;-)
Attachment #469596 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 8•14 years ago
|
||
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.
Attachment #469596 -
Flags: review?(bugspam.Callek) → review+
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #469330 -
Attachment is obsolete: true
Attachment #469738 -
Flags: review?(neil)
Assignee | ||
Comment 11•14 years ago
|
||
Comment on attachment 469596 [details] [diff] [review]
certError fix
[Checked in: Comment 11]
http://hg.mozilla.org/comm-central/rev/b9b8db196a54
Attachment #469596 -
Attachment description: certError fix → certError fix [checked in]
Comment 12•14 years ago
|
||
Err, 2.0 Branch?!
Updated•14 years ago
|
Target Milestone: --- → seamonkey2.1b1
Version: SeaMonkey 2.0 Branch → Trunk
Comment 13•14 years ago
|
||
(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 14•14 years ago
|
||
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.]
Comment 15•14 years ago
|
||
> Hmm, well I'm sure getBoundingClientRect was added to replace something...
IIRC document.height/document.width
Comment 16•14 years ago
|
||
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.
Attachment #469738 -
Flags: review?(neil) → review-
Comment 17•14 years ago
|
||
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)
Blocks: SmTestFail
Assignee | ||
Comment 18•14 years ago
|
||
(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)
Assignee | ||
Comment 19•14 years ago
|
||
Attachment #469738 -
Attachment is obsolete: true
Attachment #471394 -
Flags: review?(neil)
Attachment #471394 -
Flags: feedback?
Assignee | ||
Updated•14 years ago
|
Attachment #471394 -
Flags: feedback? → feedback?(stefanh)
Assignee | ||
Comment 20•14 years ago
|
||
(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 21•14 years ago
|
||
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.
Comment 22•14 years ago
|
||
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?
Attachment #471449 -
Flags: review?(bugspam.Callek)
Attachment #471449 -
Flags: feedback?(gavin.sharp)
Comment 23•14 years ago
|
||
Comment on attachment 471394 [details] [diff] [review]
This should be it
Looks good from a mac pov.
Attachment #471394 -
Flags: feedback?(stefanh) → feedback+
Reporter | ||
Comment 24•14 years ago
|
||
Subscribing to feeds is completely broken currently, could you guys please get this in really soon now, preferably yesterday?
blocking-seamonkey2.1: --- → b1+
Keywords: regression
Attachment #471449 -
Attachment description: With fewer hacks → With fewer hacks [Checked in: Comment 25]
Comment 25•14 years ago
|
||
Comment on attachment 471449 [details] [diff] [review]
With fewer hacks [Checked in: Comment 25]
http://hg.mozilla.org/comm-central/rev/e7918600229d
Updated•14 years ago
|
Attachment #471394 -
Attachment is obsolete: true
Attachment #471394 -
Flags: review?(neil)
Assignee | ||
Updated•14 years ago
|
Attachment #471449 -
Flags: review?(bugspam.Callek) → review+
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Attachment #469596 -
Attachment description: certError fix [checked in] → certError fix
[Checked in: Comment 11]
Comment 26•14 years ago
|
||
(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
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Comment 27•14 years ago
|
||
Attachment #492293 -
Flags: review?(gavin.sharp)
Comment 28•14 years ago
|
||
Gavin, ping for feedback and review.
Updated•13 years ago
|
Attachment #471449 -
Flags: feedback?(gavin.sharp)
Comment 29•13 years ago
|
||
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?)
Attachment #492293 -
Flags: review?(gavin.sharp) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•