176177 - History, Bookmarks and Highlight buttons don't have a pressed state in WinXP Luna

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect, P3)

Description

[Rack]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021022 Phoenix/0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021022 Phoenix/0.3

I customized my toolbar to include the download button, but I noticed that when
I click it to bring up that sidebar, the button has a while background instead
of the expected gray (or transparent perhaps)

Reproducible: Always

Steps to Reproduce:
1. Customize toolbar to include the download button
2. click button


Actual Results:  
white background on button

Expected Results:  
gray / default background on button

Comment 1

[Simon Paquet [:sipaq]]

Confirming on Mozilla/5.0 (Windows NT 5.0; rv:1.2b) Gecko/20021022 Phoenix/0.3

Comment 2

[Stephen Perry]

Confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021023
Phoenix/0.3.  Similar problem can been seen on the buttons for displaying the
bookmarks and history sidebars as well.

Comment 3

[Rack]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021022 Phoenix/0.3

I confirm history and bookmarks as well.  I never thought to check.

Comment 4

[Simon Paquet [:sipaq]]

Attachment: Patch for white background on history, download and bookmark buttons

I've created a patch for this problem. This is my first patch, so please bear
with me if anything is not correct.

Comment 5

[Chris Neale]

NB this is for Phoenix Classic

pseudo patching as userChrome.css entries : doesn't work as in patch

#history-button {
  background-color: transparent !important;
}

with 

#history-button[checked="true"]{
  background-color: transparent !important;
}

white background not gone either

patching global/toolbarbuttons.css would probably be more promising

when phoenix cvs rebuilt I'll use mozengineer/DOM Inspector to investigate further

Comment 6

[Chris Neale]

: [ ; pseudo patched userChrome[-example].css; of course intention was to save
as userChrome.css

patch should work fine

sorry for bugspam

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 104643 [details] [diff] [review]
Patch for white background on history, download and bookmark buttons

blake, can you review this patch? it's basically putting a background-color:
transparent !important on the sidebar toggle buttons. I seem to remember that
this was proposed but not used because of native theme issues.

Comment 8

[Simon Paquet [:sipaq]]

I'm definiteley not very much into UI design and programming, but how can a
"background-color: transparent" affect native theme colors? In my limited
understanding a transparent background-color would always honor the native theme
color for toolbars, windows etc.

Comment 9

[David Tenser [:djst]]

I am not sure if this is the same bug or a related one, but in Windows XP (Luna
interface), these three buttons don't have a depressed state at all. There is
absolutely no visual clue as to whether or not the button is depressed.

In IE6, if you click on Favorites for example, that button gets a white background.

Comment 10

[Sébastien Delahaye]

Marking as WFM, since this was a Px Classic issue, and Qute is now the default
theme.

Comment 11

[David Tenser [:djst]]

What are you talking about, Sébastien? Try it! :)

Comment 12

[Sébastien Delahaye]

Qute buttons for download/history/bookmarks don't have a white background when
clicked, at least on w2k. See http://images.delahaye.net/buttons.png
You can compare to http://seb.delahaye.net/phoenix/images/phoenix-history.png
where the problem appears (I don't think it was a problem, though).

That's what this bug is about.

Comment 13

[David Tenser [:djst]]

You're right. Changing summary. Compare with IE to see how it should look like.

Comment 14

[Simon Paquet [:sipaq]]

I don't see this bug here with all the latest nightlies (I tried the nightlies
21/2, 23/2, 25/2 and 27/2). Can anybody confirm this and mark this as WFM.

Comment 15

[David Tenser [:djst]]

I'm morphing this bug into a Windows XP-specific bug instead.

When using the Luna interface in Windows XP, you don't see if the buttons are
pressed or not, as I previously mentioned in comment 9.

Attaching screenshots.

Comment 16

[David Tenser [:djst]]

Attachment: Windows Classic works fine

In Windows Classic, all is fine.

Comment 17

[David Tenser [:djst]]

Attachment: Windows Luna screenshot

When using the Luna theme in Windows XP, you don't see that the Downloads
button is pressed.

Comment 18

[Simon Paquet [:sipaq]]

Correcting typo in bug description.

Comment 19

[Simon Paquet [:sipaq]]

David, how about this? Have the new icons from Arvid changed anything here?

Comment 20

[David Tenser [:djst]]

Still the same as shown in the screenshots.

Comment 21

[Blake Ross]

patches anyone?

Comment 22

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 23

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

-> 0.9

Comment 24

[PikeUK]

I think this might be because the buttons have "-moz-appearance: toolbutton"
set. As I understand it, on XP Luna (and maybe MacOS X?) this causes the border
and background properties to be ignored and uses native rendering instead (see
http://www.w3.org/TR/css3-ui/#appearance). Unfortunately this means that most of
the properties in the toolbarbutton[checked="true"] rule are going to have no
effect on the button's appearance.

The ideal solution to this would be to have something like a "-moz-appearance:
toolbarbutton-selected" property that could draw a native version of the button
but in the selected state. Failing that, adding "-moz-appearance: none" to the
toolbarbutton[checked="true"] rule also fixes this (though you lose the pretty
rounded button look when selected).

Of course I could be totally off the mark here...

p.s. This bug also affects the buttons in the JS console (I can't be the only
one who's accidentally set the filter to "Messages" and then spent 15 minutes
wondering why my buggy JS isn't generating any error messages :-)

Comment 25

[Richard Klein]

This occurs for all buttons that are of type="checkbox".  Asking for 1.0 blocking.

Comment 26

[Richard Klein]

Shouldn't there be some logic to look for a checked state in the function
GetThemePartAndState in nsNativeThemeWin.cpp for case of NS_THEME_BUTTON and
NS_THEME_TOOLBAR_BUTTON.

Comment 27

[Jesse Ruderman]

This is more visible now because of the Highlight button in the Find toolbar,
which you can see with default settings.  Nominating for blocking1.0.

Comment 28

[Blake Ross]

-> me

Comment 29

[:deanis74]

We should remove "Download" from the summary, since it's not a toggle button
anymore.

Comment 30

[Steffen Wilberg]

Removing Downloads, adding Hightlight button to summary.

Comment 31

[Blake Ross]

Fixed.

Comment 32

[David Tenser [:djst]]

(In reply to comment #30)
> Removing Downloads, adding Hightlight button to summary.

Why? Why wasn't Downloads fixed? Sure, it's not a sidebar anymore, but it should
have a pressed state when visible anyway. Clicking on it while pressed should
close the Download manager again. Should a separate bug be filed for that?

Comment 33

[:deanis74]

No, no it shouldn't.  Downloads is now a normal push button, just like Refresh
or Stop.  There is no toggle state to its action, unlike the two sidebar
buttons.  Pressing it always shows the Downloads window.

Your suggestion of giving it a pressed state when the Downloads window is open
would add more confusion.  For the two sidebar buttons, clicking them when they
are in the pressed state hides the associated object.  So what would clicking a
pressed Downloads button do, hide the Downloads window?  Anything else would be
inconsistent with how toggle buttons work.  Downloads is definitely not a toggle
button.

Comment 34

[David Tenser [:djst]]

(In reply to comment #33)
> Your suggestion of giving it a pressed state when the Downloads window is open
> would add more confusion.  [...]  So what would clicking a
> pressed Downloads button do, hide the Downloads window?

Yes, that was my suggestion. I don't see why that would add more confusion than
hiding the sidebar as is done now if clicking on a pressed History or Bookmarks
icon? It would add consistency, though.

Comment 35

[:deanis74]

That's changing the behavior of the button.  I like what it does now, being
always showing the Downloads window.  Downloads is no longer a sidebar, why
should it behave like one?

Comment 36

[Steffen Wilberg]

I attached a patch to bug 228699. It makes the download manager close on ctrl-y
or pressing the toolbar button. It also gives the button a pressed state, the
same for all windows.
The result is that you can toggle the download manager: Click to have a look at
your downloads. Click again to get it out of the way. Or hit ctrl-y twice for that.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The changes here should be checked into the trunk, and also the 1.7 branch. We
don't want Aviary to have a forked Gecko.

Also, changes to widget/ need review. Even on the Aviary branch.

Comment 38

[PikeUK]

As far as I can see, (at least part of) this never went into the trunk:

http://bonsai.mozilla.org/cvsview2.cgi?subdir=mozilla/gfx/src/windows&command=DIFF_FRAMESET&file=nsNativeThemeWin.cpp&rev1=3.35.2.1&rev2=3.35.2.1.4.1
http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsNativeThemeWin.cpp#442
http://lxr.mozilla.org/aviarybranch/source/gfx/src/windows/nsNativeThemeWin.cpp#551

http://mail.mozilla.org/pipermail/aviary/2004-November/000176.html

?

Comment 39

[PikeUK]

Still needed on trunk.

Comment 40

[Peter Lairo]

[OT]
Would fixing this bug magically also fix the similar bug in Thunderbird's
compose window: the "Contacts" button?
[/OT]

Comment 41

[Worcester12345]

Needs new target milestone since the one listed is long gone.

Comment 42

[Worcester12345]

I take it that taking away any target and removing any blocking indicates this
is pretty much going into the dump file?

Comment 43

[Son Le]

Attachment: patch v0

Add checked state back to toolbarbuttons.

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 177350 [details] [diff] [review]
patch v0

I don't know the native theme code

Comment 45

[Ere Maijala (slow)]

Comment on attachment 177350 [details] [diff] [review]
patch v0

r=emaijala

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177350 [details] [diff] [review]
patch v0

I don't follow this.  Why is TB_CHECKED 5, of all things?  Where are TB_CHECKED
and TB_HOVER_CHECKEd actually used?

Comment 47

[Son Le]

(In reply to comment #46)
> I don't follow this.  Why is TB_CHECKED 5, of all things?  Where are TB_CHECKED
> and TB_HOVER_CHECKEd actually used?

These are used by drawThemeBG() in DrawWidgetBackground() to render the widget.
TB_CHECKED and TB_HOVER_CHECKED are specific states of the toolbar 'part' (see
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/commctls/userex/topics/partsandstates.asp
).

Note:
TB_CHECKED = TS_CHECKED = 5;
TB_HOVER_CHECKED = TS_HOT_CHECKED = 6;

I chose to keep the constants consistent with the aviarybranch (see comment #38).

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177350 [details] [diff] [review]
patch v0

sr=bzbarsky if some comments are added (before this list of constants?)
pointing to the relevant MSDN documentation....

Comment 49

[Son Le]

Attachment: patch v1

Updated patch with additional comments.

Can someone please check this in for me?

Comment 50

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in gfx/src/shared/nsNativeTheme.h;
/cvsroot/mozilla/gfx/src/shared/nsNativeTheme.h,v  <--  nsNativeTheme.h
new revision: 1.14; previous revision: 1.13
done
Checking in gfx/src/windows/nsNativeThemeWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsNativeThemeWin.cpp,v  <--  nsNativeThemeWin.cpp
new revision: 3.43; previous revision: 3.42
done

please mark fixed if it is

Comment 51

[Son Le]

Comment on attachment 177350 [details] [diff] [review]
patch v0

emaijala's review+ flag was lost

Comment 52

[[e-mail]]

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

Comment 53

[Hussam Al-Tayeb]

There is no pressed state using gtk2 clearlooks theme. Is this ther same or as different bug?

Comment 54

[Son Le]

This bug was a Win32 only so I'd say the gtk2 issue is a different bug.

Comment 55

[Worcester12345]

(In reply to comment #54)
> This bug was a Win32 only so I'd say the gtk2 issue is a different bug.
> 

The original poster lists:
"User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b)
Gecko/20021022 Phoenix/0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b)
Gecko/20021022 Phoenix/0.3"

??

Comment 56

[Son Le]

(In reply to comment #55)

Regardless of the browser, this bug for OS = WinXP, Component = GFX: Win32. If it affects Linux/gtk2 another bug should be raised.