21350 - Unknown content type dialog buttons don't look like buttons

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Akkana Peck]

A few days ago, there was apparently a CSS change somewhere that removed the
borders from buttons; now they just look like any other label, with no border
and no mouseover highlighting, and there's no way to tell that they're buttons
unless you already know where the buttons are in a dialog.

I hate to think that we'll ship M12 like this (I predict we'll get a lot of
complaints).

Comment 1

[Akkana Peck]

Cc'ing Kathy, who filed a similar bug to hyatt recently; that bug has been
closed, but the problem still exists at least as of this morning's build.

Comment 2

[hangas]

This is an easy fix.  Each dialog just needs the appropriate line in either the
xul or css (each case is different) that pulls in the global skin file.  I have
fixed some of these in my local tree already because I don't like using the
product in the current state.  Marking as dogfood to see if the PDT group will
bless this one.

Comment 3

[David Hyatt]

XUL windows/dialogs always load their package's master skin file, which is just
the package name, e.g., chrome://messenger/skin/.  XUL files NEVER load the
global skin file.

The master skin files for all packages should already be set up to import the
global skin.

Comment 4

[Eli Goldberg]

*** Bug 21403 has been marked as a duplicate of this bug. ***

Comment 5

[don]

Paul, how many other places does this need to be fixed?  Should we file separate
bugs for each dialog?  (BTW, I agree this sucks and it should be PDT+.)

Comment 6

[leger]

Putting on the PDT- radar. Not mandatory for dogfood.

Comment 7

[hangas]

Moving to M13 because of PDT-

Comment 8

[Simon Fraser [no longer active]]

Um, excuse me, but this has serious usability implications for all our dialogs.
You can't tell the buttons apart, heck, you can't even tell which are buttons and
which are not. Someone who doesn't know the history of the problem is going to be
completely confused by our dialogs.

I SERIOUSLY think that this should be PDT+

Comment 9

[Simon Fraser [no longer active]]

Cc:ing self so I can see this bug bouncing down the path to M15

Comment 10

[Simon Fraser [no longer active]]

What is the real fix here? How many files?

Comment 11

[Alec Flett]

The fix for this is incredibly easy, as long as hyatt approves. Hyatt - these
are global dialogs, so there's no component-specific CSS that inherits
global.css.

Here's my fix:

Index: dialogOverlay.xul
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/dialogOverlay.xul,v
retrieving revision 1.5
diff -u -r1.5 dialogOverlay.xul
--- dialogOverlay.xul	1999/09/14 01:50:14	1.5
+++ dialogOverlay.xul	1999/12/10 21:23:02
@@ -1,4 +1,5 @@
 <?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 <?xul-overlay href="chrome://global/content/platformDialogOverlay.xul"?>

 <!DOCTYPE window SYSTEM "chrome://global/locale/dialogOverlay.dtd">

Comment 12

[rubydoo123]

removing PDT- -- this bug should really be a PDT+, users will not be able to
distinguish what is a button -- as in things like submit, cancel, yes, no and
what isn't a button. Please reconsider this one folks.

Comment 13

[Alec Flett]

oops, after talking to hangas, that's the wrong file. It should be
commonDialog.xul, but the fix is exactly the same.

Comment 14

[Jim Roskind]

PDT was happy to see the small fix... if it grows larger, you'll have trouble
getting checkin approval next week.  Please try to get this in as soon as
sufficiently reviewed... you have jar and chofman's approval

Comment 15

[hangas]

Fix checked in.  Added <?xml-stylesheet href="chrome://global/skin/" type="text/
css"?> to commandDialog.xul.  This is not what Hyatt wanted here but it is a good
fix.  Hyatt and I will have to discuss how we want to handle common dialogs in
the future, we can either go with this approach or add a package to support
common dialogs.  The patch alecf first suggested was not used because it would
have caused duplicate loading of global.css for many dialogs.

Comment 16

[Akkana Peck]

Ahhhhhh (relieved sigh) ... thank you, thank you!

Comment 17

[Claudius Gayle]

I've verified that the fix took for most dialogs but there is at least one other
out there that didn't get the fix.
If this happens in the future, I'll file a new dialog-specific bug, but since this
is the first time, I'm going to reopen this bug simply to highlight the fact that
there are more dialogs out there.

The Unknown File -Type Dialog still exhibits the bug. I beleive this is the offending
file--> chrome://global/content/unknownContent.xul

Comment 18

[Simon Fraser [no longer active]]

cc: law for the ucth dialog.

Comment 19

[Phil Peterson]

Since this bug has morphed from its original form which was PDT+, I'm going to
change the bug summary to reflect the current problem and remove the PDT+ for
reevaluation.

Comment 20

[hangas]

As Phil pointed out this is now a different bug than it used to be.  This bug is
now concerning unknownContent.xul and downloadProgress.xul.  Both need the same
fix of <?xml-stylesheet href="chrome://global/skin/" type="text/css"?> being
added to these files.  This bug is now marked for M13 and will be addressed then.
Hyatt may decide that these "global" dialogs need their own package, in which
case we will take a different approach to fixing them.

Comment 21

[Phil Peterson]

M14 since Paul is out sick

Comment 22

[hangas]

Bumping to M15.

Comment 23

[hangas]

Sending to Ben.

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

this has probably been fixed. 

Comment 25

[Claudius Gayle]

VERIFIED fixed with 2000032910 builds