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

RESOLVED FIXED in mozilla6

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 642667
(Assignee)

Comment 1

6 years ago
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?
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #520277 - Flags: feedback?(bzbarsky)
(Assignee)

Updated

6 years ago
Whiteboard: [needs feedback]
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
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.
Attachment #520277 - Attachment is obsolete: true
Attachment #520277 - Flags: feedback?(bzbarsky)
Attachment #521322 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Whiteboard: [needs feedback] → [needs review]
(Assignee)

Comment 6

6 years ago
Created attachment 521516 [details] [diff] [review]
Patch v1.1

Tests were missing.
Attachment #521322 - Attachment is obsolete: true
Attachment #521322 - Flags: review?(bzbarsky)
Attachment #521516 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

6 years ago
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+
(Assignee)

Comment 10

6 years ago
Created attachment 530288 [details] [diff] [review]
Inter diff
(Assignee)

Comment 11

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/2bfb06f51613
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6

Updated

6 years ago
Depends on: 655860
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

6 years ago
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/624f19ac853d
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 15

6 years ago
I agree.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.