Have a native rendering for <progress> on Windows Vista and Windows 7

VERIFIED FIXED in mozilla6

Status

()

Core
Widget: Win32
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 519149 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK
Attachment #519149 - Flags: review?(doug.turner)

Comment 1

6 years ago
what is the purpose of this change?

does it make sense to put up a before/after screenshot?
(Assignee)

Comment 2

6 years ago
The purpose is to use the correct Windows Vista/7 API for progress bars instead of the XP one. Our rendering of the progress bar on Windows Vista/7 isn't really native for the moment. You can see that by downloading a file on Windows Vista/7 and look at the rendering. Then, look at what a native progress bar look like.

I will try to put some screenshots...
(Assignee)

Comment 3

6 years ago
Created attachment 519391 [details]
Before/After screenshot

The difference might not be really obvious on the screenshot because there is an animated glow on progress bars on Windows Vista/7. You can see that by openning this data URL on Chrome: data:text/html,<progress value=1></progress>
(Assignee)

Comment 4

6 years ago
Created attachment 519435 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK

Fix a warning.
Attachment #519149 - Attachment is obsolete: true
Attachment #519149 - Flags: review?(doug.turner)
Attachment #519435 - Flags: review?(doug.turner)
(Assignee)

Comment 5

6 years ago
Created attachment 519436 [details] [diff] [review]
Part 2 - Add the animated glow

Unfortunately, our code isn't great for animations. To prevent keeping too many static variables, I'm only using PR_IntervalNow(). That means the position of the glow when the page is loaded isn't guaranteed to be at the left of the element.
In addition, all progress bars will share the same glow position. That means if you create multiple progress bars with the same size, the glow will be perfectly synchronized (even if some progress bars are added later).

I will refactor this code and create proper constants when I will implement the indeterminate rendering.
Attachment #519436 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Blocks: 641905
(Assignee)

Comment 6

6 years ago
If you want to compare the rendering, please use this build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-d171ffeea1be/ (before)
and this one:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-3665ab582961/ (after)

And open this data url: data:text/html,<progress value=1></progress>
(Assignee)

Updated

6 years ago
Blocks: 641942
(Assignee)

Updated

6 years ago
Blocks: 642127
(Assignee)

Comment 7

6 years ago
Created attachment 519973 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK

Use consts and moving the review to Jim so he can review all the patches involved here.
Attachment #519435 - Attachment is obsolete: true
Attachment #519435 - Flags: review?(doug.turner)
Attachment #519973 - Flags: review?(jmathies)
(Assignee)

Comment 8

6 years ago
Created attachment 519974 [details] [diff] [review]
Part 2 - Add the animated glow
Attachment #519436 - Attachment is obsolete: true
Attachment #519436 - Flags: review?(doug.turner)
Attachment #519974 - Flags: review?(jmathies)
(Assignee)

Comment 9

6 years ago
Created attachment 519980 [details] [diff] [review]
Part 2 - Add the animated glow
Attachment #519974 - Attachment is obsolete: true
Attachment #519974 - Flags: review?(jmathies)
Attachment #519980 - Flags: review?(jmathies)

Comment 10

6 years ago
Comment on attachment 519973 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK

Review of attachment 519973 [details] [diff] [review]:

r+
Attachment #519973 - Flags: review?(jmathies) → review+

Comment 11

6 years ago
Comment on attachment 519980 [details] [diff] [review]
Part 2 - Add the animated glow

Review of attachment 519980 [details] [diff] [review]:

r+
Attachment #519980 - Flags: review?(jmathies) → review+

Updated

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

Updated

6 years ago
Whiteboard: [ready to land][waits for dependencies]
(Assignee)

Comment 12

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/f8ab655d503c
http://hg.mozilla.org/mozilla-central/rev/4a7b8019b83f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
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 14

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

Updated

6 years ago
No longer depends on: 655860

Updated

6 years ago
Depends on: 656592

Updated

6 years ago
Duplicate of this bug: 378276

Comment 16

6 years ago
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

Verified patch on Win7 and animation is present -  behavior is a bit different from what can be observed in Chrome, the animation(the glow) being slower on Firefox. Was that expected?
(Assignee)

Comment 17

6 years ago
(In reply to comment #16)
> Verified patch on Win7 and animation is present -  behavior is a bit
> different from what can be observed in Chrome, the animation(the glow) being
> slower on Firefox. Was that expected?

Our goal is certainly not to copy all Chrome bits...

Comment 18

6 years ago
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110725 Firefox/8.0a1

Considering last comment, setting status to Verified Fixed.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Depends on: 711962
You need to log in before you can comment on or make changes to this bug.