Last Comment Bug 638176 - Make <progress> friendly with -moz-appearance:none and author styling
: Make <progress> friendly with -moz-appearance:none and author styling
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
: Jet Villegas (:jet)
Depends on:
Blocks: 633207 642667
  Show dependency treegraph
Reported: 2011-03-02 11:54 PST by Mounir Lamouri (:mounir)
Modified: 2011-06-01 02:32 PDT (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Is that what we want? (6.73 KB, patch)
2011-03-18 12:32 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (7.33 KB, patch)
2011-03-23 14:53 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (10.44 KB, patch)
2011-03-24 09:30 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review
Inter diff (4.61 KB, patch)
2011-05-05 06:05 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description User image Mounir Lamouri (:mounir) 2011-03-02 11:54:21 PST
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 User image Mounir Lamouri (:mounir) 2011-03-18 12:32:24 PDT
Created attachment 520277 [details] [diff] [review]
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 User image Boris Zbarsky [:bz] (still a bit busy) 2011-03-18 12:43:19 PDT
Doing this in IsWidgetStyled makes sense to me.

You could just do_QueryFrame to nsProgressFrame, no?
Comment 3 User image Mounir Lamouri (:mounir) 2011-03-22 05:29:53 PDT
(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 User image Boris Zbarsky [:bz] (still a bit busy) 2011-03-22 08:19:12 PDT
Just add it to LOCAL_INCLUDES instead of exporting.
Comment 5 User image Mounir Lamouri (:mounir) 2011-03-23 14:53:41 PDT
Created attachment 521322 [details] [diff] [review]
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 User image Mounir Lamouri (:mounir) 2011-03-24 09:30:28 PDT
Created attachment 521516 [details] [diff] [review]
Patch v1.1

Tests were missing.
Comment 7 User image Mounir Lamouri (:mounir) 2011-05-04 09:21:25 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2011-05-04 09:43:13 PDT
Yeah.  I'll try to do it today.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2011-05-04 12:42:15 PDT
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 User image Mounir Lamouri (:mounir) 2011-05-05 06:05:44 PDT
Created attachment 530288 [details] [diff] [review]
Inter diff
Comment 11 User image Mounir Lamouri (:mounir) 2011-05-09 05:38:19 PDT
Comment 12 User image Shawn Wilsher :sdwilsh 2011-05-09 16:12:03 PDT
Backed out in to resolve bug 655860.
Comment 13 User image Mounir Lamouri (:mounir) 2011-05-10 06:58:10 PDT
The regression wasn't caused by these patches. Re-landed:
Comment 14 User image Eric Shepherd [:sheppy] 2011-05-31 13:36:49 PDT
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 User image Mounir Lamouri (:mounir) 2011-06-01 02:32:45 PDT
I agree.

Note You need to log in before you can comment on or make changes to this bug.