See the URL for proof -- the entities have to be valid JS strings, meaning no newlines. Currently, this forces entities to look like those at <http://lxr.mozilla.org/mozilla/source/dom/locales/en-US/chrome/netError.dtd> -- single-line entities that are difficult to both read and review (and type, because you have to think and consciously use [] instead of <>). (And no, JS line continuations are a hack I won't tolerate as a workaround.) I think this is fixable to make things look good, and I'm going to work on a patch that fixes this problem. Expect it really, really soon because time's so short (and I want this to make bug 301208 reviewing a working patch easier). I hope for tomorrow, but Sunday's more likely. If I hit insurmountable snags, I'll let people know ASAP so other people can take a stab if they want. If there are other issues I might hit with error pages besides the "copying getting undisplayed text" bug 39098, please post them here so I know what I need to do my best to avoid.

The patch works just fine for me; I took a look at an error page in the DOM inspector and it looked like I planned it would look. I also changed one entity in netError.dtd to test that description entities containing HTML work, and they do. I didn't convert netError.dtd because bug 301208 will be hacking that to pieces really soon and can fix things then. I'm not sure if the way I'm removing errorContainer is the way it should be done, so please make sure to look there so I know if there's a better way to do it. Removing an element by |el.parentNode.removeChild(el)| seems hackish, but I haven't done a whole lot of DOM scripting to know if that's what has to be done.

There's no way I can get to this before Aug 14.

(In reply to comment #1) > I'm not sure if the way I'm removing errorContainer is the way it should be > done, so please make sure to look there so I know if there's a better way to do > it. Removing an element by |el.parentNode.removeChild(el)| seems hackish, but Personally I don't like that way. I also made different thoughts in the last weeks about that issue. My intention would be to use a XML file containing all neccessary data and use XSLT to transform the source into XHTML. Would this be possible with the current restrictions of netError? In that case we have a clean Javascript free XHTML source. Also netError.dtd could contain normal tags.

(In reply to comment #3) > My intention would be to use a XML file containing all neccessary data and use > XSLT to transform the source into XHTML. Would this be possible with the > current restrictions of netError? In that case we have a clean Javascript free > XHTML source. Also netError.dtd could contain normal tags. It might be possible, but I see a couple problems. First, it's unlikely to be space-efficient. If we can add 13 lines of code that happens to be JavaScript and solve the problem, I'll take that any day over adding a much larger number of lines. Second, you'd be introducing an XSLT dependency (XSLT is defined in extensions/transformiix, which is an optional part of the build) in docshell which probably wouldn't be tolerated.

Firefox ships with XSLT, and XSLT will be part of XULRunner, so using XSLT in the implementation of error pages should not be a no-no. Removing the JS dependency may be of interest since we had to add special cases to ensure that error pages could use JS even when JS is globally disabled. So, we could remove those hacks if error pages didn't require JS. Just a thought ;-)

(In reply to comment #5) > Removing the JS dependency may be of interest since we had to add special > cases to ensure that error pages could use JS even when JS is globally > disabled. So, we could remove those hacks if error pages didn't require JS. I'm all for removing hacks where possible, but no one's written the XSLT patch yet, and my XSLT-fu's not good enough to do it. The current, already-written and already-available patch does 95% of the job with very little effort; using an as-yet-unwritten XSLT-based patch will get us the last 5% of the way but only with far more effort. I also remind people that the chrome JS hacks are already in place, and if they were deemed acceptable for the existing implementation they should still be acceptable now. If and when we see an XSLT implementation, then we should consider using it and removing the hacks. However, in the meantime we have a choice between doing nothing (and being stuck in a crappy situation) and doing something that's not quite perfect but is loads better than what we have now. I much prefer doing something for some improvement over doing nothing for no improvement. I also point out that for the purposes of this bug, *XSLT is a red herring*. This bug is about making entities readable, not about removing chrome JS hacks or using XSLT in error pages.

Jeff: I totally agree with you. I didn't mean to suggest that we *should* or *must* use XSLT. That's why I used words like *may* and *could* ;-)

Comment on attachment 191137 [details] [diff] [review] Patch > Removing an element by |el.parentNode.removeChild(el)| seems hackish seems perfectly fine to me... Can you explain your try..catch? I am not sure why it does the right thing. I think it largely catches an exception trying to read the textContent of null, and I don't think that's so great. why not: + // if it's an unknown error or there's no title/description + // defined, get the generic message + errTitle = document.getElementById("et_" + err).textContent; + errDesc = document.getElementById("ed_" + err); if (!errTitle) errTitle = document.getElementById("et_generic"); if (!errDesc) errDesc = document.getElementById("ed_generic"); errTitle = errTitle.textContent; (maybe with an intermediate variable for the element. or even don't assign textContent here, and just use .textContent later) have you considered using <h1> instead of <span> for the list of titles, and using replaceChild here too? I'd probably style errContainer to display:none, just in case...

(In reply to comment #8) > Can you explain your try..catch? Um, it was supposed to be that if there were no description or title we'd fall back to the generic one, but I was only thinking correctly for half the statements in the try..catch. > if (!errTitle) > errTitle = document.getElementById("et_generic"); > if (!errDesc) > errDesc = document.getElementById("ed_generic"); > errTitle = errTitle.textContent; This is what I do now, except that I test for whether title *or* desc is null so that description and title are always in sync. > have you considered using <h1> instead of <span> for the list of titles, and > using replaceChild here too? I was under the impression titles should be text-only, but I suppose it doesn't hurt to let them be arbitrary HTML. > I'd probably style errContainer to display:none, just in case... Done. I didn't do this first time through because of how many more files would need to be touched, but I had a feeling it might be requested.

has this landed on the 1.8 branch yet? /cb

(In reply to comment #10) > has this landed on the 1.8 branch yet? No, because the patch requires changes to error string formatting in netError.dtd in order for it to work properly. The plan was this would happen in bug 301208, but it now looks like it would be more expedient to quickly change the formatting here and let that bug be fixed in its own time. I'll cook up a patch to change the error string formatting so we can land this. It should basically be s/\[/</g and s/\]/>/g in netError.dtd, so it'll be trivial to make.

I took a look through the file, and the only instances of "[" and "]" were in fake elements inside error strings, so tr/[]/<>/ was all I had to do to make this patch.

Comment on attachment 193230 [details] [diff] [review] Convert [tag][/tag] in error strings to <tag></tag> This patch is needed to be able to check in the previous patch posted here (because bug 301208's taking longer to finish than was expected).

The bitrotting occurred due to an addition by Jesse of some focusing code right at the end of the main JS section; adding his section into the code this patch created trivially resolves the conflict.

Attachment 192114 [details] [diff]: Trunk: Checking in docshell/resources/content/netError.xhtml; new revision: 1.12; previous revision: 1.11 Checking in themes/classic/global/netError.css; new revision: 1.2; previous revision: 1.1 Checking in themes/modern/global/netError.css; new revision: 1.2; previous revision: 1.1 Checking in toolkit/themes/pinstripe/global/netError.css; new revision: 1.2; previous revision: 1.1 Checking in toolkit/themes/qute/global/netError.css; new revision: 1.2; previous revision: 1.1 Checking in toolkit/themes/winstripe/global/netError.css; new revision: 1.2; previous revision: 1.1 done Branch: Checking in docshell/resources/content/netError.xhtml; new revision: 1.10.2.2; previous revision: 1.10.2.1 Checking in themes/classic/global/netError.css; new revision: 1.1.2.1; previous revision: 1.1 Checking in themes/modern/global/netError.css; new revision: 1.1.2.1; previous revision: 1.1 Checking in toolkit/themes/pinstripe/global/netError.css; new revision: 1.1.2.1; previous revision: 1.1 Checking in toolkit/themes/qute/global/netError.css; new revision: 1.1.2.1; previous revision: 1.1 Checking in toolkit/themes/winstripe/global/netError.css; new revision: 1.1.2.1; previous revision: 1.1 Attachment 193230 [details] [diff]: Trunk: Checking in netError.dtd; new revision: 1.3; previous revision: 1.2 Branch: Checking in netError.dtd; new revision: 1.2.2.1; previous revision: 1.2

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050829 Firefox/1.0+