638176 - Make <progress> friendly with -moz-appearance:none and author styling

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

Currently, I see two issues with <progress> and -moz-appearance:none:

1. You have to explicitly set -moz-appearance:none; to your stylesheet if you want to remove the native rendering, regardless of the properties you have set. For example, "progress { background-color: red; }" will do nothing. It seems that most elements fallback to moz-appearance:none if they can't fulfill a given rule. Should we do the same for progress?

2. GTK and Windows have an appearance for the progress element border and an appearance for the bar itself but the Cocoa widget has only one appearance (the widget draws the bar itself). As a consequence, currently, if you do progress::-moz-progress-bar { -moz-appearance: none; }, nothing will happen if you are using MacOS X. I think we should probably make sure that progress and progress::-moz-progress-bar have -moz-appearance: none if one of them has the property set to none.

FWIW, if you do: progress::-webkit-progress-bar-value { background-color: red; }, the rendering fallback to the default one (non-native) for the bar and the border. I guess that means Chrome's version of Webkit is doing 1 and 2.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Is that what we want?

So, this is working but I wonder if that's the best way of doing it. For the code but also for the rules I'm using.

Here is a description of what this patch does:
(let's call "progress" the nsProgressFrame, created with <progress></progress> and "bar" the anonymous child that represents the bar)
- If border or background is set on progress, it isn't using the native style ;
- If progress isn't using the native style (means explicitly set -moz-appearance: none), bar isn't using the native style ;
- If progress is styled (means border or background are set), bar isn't using the native style ;
- If bar isn't using the native style (means explicitly set -moz-appearance: none), progress isn't using the native style.

I've been thinking of making progress and bar not native if background or border are used on bar but I realized it would make things even uglier and it's not really useful. The end of comment 0 is actually wrong: Webkit (at least Chrome's version) doesn't do that. Maybe it was when I wrote the comment but not now.

So, I wonder if the rules are correct. In addition, I wonder if checking everything in "IsWidgetStyled" is the best idea. It seems that the code is doing a bit more than checking if the widget is styled.
In addition, I'm somewhat duplicating the code between nsProgressFrame and IsWidgetStyled. The only way I see to share this logic would be to have a nsIProgressFrame (which doesn't seem desirable) or add a method to nsIFrame (doesn't seem better). Or maybe there is already something that I don't know?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Doing this in IsWidgetStyled makes sense to me.

You could just do_QueryFrame to nsProgressFrame, no?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> You could just do_QueryFrame to nsProgressFrame, no?

That means exporting nsProgressFrame.h which requires exporting a lot of headers. Do we want to do that?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Just add it to LOCAL_INCLUDES instead of exporting.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

The QueryFrame fix could move to the proper patch but I guess it's no big deal to have it here.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Tests were missing.

Comment 7

[Mounir Lamouri (:mounir)]

We are close to have <progress> ready to be pushed (and thus being in the FF6 train). Do you think you can review that patch soon?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah.  I'll try to do it today.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 521516 [details] [diff] [review]
Patch v1.1

>+  // - both frames don't have author specify rules setting the border or the

"neither frame has author specified rules ..."

>+   * @return whether the frame and its child should use the native style.

That's totally redundant.  Just nix it.

There's no need to pass aPresContext to ShouldUseNativeStyle(); just use PresContext() in there.

r=me with those nits addressed.  Sorry this took so long.  :(

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 11

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/2bfb06f51613

Comment 12

[Shawn Wilsher :sdwilsh]

Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.

Comment 13

[Mounir Lamouri (:mounir)]

The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/624f19ac853d

Comment 14

[Eric Shepherd [:sheppy]]

I don't think this requires documentation, as it makes the behavior work the way people would expect it to work. Any disagreement?

Comment 15

[Mounir Lamouri (:mounir)]

I agree.