350973 - Image overlayed on select widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[philippe (part-time)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a1) Gecko/20060831 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a1) Gecko/20060831 Camino/1.2+

Strange thing. With the 2006083120  trunk build, there is what looks like an image overlayed over the arrows of the select widgets.
Screenshot next.

Works correctly: 2006083116 build (Maya tinderbox)

Fails: 2006083120 (Maya tinderbox)

(with default profile and all that...)


Reproducible: Always

Comment 1

[philippe (part-time)]

Attachment: screenshot

From Bugzilla.
Position of the image varies slightly depending on font-size and width of select widget.

Comment 2

[philippe (part-time)]

Hmm, the same happens on a Cocoa Firefox build

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060831 Minefield/3.0a1 ID:2006083118

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Based on that, checkins: http://tinyurl.com/oclf3

I suspect bug 348614.

Comment 4

[Neil Deakin]

I suspect that the patch https://bugzilla.mozilla.org/attachment.cgi?id=234422 just needs to be ported to the Cocoa version

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Doesn't seem to happen with cocoa-cairo (which has other problems); but I think the patch for 234422 needs to be ported to the HITheme code I wrote as well.

Comment 7

[Nick Kreeger]

Attachment: Proposed Patch

It looks like the problem was that the patch on bug 348614 introduced code into the NS_THEME_DROPDOWN_BUTTON portion of |DrawWidgetBackground()|. That code is what handles the select buttons that have the two arrows on it (up/down arrow). Introducing the code that now sits in NS_THEME_DROPDOWN_BUTTON forces the generic arrow element that only points down to paint over the widget that has the two arrows.

Comment 8

[Neil Deakin]

Comment on attachment 236746 [details] [diff] [review]
Proposed Patch

Doesn't look like the right patch to me.

Bug 348614 added support for a standalone <dropmarker> with native theme support.
http://xulplanet.com/ndeakin/tests/xts/button/button-dropmarker.xul
should display a dropmarker.

Comment 9

[Nick Kreeger]

(In reply to comment #8)
> (From update of attachment 236746 [details] [diff] [review] [edit])
> Doesn't look like the right patch to me.
> 
> Bug 348614 added support for a standalone <dropmarker> with native theme
> support.
> http://xulplanet.com/ndeakin/tests/xts/button/button-dropmarker.xul
> should display a dropmarker.
> 

>I suspect that the patch https://bugzilla.mozilla.org/attachment.cgi?id=234422
>just needs to be ported to the Cocoa version

This is the updated version to the patch you pointed out above.

Comment 10

[Neil Deakin]

Not clear what you mean. If this is a Cocoa only bug (as I don't see it on the standard Mac build), should it not be the case that nsNativeThemeCocoa.cpp needs to be updated to match the changes in nsNativeThemeMac.cpp?

Comment 11

[Nick Kreeger]

(In reply to comment #10)
> Not clear what you mean. If this is a Cocoa only bug (as I don't see it on the
> standard Mac build), should it not be the case that nsNativeThemeCocoa.cpp
> needs to be updated to match the changes in nsNativeThemeMac.cpp?
> 

From my research, nsNativeThemeCocoa.cpp is used only for Cairo-Mac.

Comment 12

[Nick Kreeger]

(In reply to comment #11)
> (In reply to comment #10)
> > Not clear what you mean. If this is a Cocoa only bug (as I don't see it on the
> > standard Mac build), should it not be the case that nsNativeThemeCocoa.cpp
> > needs to be updated to match the changes in nsNativeThemeMac.cpp?
> > 
> 
> From my research, nsNativeThemeCocoa.cpp is used only for Cairo-Mac.
> 

From the Makefile in widget/src/cocoa:

ifndef MOZ_ENABLE_CAIRO_GFX
MAC_LCPPSRCS +=	nsNativeThemeMac.cpp \
		nsSound.cpp \
		$(NULL)
endif

Comment 13

[Håkan Waara]

We really need to get this bug fixed on trunk.

(In reply to comment #10)
> If this is a Cocoa only bug (as I don't see it on the
> standard Mac build), should it not be the case that nsNativeThemeCocoa.cpp
> needs to be updated to match the changes in nsNativeThemeMac.cpp?

If there is confusion whether the (in the patch, removed) code is needed still for the Carbonfox, can someone just check their normal firefox build (*non*-cocoa) to verify? 

If carbon still needs it, you can add a #ifndef MOZ_WIDGET_COCOA.

Comment 14

[Nick Kreeger]

Attachment: Cocoa widget only patch (v2)

Here is an updated patch that draws the <dropmarker> element if we aren't using cocoa widgets.

Comment 15

[Neil Deakin]

As I said I can't imagine that is the right patch. Did you try the testcase pointed to in comment 8? It should display a native themed dropmarker.

The real issue is that Cocoa is using native themed <select> widgets whereas Carbon is not. The real patch needs to modify layout/style/forms.css so that the dropmarker is not shown on Mac.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #15)
 
> The real issue is that Cocoa is using native themed <select> widgets whereas
> Carbon is not. The real patch needs to modify layout/style/forms.css so that
> the dropmarker is not shown on Mac.

Neil, we're already disabling button drawing on the selects in Cocoa forms: 

616 /* make sure nothing paints for the menulist button, since the button is
617   painted as part of the menulist. */
618 select > input[type="button"],
619 select > input[type="button"]:active {
620   background-image: none !important;
621 }

Does the dropmarker have a -moz-foo css constant we can call there to tell it not to display?  I'm not sure what/where else in forms.css's Cocoa section we can do something to make the dropmarker not appear on select buttons.  (Setting that block to display:none "solves" the problem but breaks the rest of the select sizing....)

Comment 17

[philippe (part-time)]

Based on the style rules in
https://bugzilla.mozilla.org/attachment.cgi?id=234931
themes/classic/global/mac/dropmarker.css


This sort of works for me:

/* make sure nothing paints for the menulist button, since the button is
  painted as part of the menulist. */
select > input[type="button"],
select > input[type="button"]:active {
  background-image: none !important;
  background-color: transparent !important;
  -moz-appearance: none !important;
  list-style-image: none !important;
  width: 0 !important;
  border: 0 none !important
}

I don't know how the rules in themes/classic/global/mac/dropmarker.css
get applied to the cocoa widgets, but not on a standard build of Minefield

Comment 18

[philippe (part-time)]

Attachment: forms.css with rules added

forms.css with the rules in comment 17 added. For testing purposes.
It works on my side on all my HTML test cases.

Comment 19

[Stuart Morgan]

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

Comment 20

[philippe (part-time)]

Attachment: proposed patch ?

per comment #15 and comment #17

(hopefully it is the right format, first time I do this)

Comment 21

[Josh Aas]

philippe - can you explain what this does, line by line? Thanks.

Comment 22

[Wevah (Nate Weaver)]

It seems that only three of those lines are actually necessary, at least in my testing:

  background-color: transparent !important; /* suppress default gray background */
  border: none !important;                  /* suppress default quasi-bezel */
  -moz-appearance: none;                    /* suppress default aqua-themed appearance */

Comment 23

[philippe (part-time)]

(In reply to comment #22)
> It seems that only three of those lines are actually necessary, at least in my
> testing:
> 
>   background-color: transparent !important; /* suppress default gray background
> */
>   border: none !important;                  /* suppress default quasi-bezel */
>   -moz-appearance: none;                    /* suppress default aqua-themed
> appearance */
> 

You're right. I added the list-style-image more for safety's sake than anything.
And thanks for explaining those rules.

@Josh
Bug 348614 modified (and added) some rules in 
skin/classic/global/dropmarker.css that are causing the problems for Cocoa widgets
(see patch https://bugzilla.mozilla.org/attachment.cgi?id=234931).
I  neutralised them here. 
I've tested this on a whole bunch of html testcases both with Camino and Cocoa Firefox without seeing any problems.
It also passes the XUL testcase mentioned by Neal in comment #8.

Comment 24

[Josh Aas]

Neil - are you willing to do a first review of this? If you can let me know if this looks good to you I can do a second review in which I actually test it.

Comment 25

[Andrey Nikanorov]

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

Comment 26

[Wevah (Nate Weaver)]

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

Comment 27

[(not reading, please use [e-mail] instead)]

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

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 239285 [details] [diff] [review]
proposed patch ?

>   background-image: none !important;
>+  background-color: transparent !important;

You should combine these two into a shorthand:

background: transparent ! important;

>+  -moz-appearance: none !important;
>+  list-style-image: none !important;

I don't see how list-style-image is at all relevant.  Please remove it or convince me that it's needed.

>+  border: 0 none !important

It's more conventional to write

border: none ! important;


That said, my real concerns here are bigger:  this code shouldn't have been enabled on the trunk because we've turned on cocoa widgets.  We shouldn't make the way our form controls respond to author styling totally inconsistent across platforms.  (I think Mano was talking about undoing that.)  Using nsITheme for form controls should be significantly more consistent (if done right) if you can take that route.

Comment 29

[philippe (part-time)]

Attachment: updated patch

per comment #28

(In reply to comment #28)
> That said, my real concerns here are bigger:  this code shouldn't have been
> enabled on the trunk because we've turned on cocoa widgets.  We shouldn't make
> the way our form controls respond to author styling totally inconsistent across
> platforms.  (I think Mano was talking about undoing that.)  Using nsITheme for
> form controls should be significantly more consistent (if done right) if you
> can take that route.
> 

I believe the goal is to make form controls allow author styling, see bug 320486.

[page authors want styling, users are equally divided, some don't care, some prefer no styleable widgets :-)]

Comment 30

[Josh Aas]

landed on trunk. Thanks Philippe!