46683 - items in menus for Folders on Personal Toolbar are all underlined

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect, P3)

Description

[Rann Bar-On]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win95; en-US; m17) Gecko/20000727
BuildID:    2000072708

in today's win32 build, the bookmarks in the personal toolbar using the classic
skin are underlines, as are the buttons on the toolbar.  The underlines are
uneven and too long.

Reproducible: Always
Steps to Reproduce:
1. switch to classic skin
2. mouseover on the personal toolbar buttons and/or click the bookmarks menu on
the personal toolbar

Actual Results:  underlined bookmarks

Expected Results:  non-underlined bookmarks

Comment 1

[Stephen Koren]

Repro on 072708 NT4. Confirming bug. Would look a lot nicer as it was in 
previously builds, with non-underlined buttons on personal toolbar.

Comment 2

[Stephen Koren]

Was this done intentionally?

Comment 3

[Blake Ross]

->themes

Comment 4

[Blake Ross]

cc ben and nikhil

Comment 5

[Nikhil Bhatla (gone)]

Ben, did you change this?  I know this is a recent change I made to mac Classic, 
but did you port this to windows classic as well?  Reassigning to you.

Comment 6

[Rann Bar-On]

in today's build (2000072908), the bookmarks menu on the personal toolber has
every bookmark underlined when I initially click on the bookmarks button, but
the underlines disappear when I move the mouse over the bookmarks.  The buttons
on the toolbar are still underlined, no change there.

Comment 7

[Blake Ross]

The link-style personal toolbar buttons was intentional.  Ben has a fix for the
other menu problem.  Updating summary to reflect that...

Comment 8

[Phil Peterson]

add classic, polish keywords, cc me

Comment 9

[Dan Nunn]

The bookmarks menu in the personal toolbar is always underlined in today's build
(082508). Would be nice to see it go.

Comment 10

[Blake Ross]

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

Comment 11

[Blake Ross]

cc jrgm, who ben said he was talking with about this problem.

Comment 12

[John Morrison]

Was there a question about the CSS 'text-decoration' property? (I was saying
that (unfortunately) all child elements of a parent with this property must
be styled with the same text-decoration. i.e., this is the one CSS property
you cannot override).

Comment 13

[Blake Ross]

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

Comment 14

[Blake Ross]

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

Comment 15

[Andrew Thompson]

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

Comment 16

[Andrew Thompson]

Added dependency to underlying bug

Comment 17

[timeless]

Attachment: Proposed patch. We shouldn't underline folders, only links

Comment 18

[timeless]

Patch attached. Seeking review and approval.
Risk assessment: minimal

Comment 19

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

I can supply review or super review. Either way this looks fine. Get someone 
else to look at it and go right ahead. 

r or a=ben. 

Comment 20

[Blake Ross]

so this patch just works around the problem by not underlining folders on 
hover, right?

Comment 21

[Syd Logan]

a=syd

Comment 22

[timeless]

<trudelle_> timeless: you can rtm+ yourself if you have a=/r=, if I understand 
latest PDT process.

PDT: if you decide not to plus this bug which is absolutely painless, mark as 
fixed, because it's fixed on trunk.

Blake will verify when it's marked as fixed.

Comment 23

[timeless]

This bug as is makes the folder popup menus very hard to read, and has high 
visibility.

Comment 24

[Peter Trudelle]

PDT: I hope I recalled the stated policy for non-NS rtm nomination correctly.
I'd also mention that this fix stops folders from being underlined, which is
consistent, since that is a visual cue for the link affordance, and folders are
not links.

Comment 25

[Peter Trudelle]

Fixing summary: problem is that all items in all PT folder menus are underlined,
always, not just on mouseover.

Comment 26

[Phil Peterson]

rtm++

Comment 27

[timeless]

fix checked in to trunk and branch.

Comment 28

[lchiang]

Claudius - please verify this on the branch builds. thanks.

Comment 29

[Claudius Gayle]

VERIFIED Fixed in 2000101608 branch builds

Comment 30

[Andrew Thompson]

Guys, I'm still seeing this in yesterday's Solaris build with the Classic skin.

To be specific, I can see it in the Bookmarks menu in the personal toolbar if I 
turn that on. Someone should confirm this is still there on Solaris or confirm 
that I'm seeing things, because I could have sworn this was fixed.

Comment 31

[Claudius Gayle]

you are not crazy this is very much not fixed on the trunk. I'm looking at a 2000101608
Mac Trunk build and it's not fixed.

reopening - but for TRUNK ONLY

NOTE: This is still VERIFIED FIXED on the branch.

Comment 32

[timeless]

I object. it looks correct for me on solaris8intel built sometime today from 
cvs. I even turned that on.  You all know where to reach me, setup a vnc server 
and i'll investigate.

Comment 33

[John Morrison]

I would agree with timeless based on an inspection of the package that is 
pushed out on Solaris (Okay, I don't have solaris handy, but if it is fixed
for linux but not solaris, that would have to be a build problem, but I 
see timeless's changes in those bits' css files). 

But this caused a second look to note that this is still visible on Mac 
Classic. And I even see this in the release bits for today.

This is likely because some other style rule is matching that button for its 
hover state, specified somewhere else in the Mac Classic skin. As I noted, once 
this rule for 'text-decoration' is turned on for a parent, it cannot be 
overridden for the children by any method (short of changing the document's 
structure). 

-> hewitt, for a Mac fix Classic skin -- it may just be that you need to remove 
the text-decoration property from '.button-toolbar:hover, 
.button-toolbar:hover:active' in classic/communicator/mac/button.css
(

Comment 34

[John Morrison]

Oops, meant to -> hewitt. See the above comment. 

At any rate, this one-line change appeared to fix it for me on Mac, but I 
don't really have a Mac build to test properly on (I just jammed the change 
into the classic.jar and copied it back). 

Index: button.css
===================================================================
RCS file: /cvsroot/mozilla/themes/classic/communicator/mac/button.css,v
retrieving revision 1.14
diff -u -r1.14 button.css
--- button.css  2000/09/06 02:16:35     1.14
+++ button.css  2000/10/17 05:59:37
@@ -43,7 +43,6 @@
   .button-toolbar:hover, .button-toolbar:hover:active
     {
          color                           : #0000FF;
-         text-decoration         : underline;
     }

Comment 35

[timeless]

Yeah, I traced it to that line, unfortunately that defeats all hover. I need a 
css expert, i suspect that mac is clobbering the bookmarks.css rules. But i'm 
not sure how to prove it.

Attn: Hixie: helpwanted.

Comment 36

[John Morrison]

How do you mean 'it defeats all hover'. Yes, I guess it does remove the 
'text-decoration: underline' for any toolbar button, but I assumed that 
was the state of affairs for Win Classic. (The problem is that if a selector
matches and has a rule for text-decoration, all children will get it, no matter 
what.)

(By the way, are you on a Mac timeless. I gave it to hewitt assuming that you 
didn't have access to one.)

Comment 37

[timeless]

Sorry, i do have macs, they don't have networking, i'm talking w/ lordpixel who 
was dazed.  After he thought it through he agreed that removing the line works.
r=timeless on your patch.

Comment 38

[Andrew Thompson]

Also my original comment about Solaris was completely bogus.
Seems my brain was so addled yesterday I managedto confuse CDE with Mac OS !

Bug was on Mac OS, John Morrision's patch fixes it on my Mac.

Comment 39

[hangas]

Marking rtm- because Netscape folks not allowed to make changes like this at
this time.  We need to get this in to the trunk as soon as we are allowed.

Comment 40

[timeless]

This bug should not have been unassigned from me. I was working on it and I 
came to the same conclusions as...John Morrison                                 

changing the status to the status from 2000-10-04 00:45:55.
I am sorry that my fix was incomplete, I think i did mention that other file 
while i was working on the original patch but I was uncertain of its purpose.
This bug should not have been verified fixed since it obviously wasn't.

Rewriting summary.

Comment 41

[Claudius Gayle]

let me just mention, this bug never was marked 'verified' fixed - because it was never verified
on the trunk.

I did note in this bug report that it is 'verified fixed' on the branch - mostly because it is, on every
branch build from the 3 top platforms with the 2000101608 builds. Given recent comments, I'm not
sure why, but it does work.

Comment 42

[Claudius Gayle]

Hmmm, in a complete turn of events I've since discovered that my build is special and this bug
works for _me_ on my branch build - but no one else's. So this'll need to be completely verified
from scratch when it's properly fixed (removing vtrunk kw).

Comment 43

[timeless]

Attachment: here is the patch that jrgm and I suggested (it cleans out some tabs from the previous line)

Comment 44

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

OK this patch works for me. a=ben. 

Comment 45

[Jim Roskind]

Since this bug was re-opened, pushing it back to rtm need info.

Please nominate to rtm-plus when you have a fix in hand, and its risk and value
can be evaluated for the branch.

Thanks,
Jim

Comment 46

[John Morrison]

r=jrgm. So that, plus the patch 10/19 22:49, plus a=ben, would make this good
to go. Setting to [rtm+].

Comment 47

[timeless]

Fix checked into trunk.
Updating platform, and marking pp since all the other platforms are doing the 
right thing.
Risk assesment: minimial [approximating zero]
This bug makes folder popup menus very hard to read, and has high visibility on 
macintosh (a platform where presentation really matters). The fix stops folders 
from being underlined (on macos), which is consistent, since that is a visual 
cue for the link affordance, and folders are not links. This also gives the mac 
build parity. [rtm+] info given.

Comment 48

[Phil Peterson]

rtm++. Please get this checked into the Netscape branch, getting help from a
Netscape employee if necessary.

Comment 49

[Blake Ross]

It is a little upsetting that this just got approval for branch checkin.  At 
this late date, and in the face of infinitely more important bug fixes getting 
cut, this theme-dependent bug, which is nothing more than a visual annoyance, 
certainly does not meet PDT's "strict" criteria for P1, stop-ship, extreme 
dataloss or crasher bugs ONLY.

Guys, don't get me wrong -- this bug is annoying beyond belief, and I'm not 
making an effort to get it cut.  But I have seen far too many important bugs, 
much more significant than this, and with similarly small patches, get minused 
to watch this be given ++.  And one of these days, when the rtm excitement 
cools down, I'll probably ask PDT what their rationale for doing so was.

(cc'ing Ian, whose probably seen one too many standards compliance bug get cut 
to be happy about this, and contacting timeless immediately to check this in so 
I don't feel bad if this gets cut because of my little rant, which certainly 
wasn't my intention)

Comment 50

[timeless]

10/21/2000 12:35 jag 1.14.4.1 Netscape_20000922_BRANCH (Checking in for 
timeless) fix.

blake: this was already fixed by the time you nagged. And yes I understand your 
issues.

Comment 51

[Andrew Thompson]

Also, this was a one libne fix to CSS so the risk was vanishingly small.

I also sympathise with your issues but I'm sure *some of* those "small patches" 
you mention were siginificantly riskier than this.

Comment 52

[Blake Ross]

No.

There have been many such "vanishingly small risk" patches that have been 
turned down.  I agree with this, since Netscape has to draw the line somewhere 
and ship, but then it baffles me when something like this is allowed.

And I think the fact that PDT allowed whitespace fixes in this patch shows that 
there was something fundamentally wrong in their decision to ++ it.  PDT ++ 
rules have ALWAYS been: whitespace/correctness clean up in the trunk, 
smallest/most minimized patch in the branch.

In any case, what's done is done.  QA assigning to patty to verify since I 
don't have a mac.

Comment 53

[Andrew Thompson]

The key phrase was *some of*

As for whitespace, if that's PDT policy then I agree they broken it, but that's  
a meaningless policy anyway. Who gives a (random profanity) about whitespace?

Or am I being dumb... can whitespace break patch/diff ? 

Comment 54

[timeless]

stop spamming my bug!! use a newsgroup and don't cc me.

I'm sorry about the whitespace; for my next big c++ patch the rtm branch will 
get none of the whitespace.

Comment 55

[Patty Mac]

marking verified on mac (2000-10-23-08-MN6).