Closed Bug 302729 Opened 19 years ago Closed 19 years ago

netError.dtd entities can't be formatted prettily

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.8beta4

People

(Reporter: Waldo, Assigned: Waldo)

References

()

Details

(Keywords: fixed1.8, late-l10n, Whiteboard: [affects l10n])

Attachments

(3 files, 1 obsolete file)

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.
Blocks: 301208
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #191137 - Flags: superreview?(bzbarsky)
Attachment #191137 - Flags: review?(benjamin)
Attachment #191137 - Flags: review?(benjamin) → review?(cbiesinger)
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.
Attachment #191137 - Flags: superreview?(bzbarsky) → superreview?(darin)
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.
Status: NEW → ASSIGNED
Flags: blocking1.8b4?
Whiteboard: [affects l10n]
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...
Attachment #191137 - Flags: review?(cbiesinger) → review-
Attached patch Updated patchSplinter Review
(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.
Attachment #191137 - Attachment is obsolete: true
Attachment #192114 - Flags: superreview?(darin)
Attachment #192114 - Flags: review?(cbiesinger)
Attachment #192114 - Flags: review?(cbiesinger) → review+
Attachment #191137 - Flags: superreview?(darin)
Flags: blocking1.8b4? → blocking1.8b4-
Attachment #192114 - Flags: superreview?(darin) → superreview+
Attachment #192114 - Flags: approval1.8b4?
Attachment #192114 - Flags: approval1.8b4? → approval1.8b4+
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.
Attachment #193230 - Flags: superreview?(darin)
Attachment #193230 - Flags: review?(darin)
Attachment #193230 - Flags: review?(darin) → review+
Attachment #193230 - Flags: superreview?(darin) → superreview+
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).
Attachment #193230 - Flags: approval1.8b4?
Attachment #193230 - Flags: approval1.8b4? → approval1.8b4+
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8, late-l10n
Resolution: --- → FIXED
Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050829
Firefox/1.0+
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.8beta4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: