Closed Bug 638176 Opened 9 years ago Closed 9 years ago

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


(Core :: Layout: Form Controls, defect)

Not set





(Reporter: mounir, Assigned: mounir)




(2 files, 2 obsolete files)

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.
Blocks: 642667
Attached patch Is that what we want? (obsolete) — Splinter Review
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?
Assignee: nobody → mounir.lamouri
Attachment #520277 - Flags: feedback?(bzbarsky)
Whiteboard: [needs feedback]
Summary: Make <progress> friendly with -moz-appearance:none → Make <progress> friendly with -moz-appearance:none and author styling
Doing this in IsWidgetStyled makes sense to me.

You could just do_QueryFrame to nsProgressFrame, no?
(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?
Just add it to LOCAL_INCLUDES instead of exporting.
Attached patch Patch v1 (obsolete) — Splinter Review
The QueryFrame fix could move to the proper patch but I guess it's no big deal to have it here.
Attachment #520277 - Attachment is obsolete: true
Attachment #520277 - Flags: feedback?(bzbarsky)
Attachment #521322 - Flags: review?(bzbarsky)
Whiteboard: [needs feedback] → [needs review]
Attached patch Patch v1.1Splinter Review
Tests were missing.
Attachment #521322 - Attachment is obsolete: true
Attachment #521322 - Flags: review?(bzbarsky)
Attachment #521516 - Flags: review?(bzbarsky)
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?
Yeah.  I'll try to do it today.
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.  :(
Attachment #521516 - Flags: review?(bzbarsky) → review+
Attached patch Inter diffSplinter Review
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6
Depends on: 655860
Backed out in to resolve bug 655860.
Resolution: FIXED → ---
The regression wasn't caused by these patches. Re-landed:
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
No longer depends on: 655860
I don't think this requires documentation, as it makes the behavior work the way people would expect it to work. Any disagreement?
I agree.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.